[MPlayer-dev-eng] vbr mp3 runtime

[Ingo Brückl]

Reimar Döffinger wrote on Thu, 1 Sep 2011 23:16:39 +0200:

> 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.

So you're encouraging me?

> 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.

This is why I called it a "feasibility study". mp3_vbr_frames() is far from
being finished.

> I also think there was some bugzilla with a finished patch actually...

If you refer to #465, that won't help much (though it's nice to see the
runtime constantly making new guesses in the GUI).

Based on the bugzilla reports, don't you think it maybe *is* a good idea to
have vbr support?

I'm willing to put work into mp3_vbr_frames() to the best of my knowledge,
but not if you are seriously doubting that it's a good idea, which (to my
understanding) sounds like I'd waste my time.

Ingo