diff mbox series

[FFmpeg-devel,v2,1/2] avcodec/av1dec: Fix segfault upon allocation error

Message ID 20200917000816.497453-1-andreas.rheinhardt@gmail.com
State Accepted
Commit 92b578e1d62adf933cfacd36c02aabc448ed214d
Headers show
Series [FFmpeg-devel,v2,1/2] avcodec/av1dec: Fix segfault upon allocation error
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Andreas Rheinhardt Sept. 17, 2020, 12:08 a.m. UTC
Up until now, the AV1 decoder always checks before calling its wrapper
around ff_thread_release_buffer() whether the ThreadFrame was used at
all, i.e. it checked whether the first data buffer of the AVFrame
contained therein is NULL or not. Yet this presumes that the AVFrame has
been successfully allocated, even though this can of course fail; and if
it did, one would encounter a segfault.
Fix this by removing the checks altogether: ff_thread_release_buffer()
can handle both unallocated as well as empty frames (since commit
f6774f905fb3cfdc319523ac640be30b14c1bc55).

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
Removing the checks is based upon a suggestion by James Almer. I have
not removed the checks from the other callers of av1_frame_unref() as I
don't know how probable it is for the frame to be empty at this point.
I tested this as well as I can, but I have no hardware with AV1 hardware
acceleration.

 libavcodec/av1dec.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

James Almer Sept. 17, 2020, 12:32 a.m. UTC | #1
On 9/16/2020 9:08 PM, Andreas Rheinhardt wrote:
> Up until now, the AV1 decoder always checks before calling its wrapper
> around ff_thread_release_buffer() whether the ThreadFrame was used at
> all, i.e. it checked whether the first data buffer of the AVFrame
> contained therein is NULL or not. Yet this presumes that the AVFrame has
> been successfully allocated, even though this can of course fail; and if
> it did, one would encounter a segfault.
> Fix this by removing the checks altogether: ff_thread_release_buffer()
> can handle both unallocated as well as empty frames (since commit
> f6774f905fb3cfdc319523ac640be30b14c1bc55).
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> Removing the checks is based upon a suggestion by James Almer. I have
> not removed the checks from the other callers of av1_frame_unref() as I
> don't know how probable it is for the frame to be empty at this point.
> I tested this as well as I can, but I have no hardware with AV1 hardware
> acceleration.

You can remove the check that aborts when no hwaccel is present, which
lets get_format() successfully return a software pixel format. This will
make the decoder "work", simply outputting zeroed buffers into frames,
which is enough to test behavior like the above.

> 
>  libavcodec/av1dec.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c
> index bd8acdaafe..871db76b4d 100644
> --- a/libavcodec/av1dec.c
> +++ b/libavcodec/av1dec.c
> @@ -388,12 +388,10 @@ static av_cold int av1_decode_free(AVCodecContext *avctx)
>      AV1DecContext *s = avctx->priv_data;
>  
>      for (int i = 0; i < FF_ARRAY_ELEMS(s->ref); i++) {
> -        if (s->ref[i].tf.f->buf[0])
> -            av1_frame_unref(avctx, &s->ref[i]);
> +        av1_frame_unref(avctx, &s->ref[i]);
>          av_frame_free(&s->ref[i].tf.f);
>      }
> -    if (s->cur_frame.tf.f->buf[0])
> -        av1_frame_unref(avctx, &s->cur_frame);
> +    av1_frame_unref(avctx, &s->cur_frame);
>      av_frame_free(&s->cur_frame.tf.f);
>  
>      av_buffer_unref(&s->seq_ref);
> 

LGTM.
diff mbox series

Patch

diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c
index bd8acdaafe..871db76b4d 100644
--- a/libavcodec/av1dec.c
+++ b/libavcodec/av1dec.c
@@ -388,12 +388,10 @@  static av_cold int av1_decode_free(AVCodecContext *avctx)
     AV1DecContext *s = avctx->priv_data;
 
     for (int i = 0; i < FF_ARRAY_ELEMS(s->ref); i++) {
-        if (s->ref[i].tf.f->buf[0])
-            av1_frame_unref(avctx, &s->ref[i]);
+        av1_frame_unref(avctx, &s->ref[i]);
         av_frame_free(&s->ref[i].tf.f);
     }
-    if (s->cur_frame.tf.f->buf[0])
-        av1_frame_unref(avctx, &s->cur_frame);
+    av1_frame_unref(avctx, &s->cur_frame);
     av_frame_free(&s->cur_frame.tf.f);
 
     av_buffer_unref(&s->seq_ref);