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

[Michael Niedermayer]

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

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150628/0a3a362c/attachment.asc>