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

Ronald S. Bultje rsbultje at gmail.com
Sun Jun 28 13:13:13 CEST 2015


Hi,

On Sun, Jun 28, 2015 at 5:28 AM, Andreas Cadhalpun <
andreas.cadhalpun at googlemail.com> wrote:

> On 27.06.2015 23:01, Michael Niedermayer wrote:
> > On Sat, Jun 27, 2015 at 08:36:15PM +0200, Andreas Cadhalpun wrote:
> >> Claiming to have decoded more bytes than the packet size is wrong.
> >>
> >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> >> ---
> >>  libavcodec/wmavoice.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
> >> index ae88d4e..6cd407a 100644
> >> --- a/libavcodec/wmavoice.c
> >> +++ b/libavcodec/wmavoice.c
> >> @@ -1982,7 +1982,7 @@ static int wmavoice_decode_packet(AVCodecContext
> *ctx, void *data,
> >>                      *got_frame_ptr) {
> >>                      cnt += s->spillover_nbits;
> >>                      s->skip_bits_next = cnt & 7;
> >> -                    return cnt >> 3;
> >> +                    return FFMIN(cnt >> 3, avpkt->size);
> >>                  } else
> >>                      skip_bits_long (gb, s->spillover_nbits - cnt +
> >>                                      get_bits_count(gb)); // resync
> >> @@ -2001,7 +2001,7 @@ static int wmavoice_decode_packet(AVCodecContext
> *ctx, void *data,
> >>      } else if (*got_frame_ptr) {
> >>          int cnt = get_bits_count(gb);
> >>          s->skip_bits_next = cnt & 7;
> >> -        return cnt >> 3;
> >> +        return FFMIN(cnt >> 3, avpkt->size);
> >>      } else if ((s->sframe_cache_size = pos) > 0) {
> >>          /* rewind bit reader to start of last (incomplete)
> superframe... */
> >>          init_get_bits(gb, avpkt->data, size << 3);
> >
> > am i assuming correct that gb was read beyond its end ?
>
> That only happens in the second case, not in the first.
>
> > if so this maybe should be treated as an error instead of cliping
>
> 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.

In the second case, does that actually happen? Wmavoice is one of the
limited number of decoders that internally checks for overreads.
get_bits_count() should never overread. Do you have samples for which this
happens? 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).

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

Ronald


More information about the ffmpeg-devel mailing list