[FFmpeg-devel] [PATCH] avcodec/hevcdec: do not let updated extradata corrupt state

[wm4]

On Wed, 5 Jul 2017 16:08:38 +0200
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Wed, Jul 05, 2017 at 09:56:10AM +0200, wm4 wrote:
> > On Tue,  4 Jul 2017 22:33:52 +0200
> > Michael Niedermayer <michael at niedermayer.cc> wrote:
> >   
> > > Fixes: out of array access
> > > Fixes: 2451/clusterfuzz-testcase-minimized-4781613957251072
> > > 
> > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > ---
> > >  libavcodec/hevcdec.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
> > > index cc8ac82164..55f51211c3 100644
> > > --- a/libavcodec/hevcdec.c
> > > +++ b/libavcodec/hevcdec.c
> > > @@ -3057,7 +3057,7 @@ static int verify_md5(HEVCContext *s, AVFrame *frame)
> > >      return 0;
> > >  }
> > >  
> > > -static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length)
> > > +static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length, int first)
> > >  {
> > >      int ret, i;
> > >  
> > > @@ -3069,7 +3069,7 @@ static int hevc_decode_extradata(HEVCContext *s, uint8_t *buf, int length)
> > >  
> > >      /* export stream parameters from the first SPS */
> > >      for (i = 0; i < FF_ARRAY_ELEMS(s->ps.sps_list); i++) {
> > > -        if (s->ps.sps_list[i]) {
> > > +        if (first && s->ps.sps_list[i]) {
> > >              const HEVCSPS *sps = (const HEVCSPS*)s->ps.sps_list[i]->data;
> > >              export_stream_params(s->avctx, &s->ps, sps);
> > >              break;
> > > @@ -3099,7 +3099,7 @@ static int hevc_decode_frame(AVCodecContext *avctx, void *data, int *got_output,
> > >      new_extradata = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA,
> > >                                              &new_extradata_size);
> > >      if (new_extradata && new_extradata_size > 0) {
> > > -        ret = hevc_decode_extradata(s, new_extradata, new_extradata_size);
> > > +        ret = hevc_decode_extradata(s, new_extradata, new_extradata_size, 0);
> > >          if (ret < 0)
> > >              return ret;
> > >      }
> > > @@ -3387,7 +3387,7 @@ static av_cold int hevc_decode_init(AVCodecContext *avctx)
> > >          s->threads_number = 1;
> > >  
> > >      if (avctx->extradata_size > 0 && avctx->extradata) {
> > > -        ret = hevc_decode_extradata(s, avctx->extradata, avctx->extradata_size);
> > > +        ret = hevc_decode_extradata(s, avctx->extradata, avctx->extradata_size, 1);
> > >          if (ret < 0) {
> > >              hevc_decode_free(avctx);
> > >              return ret;  
> > 
> > Couldn't that have done in a less confusing way? What the heck does
> > "first" even mean? (Also you have to look up what that means on the
> > caller site.)
> > 
> > Can you explain this?  
> 
> first means "Preceding all others of a series or kind;" https://en.wiktionary.org/wiki/first

Thanks for the English lesson. By the way, you're the only person on
the internet who uses "iam".

> in the context of hevc_decode_extradata(), a function decoding extradata
> that signifies the first of a series of extradata.
> 
> No question it can be done differently, everything can be done
> differently.

So can you explain why "first" is checked every loop, instead of
putting the loop into an if, or (what should have been done) moving the
entire loop out of the function and putting it inline into the only
place where it's effectively used?