[FFmpeg-devel] [PATCH] fix vorbis decoder amplitude bits handling

[Alex Converse]

On Thu, Nov 18, 2010 at 6:12 AM, Yuriy Kaminskiy <yumkam at mail.ru> wrote:
> Alex Converse wrote:
>> On Tue, Nov 16, 2010 at 2:42 AM, Yuriy Kaminskiy <yumkam at mail.ru> wrote:
>>> Baptiste Coudurier wrote:
>>>> Hi
>>>>
>>>> $subject. Please comment.
>>>>
>>>>
>>>>
>>>> ------------------------------------------------------------------------
>>>>
>>>> Index: libavcodec/vorbis_dec.c
>>>> ===================================================================
>>>> --- libavcodec/vorbis_dec.c ? (revision 25756)
>>>> +++ libavcodec/vorbis_dec.c ? (working copy)
>>>> @@ -563,6 +563,11 @@
>>>> ? ? ? ? ? ? ? ? ? ? ? "Floor 0 amplitude bits is 0.\n");
>>>> ? ? ? ? ? ? ? ?return -1;
>>>> ? ? ? ? ? ? ?}
>>>> + ? ? ? ? ? ?if (floor_setup->data.t0.amplitude_bits > 32) {
>>> Not enough, s/>/>=/, result of 1<<32 is undefined (and I'm sure code below
>>> really broken on x86 when amplitude_bits == 32, resulting in divide-by-zero):
>>> === cut ===
>>> ? ? ? ? ? ? ? ? ? ?q = exp((((amplitude*vf->amplitude_offset) /
>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(((1 << vf->amplitude_bits) - 1) * sqrt(p + q)))
>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? - vf->amplitude_offset) * .11512925f);
>>> === cut ===
>>>
>>
>> Libvorbis does that. When in Rome...
>
> Then libvorbis also have divide-by-zero error, someone should report them :-|

Dividing by (float)0 is not an error.

> Besides, ampbits > 32 will result in reading outside of `mask' array inside
> oggpack_read [in addition to producing totally bogus result].
>

yes ampbits > 32 is clearly wrong. What I'm curious about is the fencepost case.

> Fortunately, floor0 in vorbis is very obsolete [it is only produced by libvorbis
> before 1.0beta4 AFAIR] and rare; so I think it is totally safe to simply reject
> files with ampbits >= 32.
>

Did anyone save the quote from monty from IRC a few days ago?