diff mbox series

[FFmpeg-devel,1/2] avfilter/vf_scale: fix frame lifetimes

Message ID 20240709093437.16563-1-ffmpeg@haasn.xyz
State New
Headers show
Series [FFmpeg-devel,1/2] avfilter/vf_scale: fix frame lifetimes | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 fail Make fate failed
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Niklas Haas July 9, 2024, 9:34 a.m. UTC 
From: Niklas Haas <git@haasn.dev>

scale_frame() inconsistently handled the lifetime of `in`. Fixes a
possible double free and a possible memory leak.

The new code always has `scale_frame` take over ownership of the input
frame. I first tried writing this code in a way where the calling code
retains ownership, but this is nontrivial due to the presence of the
no-op short-circuit condition in which the input frame is directly
returned. (As an alternative, we could use av_frame_clone() instead, but
I wanted to avoid touching the original behavior in this commit)
---
 libavfilter/vf_scale.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

Comments

Anton Khirnov July 10, 2024, 9:03 a.m. UTC | #1 
Patchset looks ok.
Michael Niedermayer July 10, 2024, 12:29 p.m. UTC | #2 
On Tue, Jul 09, 2024 at 11:34:36AM +0200, Niklas Haas wrote:
> From: Niklas Haas <git@haasn.dev>
> 
> scale_frame() inconsistently handled the lifetime of `in`. Fixes a
> possible double free and a possible memory leak.
> 
> The new code always has `scale_frame` take over ownership of the input
> frame. I first tried writing this code in a way where the calling code
> retains ownership, but this is nontrivial due to the presence of the
> no-op short-circuit condition in which the input frame is directly
> returned. (As an alternative, we could use av_frame_clone() instead, but
> I wanted to avoid touching the original behavior in this commit)
> ---
>  libavfilter/vf_scale.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)

patchset LGTM

thx

[...]
diff mbox series

Patch

diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
index 841075193e..a1a322ed9e 100644
--- a/libavfilter/vf_scale.c
+++ b/libavfilter/vf_scale.c
@@ -869,17 +869,20 @@  static int scale_field(ScaleContext *scale, AVFrame *dst, AVFrame *src,
     return 0;
 }
 
-static int scale_frame(AVFilterLink *link, AVFrame *in, AVFrame **frame_out)
+/* Takes over ownership of *frame_in, passes ownership of *frame_out to caller */
+static int scale_frame(AVFilterLink *link, AVFrame **frame_in,
+                       AVFrame **frame_out)
 {
     AVFilterContext *ctx = link->dst;
     ScaleContext *scale = ctx->priv;
     AVFilterLink *outlink = ctx->outputs[0];
-    AVFrame *out;
+    AVFrame *out, *in = *frame_in;
     const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(link->format);
     char buf[32];
     int ret;
     int frame_changed;
 
+    *frame_in = NULL;
     *frame_out = NULL;
     if (in->colorspace == AVCOL_SPC_YCGCO)
         av_log(link->dst, AV_LOG_WARNING, "Detected unsupported YCgCo colorspace.\n");
@@ -922,11 +925,11 @@  static int scale_frame(AVFilterLink *link, AVFrame *in, AVFrame **frame_out)
 
             ret = scale_parse_expr(ctx, NULL, &scale->w_pexpr, "width", scale->w_expr);
             if (ret < 0)
-                return ret;
+                goto err;
 
             ret = scale_parse_expr(ctx, NULL, &scale->h_pexpr, "height", scale->h_expr);
             if (ret < 0)
-                return ret;
+                goto err;
         }
 
         if (ctx->filter == &ff_vf_scale2ref) {
@@ -957,7 +960,7 @@  FF_ENABLE_DEPRECATION_WARNINGS
         link->dst->inputs[0]->sample_aspect_ratio.num = in->sample_aspect_ratio.num;
 
         if ((ret = config_props(outlink)) < 0)
-            return ret;
+            goto err;
     }
 
 scale:
@@ -971,10 +974,9 @@  scale:
 
     out = ff_get_video_buffer(outlink, outlink->w, outlink->h);
     if (!out) {
-        av_frame_free(&in);
-        return AVERROR(ENOMEM);
+        ret = AVERROR(ENOMEM);
+        goto err;
     }
-    *frame_out = out;
 
     av_frame_copy_props(out, in);
     out->width  = outlink->w;
@@ -999,9 +1001,12 @@  scale:
         ret = sws_scale_frame(scale->sws, out, in);
     }
 
-    av_frame_free(&in);
     if (ret < 0)
-        av_frame_free(frame_out);
+        av_frame_free(&out);
+    *frame_out = out;
+
+err:
+    av_frame_free(&in);
     return ret;
 }
 
@@ -1058,7 +1063,7 @@  FF_ENABLE_DEPRECATION_WARNINGS
         }
     }
 
-    ret = scale_frame(ctx->inputs[0], in, &out);
+    ret = scale_frame(ctx->inputs[0], &in, &out);
     if (out) {
         out->pts = av_rescale_q(fs->pts, fs->time_base, outlink->time_base);
         return ff_filter_frame(outlink, out);
@@ -1077,7 +1082,7 @@  static int filter_frame(AVFilterLink *link, AVFrame *in)
     AVFrame *out;
     int ret;
 
-    ret = scale_frame(link, in, &out);
+    ret = scale_frame(link, &in, &out);
     if (out)
         return ff_filter_frame(outlink, out);