[FFmpeg-devel] [PATCH] avcodec/vp8: Maintain consistency of frame pointers

[FFmpeg-devel] [PATCH] avcodec/vp8: Maintain consistency of frame pointers

[Andreas Rheinhardt]

Right now it is possible for the pointer for the current frame to
be set in the context even when it could not be properly set up;
this does not influence various the ordinary ref frames, but only
VP8Context.prev_frame. And since this code has been ported to the
ProgressFrame API in d48d7bc434f30dfbdf346f16715e4f2044b3e000,
this leads to segfaults, because the ProgressFrame API is less
forgiving than the ThreadFrame API (waiting on an uninitialized
ProgressFrame segfaults, waiting on an uninitialized ThreadFrame
is a no-op (the code behaves as if frame-threading is not in use)).

Fix this by maintaining the consistency of the frame pointers
in the context (by setting them later).

Fixes: NULL pointer dereference
Fixes: 68192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VP8_fuzzer-6180311026171904

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/vp8.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/libavcodec/vp8.c b/libavcodec/vp8.c
index d6df018655..8945447eb6 100644
--- a/libavcodec/vp8.c
+++ b/libavcodec/vp8.c
@@ -541,9 +541,12 @@ static int vp7_fade_frame(VP8Context *s, int alpha, int beta)
 
         /* preserve the golden frame, write a new previous frame */
         if (s->framep[VP8_FRAME_GOLDEN] == s->framep[VP8_FRAME_PREVIOUS]) {
-            s->framep[VP8_FRAME_PREVIOUS] = vp8_find_free_buffer(s);
-            if ((ret = vp8_alloc_frame(s, s->framep[VP8_FRAME_PREVIOUS], 1)) < 0)
+            VP8Frame *prev_frame = vp8_find_free_buffer(s);
+
+            ret = vp8_alloc_frame(s, prev_frame, 1);
+            if (ret < 0)
                 return ret;
+            s->framep[VP8_FRAME_PREVIOUS] = prev_frame;
 
             dst = s->framep[VP8_FRAME_PREVIOUS]->tf.f;
 
@@ -2699,7 +2702,7 @@ int vp78_decode_frame(AVCodecContext *avctx, AVFrame *rframe, int *got_frame,
             &s->frames[i] != s->framep[VP8_FRAME_ALTREF])
             vp8_release_frame(&s->frames[i]);
 
-    curframe = s->framep[VP8_FRAME_CURRENT] = vp8_find_free_buffer(s);
+    curframe = vp8_find_free_buffer(s);
 
     if (!s->colorspace)
         avctx->colorspace = AVCOL_SPC_BT470BG;
@@ -2723,6 +2726,7 @@ int vp78_decode_frame(AVCodecContext *avctx, AVFrame *rframe, int *got_frame,
 
     if ((ret = vp8_alloc_frame(s, curframe, referenced)) < 0)
         goto err;
+    s->framep[VP8_FRAME_CURRENT] = curframe;
     if (s->keyframe)
         curframe->tf.f->flags |= AV_FRAME_FLAG_KEY;
     else
-- 
2.40.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".

Re: [FFmpeg-devel] [PATCH] avcodec/vp8: Maintain consistency of frame pointers

[Andreas Rheinhardt]

Andreas Rheinhardt:
> Right now it is possible for the pointer for the current frame to
> be set in the context even when it could not be properly set up;
> this does not influence various the ordinary ref frames, but only
> VP8Context.prev_frame. And since this code has been ported to the
> ProgressFrame API in d48d7bc434f30dfbdf346f16715e4f2044b3e000,
> this leads to segfaults, because the ProgressFrame API is less
> forgiving than the ThreadFrame API (waiting on an uninitialized
> ProgressFrame segfaults, waiting on an uninitialized ThreadFrame
> is a no-op (the code behaves as if frame-threading is not in use)).
> 
> Fix this by maintaining the consistency of the frame pointers
> in the context (by setting them later).
> 
> Fixes: NULL pointer dereference
> Fixes: 68192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VP8_fuzzer-6180311026171904
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavcodec/vp8.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/vp8.c b/libavcodec/vp8.c
> index d6df018655..8945447eb6 100644
> --- a/libavcodec/vp8.c
> +++ b/libavcodec/vp8.c
> @@ -541,9 +541,12 @@ static int vp7_fade_frame(VP8Context *s, int alpha, int beta)
>  
>          /* preserve the golden frame, write a new previous frame */
>          if (s->framep[VP8_FRAME_GOLDEN] == s->framep[VP8_FRAME_PREVIOUS]) {
> -            s->framep[VP8_FRAME_PREVIOUS] = vp8_find_free_buffer(s);
> -            if ((ret = vp8_alloc_frame(s, s->framep[VP8_FRAME_PREVIOUS], 1)) < 0)
> +            VP8Frame *prev_frame = vp8_find_free_buffer(s);
> +
> +            ret = vp8_alloc_frame(s, prev_frame, 1);
> +            if (ret < 0)
>                  return ret;
> +            s->framep[VP8_FRAME_PREVIOUS] = prev_frame;
>  
>              dst = s->framep[VP8_FRAME_PREVIOUS]->tf.f;
>  
> @@ -2699,7 +2702,7 @@ int vp78_decode_frame(AVCodecContext *avctx, AVFrame *rframe, int *got_frame,
>              &s->frames[i] != s->framep[VP8_FRAME_ALTREF])
>              vp8_release_frame(&s->frames[i]);
>  
> -    curframe = s->framep[VP8_FRAME_CURRENT] = vp8_find_free_buffer(s);
> +    curframe = vp8_find_free_buffer(s);
>  
>      if (!s->colorspace)
>          avctx->colorspace = AVCOL_SPC_BT470BG;
> @@ -2723,6 +2726,7 @@ int vp78_decode_frame(AVCodecContext *avctx, AVFrame *rframe, int *got_frame,
>  
>      if ((ret = vp8_alloc_frame(s, curframe, referenced)) < 0)
>          goto err;
> +    s->framep[VP8_FRAME_CURRENT] = curframe;
>      if (s->keyframe)
>          curframe->tf.f->flags |= AV_FRAME_FLAG_KEY;
>      else

Will apply. Sorry for the delay.

- Andreas

_______________________________________________
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".