diff mbox

[FFmpeg-devel] frame: add an av_frame_new_side_data_from_buf function

Message ID 20180301185828.4181-1-atomnuker@gmail.com
State New
Headers show

Commit Message

Rostislav Pehlivanov March 1, 2018, 6:58 p.m. UTC 
Also fixes a theoretical memory leak in av_frame_new_side_data_from_buf
where if ret was successfully allocated but the realloc call failed,
ret wouldn't be freed.

Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
---
 libavutil/frame.c | 42 +++++++++++++++++++++---------------------
 libavutil/frame.h | 16 ++++++++++++++++
 2 files changed, 37 insertions(+), 21 deletions(-)

Comments

James Almer March 1, 2018, 7:19 p.m. UTC | #1 
On 3/1/2018 3:58 PM, Rostislav Pehlivanov wrote:
> Also fixes a theoretical memory leak in av_frame_new_side_data_from_buf
> where if ret was successfully allocated but the realloc call failed,
> ret wouldn't be freed.

Currently, if realloc fails, ret is never allocated to begin with.

There's no leak at all in any circumstances right now. That change
you're trying to make is unnecessary.
Rostislav Pehlivanov March 1, 2018, 8:45 p.m. UTC | #2 
On 1 March 2018 at 19:19, James Almer <jamrial@gmail.com> wrote:

> On 3/1/2018 3:58 PM, Rostislav Pehlivanov wrote:
> > Also fixes a theoretical memory leak in av_frame_new_side_data_from_buf
> > where if ret was successfully allocated but the realloc call failed,
> > ret wouldn't be freed.
>
> Currently, if realloc fails, ret is never allocated to begin with.
>
> There's no leak at all in any circumstances right now. That change
> you're trying to make is unnecessary.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

I see what you mean, I don't remember why I moved that back in November.

Pushed, thanks for the reviews
diff mbox

Patch

diff --git a/libavutil/frame.c b/libavutil/frame.c
index 662a7e5ab5..4951e48045 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -26,11 +26,6 @@ 
 #include "mem.h"
 #include "samplefmt.h"
 
-
-static AVFrameSideData *frame_new_side_data(AVFrame *frame,
-                                            enum AVFrameSideDataType type,
-                                            AVBufferRef *buf);
-
 #if FF_API_FRAME_GET_SET
 MAKE_ACCESSORS(AVFrame, frame, int64_t, best_effort_timestamp)
 MAKE_ACCESSORS(AVFrame, frame, int64_t, pkt_duration)
@@ -356,8 +351,10 @@  FF_ENABLE_DEPRECATION_WARNINGS
             }
             memcpy(sd_dst->data, sd_src->data, sd_src->size);
         } else {
-            sd_dst = frame_new_side_data(dst, sd_src->type, av_buffer_ref(sd_src->buf));
+            AVBufferRef *ref = av_buffer_ref(sd_src->buf);
+            sd_dst = av_frame_new_side_data_from_buf(dst, sd_src->type, ref);
             if (!sd_dst) {
+                av_buffer_unref(&ref);
                 wipe_side_data(dst);
                 return AVERROR(ENOMEM);
             }
@@ -642,9 +639,9 @@  AVBufferRef *av_frame_get_plane_buffer(AVFrame *frame, int plane)
     return NULL;
 }
 
-static AVFrameSideData *frame_new_side_data(AVFrame *frame,
-                                            enum AVFrameSideDataType type,
-                                            AVBufferRef *buf)
+AVFrameSideData *av_frame_new_side_data_from_buf(AVFrame *frame,
+                                                 enum AVFrameSideDataType type,
+                                                 AVBufferRef *buf)
 {
     AVFrameSideData *ret, **tmp;
 
@@ -652,18 +649,20 @@  static AVFrameSideData *frame_new_side_data(AVFrame *frame,
         return NULL;
 
     if (frame->nb_side_data > INT_MAX / sizeof(*frame->side_data) - 1)
-        goto fail;
+        return NULL;
+
+    ret = av_mallocz(sizeof(*ret));
+    if (!ret)
+        return NULL;
 
     tmp = av_realloc(frame->side_data,
                      (frame->nb_side_data + 1) * sizeof(*frame->side_data));
-    if (!tmp)
-        goto fail;
+    if (!tmp) {
+        av_free(ret);
+        return NULL;
+    }
     frame->side_data = tmp;
 
-    ret = av_mallocz(sizeof(*ret));
-    if (!ret)
-        goto fail;
-
     ret->buf = buf;
     ret->data = ret->buf->data;
     ret->size = buf->size;
@@ -672,17 +671,18 @@  static AVFrameSideData *frame_new_side_data(AVFrame *frame,
     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 *ret;
+    AVBufferRef *buf = av_buffer_alloc(size);
+    ret = av_frame_new_side_data_from_buf(frame, type, buf);
+    if (!ret)
+        av_buffer_unref(&buf);
+    return ret;
 }
 
 AVFrameSideData *av_frame_get_side_data(const AVFrame *frame,
diff --git a/libavutil/frame.h b/libavutil/frame.h
index d54bd9a354..59cee8ceab 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -800,6 +800,22 @@  AVFrameSideData *av_frame_new_side_data(AVFrame *frame,
                                         enum AVFrameSideDataType type,
                                         int size);
 
+/**
+ * Add a new side data to a frame from an existing AVBufferRef
+ *
+ * @param frame a frame to which the side data should be added
+ * @param type  the type of the added side data
+ * @param buf   an AVBufferRef to add as side data. The ownership of
+ *              the reference is transferred to the frame.
+ *
+ * @return newly added side data on success, NULL on error. On failure
+ *         the frame is unchanged and the AVBufferRef remains owned by
+ *         the caller.
+ */
+AVFrameSideData *av_frame_new_side_data_from_buf(AVFrame *frame,
+                                                 enum AVFrameSideDataType type,
+                                                 AVBufferRef *buf);
+
 /**
  * @return a pointer to the side data of a given type on success, NULL if there
  * is no side data with such type in this frame.