diff mbox series

[FFmpeg-devel,01/13] avcodec/vp9: Do not destroy uninitialized mutexes/conditions

Message ID AM7PR03MB6660A9F027E4DD571CDF30C98FCE9@AM7PR03MB6660.eurprd03.prod.outlook.com
State Accepted
Commit bd95f2f599a6605dafa73b95ca7590aa6a5e941d
Headers show
Series [FFmpeg-devel,01/13] avcodec/vp9: Do not destroy uninitialized mutexes/conditions
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Andreas Rheinhardt Sept. 2, 2021, 3:34 p.m. UTC
Also do not destroy and reinitialize mutexes and conditions when
certain input parameters change. Given that the decoder did not
create these variables at all during init, uninitialized mutexes
and conditions are destroyed before the very first initialization.
This is undefined behaviour and certain threading implementations
like pthreadGC2 crash when it is attempted.

Fix this by initializing these objects once during init and freeing
them in close.

Reported-by: Steve Lhomme <robux4@ycbcr.xyz>
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/vp9.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

Comments

Steve Lhomme Sept. 3, 2021, 6:42 a.m. UTC | #1
On 2021-09-02 17:34, Andreas Rheinhardt wrote:
> Also do not destroy and reinitialize mutexes and conditions when
> certain input parameters change. Given that the decoder did not
> create these variables at all during init, uninitialized mutexes
> and conditions are destroyed before the very first initialization.
> This is undefined behaviour and certain threading implementations
> like pthreadGC2 crash when it is attempted.
> 
> Fix this by initializing these objects once during init and freeing
> them in close.

Works for me.

> Reported-by: Steve Lhomme <robux4@ycbcr.xyz>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>   libavcodec/vp9.c | 18 +++++++++++++-----
>   1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
> index 874005a5ae..5c20a7ec5d 100644
> --- a/libavcodec/vp9.c
> +++ b/libavcodec/vp9.c
> @@ -43,8 +43,6 @@ static void vp9_free_entries(AVCodecContext *avctx) {
>       VP9Context *s = avctx->priv_data;
>   
>       if (avctx->active_thread_type & FF_THREAD_SLICE)  {
> -        pthread_mutex_destroy(&s->progress_mutex);
> -        pthread_cond_destroy(&s->progress_cond);
>           av_freep(&s->entries);
>       }
>   }
> @@ -66,9 +64,6 @@ static int vp9_alloc_entries(AVCodecContext *avctx, int n) {
>   
>           for (i  = 0; i < n; i++)
>               atomic_init(&s->entries[i], 0);
> -
> -        pthread_mutex_init(&s->progress_mutex, NULL);
> -        pthread_cond_init(&s->progress_cond, NULL);
>       }
>       return 0;
>   }
> @@ -1252,6 +1247,12 @@ static av_cold int vp9_decode_free(AVCodecContext *avctx)
>   
>       free_buffers(s);
>       vp9_free_entries(avctx);
> +#if HAVE_THREADS
> +    if (avctx->active_thread_type & FF_THREAD_SLICE) {
> +        pthread_mutex_destroy(&s->progress_mutex);
> +        pthread_cond_destroy(&s->progress_cond);
> +    }
> +#endif
>       av_freep(&s->td);
>       return 0;
>   }
> @@ -1797,6 +1798,13 @@ static av_cold int vp9_decode_init(AVCodecContext *avctx)
>       s->last_bpp = 0;
>       s->s.h.filter.sharpness = -1;
>   
> +#if HAVE_THREADS
> +    if (avctx->active_thread_type & FF_THREAD_SLICE) {
> +        pthread_mutex_init(&s->progress_mutex, NULL);
> +        pthread_cond_init(&s->progress_cond, NULL);
> +    }
> +#endif
> +
>       for (int i = 0; i < 3; i++) {
>           s->s.frames[i].tf.f = av_frame_alloc();
>           if (!s->s.frames[i].tf.f)
> -- 
> 2.30.2
>
Andreas Rheinhardt Sept. 3, 2021, 2:11 p.m. UTC | #2
Steve Lhomme:
> On 2021-09-02 17:34, Andreas Rheinhardt wrote:
>> Also do not destroy and reinitialize mutexes and conditions when
>> certain input parameters change. Given that the decoder did not
>> create these variables at all during init, uninitialized mutexes
>> and conditions are destroyed before the very first initialization.
>> This is undefined behaviour and certain threading implementations
>> like pthreadGC2 crash when it is attempted.
>>
>> Fix this by initializing these objects once during init and freeing
>> them in close.
> 
> Works for me.
> 

Ok, then I will apply this patchset tomorrow unless there are objections.

- Andreas
diff mbox series

Patch

diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
index 874005a5ae..5c20a7ec5d 100644
--- a/libavcodec/vp9.c
+++ b/libavcodec/vp9.c
@@ -43,8 +43,6 @@  static void vp9_free_entries(AVCodecContext *avctx) {
     VP9Context *s = avctx->priv_data;
 
     if (avctx->active_thread_type & FF_THREAD_SLICE)  {
-        pthread_mutex_destroy(&s->progress_mutex);
-        pthread_cond_destroy(&s->progress_cond);
         av_freep(&s->entries);
     }
 }
@@ -66,9 +64,6 @@  static int vp9_alloc_entries(AVCodecContext *avctx, int n) {
 
         for (i  = 0; i < n; i++)
             atomic_init(&s->entries[i], 0);
-
-        pthread_mutex_init(&s->progress_mutex, NULL);
-        pthread_cond_init(&s->progress_cond, NULL);
     }
     return 0;
 }
@@ -1252,6 +1247,12 @@  static av_cold int vp9_decode_free(AVCodecContext *avctx)
 
     free_buffers(s);
     vp9_free_entries(avctx);
+#if HAVE_THREADS
+    if (avctx->active_thread_type & FF_THREAD_SLICE) {
+        pthread_mutex_destroy(&s->progress_mutex);
+        pthread_cond_destroy(&s->progress_cond);
+    }
+#endif
     av_freep(&s->td);
     return 0;
 }
@@ -1797,6 +1798,13 @@  static av_cold int vp9_decode_init(AVCodecContext *avctx)
     s->last_bpp = 0;
     s->s.h.filter.sharpness = -1;
 
+#if HAVE_THREADS
+    if (avctx->active_thread_type & FF_THREAD_SLICE) {
+        pthread_mutex_init(&s->progress_mutex, NULL);
+        pthread_cond_init(&s->progress_cond, NULL);
+    }
+#endif
+
     for (int i = 0; i < 3; i++) {
         s->s.frames[i].tf.f = av_frame_alloc();
         if (!s->s.frames[i].tf.f)