[FFmpeg-devel] [libav-devel] [PATCH] wmavoice: limit wmavoice_decode_packet return value to packet size

[Andreas Cadhalpun]

On 28.06.2015 16:26, Michael Niedermayer wrote:
> On Sun, Jun 28, 2015 at 01:39:41PM +0200, Andreas Cadhalpun wrote:
>> On 28.06.2015 13:13, Ronald S. Bultje wrote:
>>> On Sun, Jun 28, 2015 at 5:28 AM, Andreas Cadhalpun
>>>> Treating one like an error, but not the other seems strange as well.
>>>> One could add an explode mode for both. Would that be better?
>>>
>>>
>>> In the first case, it's an error. If the frame size is 2 bits, the header
>>> is 1, and it specifies a spillover bits of 2, then the frame is clearly
>>> corrupt. Returning an error is fine. The ffmin() isn't necessary. I also
>>> don't think an explode mode check is necessary here, it's a clear error
>>> that is unrecoverable for this frame.
>>
>> I've no strong preference. Attached is a variant just returning an error.
>>
>>> In the second case, does that actually happen?
>>
>> Yes.
>>
>>> Wmavoice is one of the
>>> limited number of decoders that internally checks for overreads.
>>> get_bits_count() should never overread.
>>
>> It doesn't check everywhere:
>>     /* Statistics? FIXME - we don't check for length, a slight overrun
>>      * will be caught by internal buffer padding, and anything else
>>      * will be skipped, not read. */
>>     if (get_bits1(gb)) {
>>         res = get_bits(gb, 4);
>>         skip_bits(gb, 10 * (res + 1));
>>     }
>>
>>> Do you have samples for which this happens?
>>
>> Yes.
>>
>>> We currently basically return an error on any possible overread
>>> signified in the bitstream (without actually overreading), so doing so here
>>> also would make sense (if it really happens at all).
>>
>> OK.
>>
>>> (We could also remove all the overread checks in the decoder, make it use
>>> the safe bitstream reader mode, and then check for overreads at the end of
>>> synth_superframe or in the caller, and then return an error. I have no
>>> specific preference, and this may lead to less code overall.)
>>
>> That should work as well, but would be a larger change.
>>
>> Best regards,
>> Andreas
> 
>>  wmavoice.c |   18 ++++++++++++++++--
>>  1 file changed, 16 insertions(+), 2 deletions(-)
>> 6c2b085ae9d64d9d8ae33efa4f554f23df98922c  0001-wmavoice-limit-wmavoice_decode_packet-return-value-t.patch
>> From 34aa9dd95c57039d56a07d0c9dfc837dd2199af8 Mon Sep 17 00:00:00 2001
>> From: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
>> Date: Sun, 28 Jun 2015 12:40:12 +0200
>> Subject: [PATCH] wmavoice: limit wmavoice_decode_packet return value to packet
>>  size
>>
>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> 
> LGTM

OK, I pushed this variant.

Best regards,
Andreas