[FFmpeg-devel] [PATCH] avcodec/utils: do_decode: Assert that the decoder return value is not larger than the packet size

[wm4]

On Sat, 23 Apr 2016 17:35:54 +0200
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Fri, Apr 22, 2016 at 04:04:54PM +0200, wm4 wrote:
> > On Fri, 22 Apr 2016 11:59:14 +0200
> > Michael Niedermayer <michael at niedermayer.cc> wrote:
> >   
> > > On Fri, Apr 22, 2016 at 09:11:52AM +0200, wm4 wrote:  
> > > > On Fri, 22 Apr 2016 05:14:30 +0200
> > > > Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > >     
> > > > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > > > ---
> > > > >  libavcodec/utils.c |    1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> > > > > index 52e0d92..b35fabc 100644
> > > > > --- a/libavcodec/utils.c
> > > > > +++ b/libavcodec/utils.c
> > > > > @@ -2738,6 +2738,7 @@ static int do_decode(AVCodecContext *avctx, AVPacket *pkt)
> > > > >          avctx->internal->draining_done = 1;
> > > > >  
> > > > >      if (ret >= pkt->size) {
> > > > > +        av_assert0(ret == pkt->size);
> > > > >          av_packet_unref(avctx->internal->buffer_pkt);
> > > > >      } else {
> > > > >          int consumed = ret;    
> > > > 
> > > > Basically all code using the "old" API expects that the decoder can
> > > > read beyond the buffer (making use of input padding I suppose). So I
> > > > think this is expected.    
> > > 
> > > iam not aware of any decoder that would return such a overread size
> > > also to double check this yesterday i tested this with a bunch of
> > > fuzzed files before posting the patch and failed to find anything that
> > > triggers the assert
> > > 
> > > [...]  
> > 
> > I'm talking about code like this:
> > 
> >      ret = FFMIN(ret, pkt->size); /* guard against bogus return values */
> >      pkt->data += ret;
> >      pkt->size -= ret;
> > 
> > (currently found in ffprobe)
> > 
> > Checks like this are all over the place.
> > 
> > If you're confident that these checks are unnecessary and that
> > avcodec_decode_audio4 should never return a larger value than the
> > packet size, the assert should be added to avcodec_decode_audio4.
> > 
> > Why do you want to add it specifically to do_decode?  
> 
> just saw the >= check in it and felt that it shouls never be >
> patch posted which adds it to avcodec_decode_audio4
> 
> [...]

Yes, I did that because I assumed it could be higher by design due to
decoder overreads.