diff mbox series

[FFmpeg-devel,5/6] avcodec/rv34, mpegvideo: Fix segfault upon frame size change error

Message ID HE1PR0301MB215456661E199D41F327C3AC8F779@HE1PR0301MB2154.eurprd03.prod.outlook.com
State Accepted
Commit 9abda1365c5e2d827eb673b6d98245163c868bf1
Headers show
Series [FFmpeg-devel,1/6] Revert "avcodec: add FF_CODEC_CAP_INIT_CLEANUP for all codecs which use ff_mpv_common_init()"
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt April 5, 2021, 1:44 a.m. UTC
The RealVideo 3.0 and 4.0 decoders call ff_mpv_common_init() only during
their init function and not during decode_frame(); when the size of the
frame changes, they call ff_mpv_common_frame_size_change(). Yet upon
error, said function calls ff_mpv_common_end() which frees the whole
MpegEncContext and not only those parts that
ff_mpv_common_frame_size_change() reinits. As a result, the context will
never be usable again; worse, because decode_frame() contains no check
for whether the context is initialized or not, it is presumed that it is
initialized, leading to segfaults. Basically the same happens if
rv34_decoder_realloc() fails.

This commit fixes this by only resetting the parts that
ff_mpv_common_frame_size_change() changes upon error and by actually
checking whether the context is in need of reinitialization in
ff_rv34_decode_frame().

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
I actually don't like that we have two flags that indicate whether
a MpegEncContext is usable or not; how about we always call
ff_mpv_common_init() during init (and never lateron) and make it
unconditionally allocate the stuff that does not depend upon resolution
etc. and add a parameter to said function to also allocate the latter.
The decode_frame functions would then be modified to always use
ff_mpv_common_frame_size_change().

ff_rv34_decode_update_thread_context() currently checks twice whether
the source MpegEncContext is initialized; the second is trivially true,
because of the first check. But the first is also always true now,
because ff_mpv_common_end() is only called when closing the decoder
(and said check also made no sense before this patch because a
noninitialized MpegEncContext would have led to a segfault pretty soon).
Should this be changed to check for context_reinit instead?

 libavcodec/mpegvideo.c |  6 ++++--
 libavcodec/rv34.c      | 13 +++++++++----
 2 files changed, 13 insertions(+), 6 deletions(-)

Comments

Michael Niedermayer April 7, 2021, 7:48 p.m. UTC | #1
On Mon, Apr 05, 2021 at 03:44:33AM +0200, Andreas Rheinhardt wrote:
> The RealVideo 3.0 and 4.0 decoders call ff_mpv_common_init() only during
> their init function and not during decode_frame(); when the size of the
> frame changes, they call ff_mpv_common_frame_size_change(). Yet upon
> error, said function calls ff_mpv_common_end() which frees the whole
> MpegEncContext and not only those parts that
> ff_mpv_common_frame_size_change() reinits. As a result, the context will
> never be usable again; worse, because decode_frame() contains no check
> for whether the context is initialized or not, it is presumed that it is
> initialized, leading to segfaults. Basically the same happens if
> rv34_decoder_realloc() fails.
> 
> This commit fixes this by only resetting the parts that
> ff_mpv_common_frame_size_change() changes upon error and by actually
> checking whether the context is in need of reinitialization in
> ff_rv34_decode_frame().
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>

> ---
> I actually don't like that we have two flags that indicate whether
> a MpegEncContext is usable or not; how about we always call
> ff_mpv_common_init() during init (and never lateron) and make it
> unconditionally allocate the stuff that does not depend upon resolution
> etc. and add a parameter to said function to also allocate the latter.
> The decode_frame functions would then be modified to always use
> ff_mpv_common_frame_size_change().

sure if that ends up being cleaner


[...]
diff mbox series

Patch

diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c
index 7eddbdcc37..5de0719f83 100644
--- a/libavcodec/mpegvideo.c
+++ b/libavcodec/mpegvideo.c
@@ -555,7 +555,6 @@  int ff_mpeg_update_thread_context(AVCodecContext *dst,
     }
 
     if (s->height != s1->height || s->width != s1->width || s->context_reinit) {
-        s->context_reinit = 0;
         s->height = s1->height;
         s->width  = s1->width;
         if ((ret = ff_mpv_common_frame_size_change(s)) < 0)
@@ -1099,10 +1098,12 @@  int ff_mpv_common_frame_size_change(MpegEncContext *s)
         if (err < 0)
             goto fail;
     }
+    s->context_reinit = 0;
 
     return 0;
  fail:
-    ff_mpv_common_end(s);
+    free_context_frame(s);
+    s->context_reinit = 1;
     return err;
 }
 
@@ -1149,6 +1150,7 @@  void ff_mpv_common_end(MpegEncContext *s)
     av_frame_free(&s->new_picture.f);
 
     s->context_initialized      = 0;
+    s->context_reinit           = 0;
     s->last_picture_ptr         =
     s->next_picture_ptr         =
     s->current_picture_ptr      = NULL;
diff --git a/libavcodec/rv34.c b/libavcodec/rv34.c
index 7e5bfe0e22..99e580a09a 100644
--- a/libavcodec/rv34.c
+++ b/libavcodec/rv34.c
@@ -1383,6 +1383,7 @@  static int rv34_decoder_alloc(RV34DecContext *r)
 
     if (!(r->cbp_chroma       && r->cbp_luma && r->deblock_coefs &&
           r->intra_types_hist && r->mb_type)) {
+        r->s.context_reinit = 1;
         rv34_decoder_free(r);
         return AVERROR(ENOMEM);
     }
@@ -1530,7 +1531,7 @@  int ff_rv34_decode_update_thread_context(AVCodecContext *dst, const AVCodecConte
     if (dst == src || !s1->context_initialized)
         return 0;
 
-    if (s->height != s1->height || s->width != s1->width) {
+    if (s->height != s1->height || s->width != s1->width || s->context_reinit) {
         s->height = s1->height;
         s->width  = s1->width;
         if ((err = ff_mpv_common_frame_size_change(s)) < 0)
@@ -1667,11 +1668,12 @@  int ff_rv34_decode_frame(AVCodecContext *avctx,
         if (s->mb_num_left > 0 && s->current_picture_ptr) {
             av_log(avctx, AV_LOG_ERROR, "New frame but still %d MB left.\n",
                    s->mb_num_left);
-            ff_er_frame_end(&s->er);
+            if (!s->context_reinit)
+                ff_er_frame_end(&s->er);
             ff_mpv_frame_end(s);
         }
 
-        if (s->width != si.width || s->height != si.height) {
+        if (s->width != si.width || s->height != si.height || s->context_reinit) {
             int err;
 
             av_log(s->avctx, AV_LOG_WARNING, "Changing dimensions to %dx%d\n",
@@ -1689,7 +1691,6 @@  int ff_rv34_decode_frame(AVCodecContext *avctx,
             err = ff_set_dimensions(s->avctx, s->width, s->height);
             if (err < 0)
                 return err;
-
             if ((err = ff_mpv_common_frame_size_change(s)) < 0)
                 return err;
             if ((err = rv34_decoder_realloc(r)) < 0)
@@ -1744,6 +1745,10 @@  int ff_rv34_decode_frame(AVCodecContext *avctx,
         }
         s->mb_x = s->mb_y = 0;
         ff_thread_finish_setup(s->avctx);
+    } else if (s->context_reinit) {
+        av_log(s->avctx, AV_LOG_ERROR, "Decoder needs full frames to "
+               "reinitialize (start MB is %d).\n", si.start);
+        return AVERROR_INVALIDDATA;
     } else if (HAVE_THREADS &&
                (s->avctx->active_thread_type & FF_THREAD_FRAME)) {
         av_log(s->avctx, AV_LOG_ERROR, "Decoder needs full frames in frame "