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

Michael Niedermayer michael at niedermayer.cc
Fri Feb 24 02:19:59 EET 2017


On Thu, Feb 23, 2017 at 05:53:27PM +0100, wm4 wrote:
> On Thu, 23 Feb 2017 17:12:35 +0100
> Michael Niedermayer <michael at niedermayer.cc> wrote:
> 
> > 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.
> 
> Actually it looks like the Libav version doesn't even have this
> per side-data buffer ref, which makes it so different.
> 
> I'm not sure what's the actual problem here. If size==0, then data can
> be NULL just fine? If something accesses data anyway (even if size==0),
> that should probably be fixed instead.

The problem is that functions like memcpy dont allow NULL even with
a size of 0

i can add checks for every memcpy() and any other function that ubsan
finds if thats preferred or add a av_memcpy() that does allow NULL

eliminating the 0 case seemed better to me


> 
> In general it feels like the change makes the new code slightly
> trickier than necessary (and the forward declaration looks ugly), but
> feel free to ignore me.

i intended to remove the forward declaration in a seperate change
moving the function. I did it with a forw declare in the patch so
the diff shows the differences of the function clearer

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

Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- Plato
-------------- 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/20170224/c9292c11/attachment.sig>


More information about the ffmpeg-devel mailing list