diff mbox

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

Message ID 20170223141932.31110-3-michael@niedermayer.cc
State Accepted
Commit 5804201cbac2de8824013a8294e381e93bbe45f2
Headers show

Commit Message

Michael Niedermayer Feb. 23, 2017, 2:19 p.m. UTC
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@niedermayer.cc>
---
 libavutil/frame.c | 55 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 29 insertions(+), 26 deletions(-)

Comments

wm4 Feb. 23, 2017, 2:26 p.m. UTC | #1
On Thu, 23 Feb 2017 15:19:31 +0100
Michael Niedermayer <michael@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@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?
Michael Niedermayer Feb. 23, 2017, 4:12 p.m. UTC | #2
On Thu, Feb 23, 2017 at 03:26:22PM +0100, wm4 wrote:
> On Thu, 23 Feb 2017 15:19:31 +0100
> Michael Niedermayer <michael@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@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.

[...]
wm4 Feb. 23, 2017, 4:53 p.m. UTC | #3
On Thu, 23 Feb 2017 17:12:35 +0100
Michael Niedermayer <michael@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@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@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.

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.
Michael Niedermayer Feb. 24, 2017, 12:19 a.m. UTC | #4
On Thu, Feb 23, 2017 at 05:53:27PM +0100, wm4 wrote:
> On Thu, 23 Feb 2017 17:12:35 +0100
> Michael Niedermayer <michael@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@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@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

[...]
Carl Eugen Hoyos Feb. 24, 2017, 1:24 a.m. UTC | #5
2017-02-24 1:19 GMT+01:00 Michael Niedermayer <michael@niedermayer.cc>:

> 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

No, it's preferable to fix the original issue, not adding random
work-arounds all over the codebase.

Thank you, Carl Eugen
diff mbox

Patch

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,