[FFmpeg-devel] [PATCH 3/4] avutil/frame: Reimplement av_frame_new_side_data() without size=0 special case

[Michael Niedermayer]

On Thu, Feb 23, 2017 at 03:26:22PM +0100, wm4 wrote:
> On Thu, 23 Feb 2017 15:19:31 +0100
> Michael Niedermayer <michael at niedermayer.cc> wrote:
> 
> > The size 0 special case causes side data to be created which is
> > different and a special case if for any reasons size = 0 is passed
> > 
> > Fixes: multiple runtime error: null pointer passed as argument 1, which is declared to never be null
> > Fixes: 653/clusterfuzz-testcase-5773837415219200
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> > 
> > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > ---
> >  libavutil/frame.c | 55 +++++++++++++++++++++++++++++--------------------------
> >  1 file changed, 29 insertions(+), 26 deletions(-)
> > 
> > diff --git a/libavutil/frame.c b/libavutil/frame.c
> > index 2913982e91..8811dcdcfe 100644
> > --- a/libavutil/frame.c
> > +++ b/libavutil/frame.c
> > @@ -26,6 +26,11 @@
> >  #include "mem.h"
> >  #include "samplefmt.h"
> >  
> > +
> > +static AVFrameSideData *frame_new_side_data(AVFrame *frame,
> > +                                            enum AVFrameSideDataType type,
> > +                                            AVBufferRef *buf);
> > +
> >  MAKE_ACCESSORS(AVFrame, frame, int64_t, best_effort_timestamp)
> >  MAKE_ACCESSORS(AVFrame, frame, int64_t, pkt_duration)
> >  MAKE_ACCESSORS(AVFrame, frame, int64_t, pkt_pos)
> > @@ -344,20 +349,11 @@ FF_ENABLE_DEPRECATION_WARNINGS
> >              }
> >              memcpy(sd_dst->data, sd_src->data, sd_src->size);
> >          } else {
> > -            sd_dst = av_frame_new_side_data(dst, sd_src->type, 0);
> > +            sd_dst = frame_new_side_data(dst, sd_src->type, av_buffer_ref(sd_src->buf));
> >              if (!sd_dst) {
> >                  wipe_side_data(dst);
> >                  return AVERROR(ENOMEM);
> >              }
> > -            if (sd_src->buf) {
> > -                sd_dst->buf = av_buffer_ref(sd_src->buf);
> > -                if (!sd_dst->buf) {
> > -                    wipe_side_data(dst);
> > -                    return AVERROR(ENOMEM);
> > -                }
> > -                sd_dst->data = sd_dst->buf->data;
> > -                sd_dst->size = sd_dst->buf->size;
> > -            }
> >          }
> >          av_dict_copy(&sd_dst->metadata, sd_src->metadata, 0);
> >      }
> > @@ -633,40 +629,47 @@ AVBufferRef *av_frame_get_plane_buffer(AVFrame *frame, int plane)
> >      return NULL;
> >  }
> >  
> > -AVFrameSideData *av_frame_new_side_data(AVFrame *frame,
> > -                                        enum AVFrameSideDataType type,
> > -                                        int size)
> > +static AVFrameSideData *frame_new_side_data(AVFrame *frame,
> > +                                            enum AVFrameSideDataType type,
> > +                                            AVBufferRef *buf)
> >  {
> >      AVFrameSideData *ret, **tmp;
> >  
> > -    if (frame->nb_side_data > INT_MAX / sizeof(*frame->side_data) - 1)
> > +    if (!buf)
> >          return NULL;
> >  
> > +    if (frame->nb_side_data > INT_MAX / sizeof(*frame->side_data) - 1)
> > +        goto fail;
> > +
> >      tmp = av_realloc(frame->side_data,
> >                       (frame->nb_side_data + 1) * sizeof(*frame->side_data));
> >      if (!tmp)
> > -        return NULL;
> > +        goto fail;
> >      frame->side_data = tmp;
> >  
> >      ret = av_mallocz(sizeof(*ret));
> >      if (!ret)
> > -        return NULL;
> > -
> > -    if (size > 0) {
> > -        ret->buf = av_buffer_alloc(size);
> > -        if (!ret->buf) {
> > -            av_freep(&ret);
> > -            return NULL;
> > -        }
> > +        goto fail;
> >  
> > -        ret->data = ret->buf->data;
> > -        ret->size = size;
> > -    }
> > +    ret->buf = buf;
> > +    ret->data = ret->buf->data;
> > +    ret->size = buf->size;
> >      ret->type = type;
> >  
> >      frame->side_data[frame->nb_side_data++] = ret;
> >  
> >      return ret;
> > +fail:
> > +    av_buffer_unref(&buf);
> > +    return NULL;
> > +}
> > +
> > +AVFrameSideData *av_frame_new_side_data(AVFrame *frame,
> > +                                        enum AVFrameSideDataType type,
> > +                                        int size)
> > +{
> > +
> > +    return frame_new_side_data(frame, type, av_buffer_alloc(size));
> >  }
> >  
> >  AVFrameSideData *av_frame_get_side_data(const AVFrame *frame,
> 
> Why not restore Libav's version, which doesn't seem to need all this?

it would remove optimizations and who knows what else. Also iam
interrested in fixing security and undefined behavor issues in FFmpeg.
The code in FFmpeg has been extensivly tested within FFmpeg and
simply throwing it out and replacing it by something else would
bring us to code that has not been tested in FFmpeg.

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

You can kill me, but you cannot change the truth.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170223/f372a0aa/attachment.sig>