[MPlayer-dev-eng] vbr mp3 runtime

[Reimar Döffinger]

On Thu, Sep 01, 2011 at 11:01:18PM +0200, Ingo Brückl wrote:
> Reimar Döffinger wrote on Thu, 1 Sep 2011 00:24:06 +0200:
> 
> > On Mon, Aug 29, 2011 at 04:49:37AM +0200, Ingo Brückl wrote:
> >> compn wrote on Sun, 28 Aug 2011 20:31:22 -0400:
> >>
> >> > On Mon, 29 Aug 2011 01:52:15 +0200, Ingo Brückl wrote:
> >> >>Seems to work just fine.
> >>
> >> > have you tried it with an mp3 file and a fuzzer (like zzuf) to make
> >> > sure it doesnt crash or leak mem etc?
> >>
> >> Currently it's just a feasibility study. I won't put work into it until I
> >> know that it will be accepted in principle.
> 
> > However exactly that is exactly the problem.
> > After the last time such stuff was added it ended up with about
> > one major security hole per line (and lots of lines of code)
> 
> Hmm, what can possibly become a security hole? The worst thing that can
> happen is that we get a wrong number of frames leading to a wrong value
> for i_bps if a vbr header exists, which is exactly what we are having at
> the moment.

You might be reading in data incorrectly, the parsing might go wrong,
it might cause a division by 0, negative or 0 values for bitrate which
the code above might not be able to handle... The usually way security holes
happen when dealing with completely unfiltered user data.

> > I am seriously doubting that it's a good idea.
> 
> Well, if it's unwanted ... I just thought it would be nice to have vbr
> support...

If it's simple enough it would still be ok.
I guess your patch could probably be made simple enough by using the
appropriate stream_read_char/stream_read_dword functions, though it
still needs sanity checks and I think it won't handle stray Xing headers
which I think the FFmpeg demuxer can.
Though your patch only adds support for the Xing header, I am quite
sure there is at least one other VBR header.
I also think there was some bugzilla with a finished patch actually...

> > It's possible to just prefer lavf demuxer for those files.
> > No idea if there's any reason not to.
> 
> Yes, it is possible, but because you don't know in advance you have to use
> the lavf demuxer by default. (Which leads me to the question, why do we have
> own demuxers and don't just use ffmepg's? Serious question.)

Because there was no FFmpeg when MPlayer started.
And FFmpeg didn't have demuxers even longer.
And even longer FFmpeg didn't have actually reliably working demuxers.
E.g. it might be that in a streaming context the probing still takes too
long/needs too much data with the lavf demuxer even today - that is
one of the potential issues I could see.
So a bit after the motto "better a few small bugs than a large
regression" progress on moving to lavf is slow.