diff mbox series

[FFmpeg-devel,3/4] avfilter/vf_paletteuse: Fix potential double-free of AVFrame

Message ID 20200127082821.22770-3-andreas.rheinhardt@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel,1/4] avformat/mov: Free encryption data on error | expand

Checks

Context Check Description
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Andreas Rheinhardt Jan. 27, 2020, 8:28 a.m. UTC 
apply_palette() would free an AVFrame given to it only via an AVFrame *
(and not via AVFrame **) in three of its four exists (namely in the
normal path and in two error paths). So upon error the caller has no way
to know whether the frame has already been freed or not;
load_apply_palette(), the only caller, opted to free the frame in this
scenario.

This commit changes this by making apply_palette not freeing the frame
at all, which is left to load_apply_palette().

Fixes Coverity issue #1452434.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavfilter/vf_paletteuse.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

Comments

Andreas Rheinhardt Feb. 7, 2020, 7:05 p.m. UTC | #1 
Andreas Rheinhardt:
> apply_palette() would free an AVFrame given to it only via an AVFrame *
> (and not via AVFrame **) in three of its four exists (namely in the
> normal path and in two error paths). So upon error the caller has no way
> to know whether the frame has already been freed or not;
> load_apply_palette(), the only caller, opted to free the frame in this
> scenario.
> 
> This commit changes this by making apply_palette not freeing the frame
> at all, which is left to load_apply_palette().
> 
> Fixes Coverity issue #1452434.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavfilter/vf_paletteuse.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/libavfilter/vf_paletteuse.c b/libavfilter/vf_paletteuse.c
> index ed128813d6..255c9d79e3 100644
> --- a/libavfilter/vf_paletteuse.c
> +++ b/libavfilter/vf_paletteuse.c
> @@ -903,7 +903,6 @@ static int apply_palette(AVFilterLink *inlink, AVFrame *in, AVFrame **outf)
>  
>      AVFrame *out = ff_get_video_buffer(outlink, outlink->w, outlink->h);
>      if (!out) {
> -        av_frame_free(&in);
>          *outf = NULL;
>          return AVERROR(ENOMEM);
>      }
> @@ -916,7 +915,6 @@ static int apply_palette(AVFilterLink *inlink, AVFrame *in, AVFrame **outf)
>      if (av_frame_ref(s->last_in, in) < 0 ||
>          av_frame_ref(s->last_out, out) < 0 ||
>          av_frame_make_writable(s->last_in) < 0) {
> -        av_frame_free(&in);
>          av_frame_free(&out);
>          *outf = NULL;
>          return AVERROR(ENOMEM);
> @@ -934,7 +932,6 @@ static int apply_palette(AVFilterLink *inlink, AVFrame *in, AVFrame **outf)
>      memcpy(out->data[1], s->palette, AVPALETTE_SIZE);
>      if (s->calc_mean_err)
>          debug_mean_error(s, in, out, inlink->frame_count_out);
> -    av_frame_free(&in);
>      *outf = out;
>      return 0;
>  }
> @@ -1023,20 +1020,17 @@ static int load_apply_palette(FFFrameSync *fs)
>      if (ret < 0)
>          return ret;
>      if (!master || !second) {
> -        ret = AVERROR_BUG;
> -        goto error;
> +        av_frame_free(&master);
> +        return AVERROR_BUG;
>      }
>      if (!s->palette_loaded) {
>          load_palette(s, second);
>      }
>      ret = apply_palette(inlink, master, &out);
> +    av_frame_free(&master);
>      if (ret < 0)
> -        goto error;
> +        return ret;
>      return ff_filter_frame(ctx->outputs[0], out);
> -
> -error:
> -    av_frame_free(&master);
> -    return ret;
>  }
>  
>  #define DEFINE_SET_FRAME(color_search, name, value)                             \
> 
Ping.

- Andreas
Paul B Mahol Feb. 7, 2020, 7:35 p.m. UTC | #2 
LGTM

On 1/27/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
> apply_palette() would free an AVFrame given to it only via an AVFrame *
> (and not via AVFrame **) in three of its four exists (namely in the
> normal path and in two error paths). So upon error the caller has no way
> to know whether the frame has already been freed or not;
> load_apply_palette(), the only caller, opted to free the frame in this
> scenario.
>
> This commit changes this by making apply_palette not freeing the frame
> at all, which is left to load_apply_palette().
>
> Fixes Coverity issue #1452434.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavfilter/vf_paletteuse.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/libavfilter/vf_paletteuse.c b/libavfilter/vf_paletteuse.c
> index ed128813d6..255c9d79e3 100644
> --- a/libavfilter/vf_paletteuse.c
> +++ b/libavfilter/vf_paletteuse.c
> @@ -903,7 +903,6 @@ static int apply_palette(AVFilterLink *inlink, AVFrame
> *in, AVFrame **outf)
>
>      AVFrame *out = ff_get_video_buffer(outlink, outlink->w, outlink->h);
>      if (!out) {
> -        av_frame_free(&in);
>          *outf = NULL;
>          return AVERROR(ENOMEM);
>      }
> @@ -916,7 +915,6 @@ static int apply_palette(AVFilterLink *inlink, AVFrame
> *in, AVFrame **outf)
>      if (av_frame_ref(s->last_in, in) < 0 ||
>          av_frame_ref(s->last_out, out) < 0 ||
>          av_frame_make_writable(s->last_in) < 0) {
> -        av_frame_free(&in);
>          av_frame_free(&out);
>          *outf = NULL;
>          return AVERROR(ENOMEM);
> @@ -934,7 +932,6 @@ static int apply_palette(AVFilterLink *inlink, AVFrame
> *in, AVFrame **outf)
>      memcpy(out->data[1], s->palette, AVPALETTE_SIZE);
>      if (s->calc_mean_err)
>          debug_mean_error(s, in, out, inlink->frame_count_out);
> -    av_frame_free(&in);
>      *outf = out;
>      return 0;
>  }
> @@ -1023,20 +1020,17 @@ static int load_apply_palette(FFFrameSync *fs)
>      if (ret < 0)
>          return ret;
>      if (!master || !second) {
> -        ret = AVERROR_BUG;
> -        goto error;
> +        av_frame_free(&master);
> +        return AVERROR_BUG;
>      }
>      if (!s->palette_loaded) {
>          load_palette(s, second);
>      }
>      ret = apply_palette(inlink, master, &out);
> +    av_frame_free(&master);
>      if (ret < 0)
> -        goto error;
> +        return ret;
>      return ff_filter_frame(ctx->outputs[0], out);
> -
> -error:
> -    av_frame_free(&master);
> -    return ret;
>  }
>
>  #define DEFINE_SET_FRAME(color_search, name, value)
>     \
> --
> 2.20.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Michael Niedermayer Feb. 8, 2020, 4:55 p.m. UTC | #3 
On Fri, Feb 07, 2020 at 08:35:23PM +0100, Paul B Mahol wrote:
> LGTM

will apply

thx

[...]
diff mbox series

Patch

diff --git a/libavfilter/vf_paletteuse.c b/libavfilter/vf_paletteuse.c
index ed128813d6..255c9d79e3 100644
--- a/libavfilter/vf_paletteuse.c
+++ b/libavfilter/vf_paletteuse.c
@@ -903,7 +903,6 @@  static int apply_palette(AVFilterLink *inlink, AVFrame *in, AVFrame **outf)
 
     AVFrame *out = ff_get_video_buffer(outlink, outlink->w, outlink->h);
     if (!out) {
-        av_frame_free(&in);
         *outf = NULL;
         return AVERROR(ENOMEM);
     }
@@ -916,7 +915,6 @@  static int apply_palette(AVFilterLink *inlink, AVFrame *in, AVFrame **outf)
     if (av_frame_ref(s->last_in, in) < 0 ||
         av_frame_ref(s->last_out, out) < 0 ||
         av_frame_make_writable(s->last_in) < 0) {
-        av_frame_free(&in);
         av_frame_free(&out);
         *outf = NULL;
         return AVERROR(ENOMEM);
@@ -934,7 +932,6 @@  static int apply_palette(AVFilterLink *inlink, AVFrame *in, AVFrame **outf)
     memcpy(out->data[1], s->palette, AVPALETTE_SIZE);
     if (s->calc_mean_err)
         debug_mean_error(s, in, out, inlink->frame_count_out);
-    av_frame_free(&in);
     *outf = out;
     return 0;
 }
@@ -1023,20 +1020,17 @@  static int load_apply_palette(FFFrameSync *fs)
     if (ret < 0)
         return ret;
     if (!master || !second) {
-        ret = AVERROR_BUG;
-        goto error;
+        av_frame_free(&master);
+        return AVERROR_BUG;
     }
     if (!s->palette_loaded) {
         load_palette(s, second);
     }
     ret = apply_palette(inlink, master, &out);
+    av_frame_free(&master);
     if (ret < 0)
-        goto error;
+        return ret;
     return ff_filter_frame(ctx->outputs[0], out);
-
-error:
-    av_frame_free(&master);
-    return ret;
 }
 
 #define DEFINE_SET_FRAME(color_search, name, value)                             \