diff mbox series

[FFmpeg-devel] avcodec/vp9: avoid using uninitialized mutex/condition

Message ID 20210902090228.13132-1-robux4@ycbcr.xyz
State New
Headers show
Series [FFmpeg-devel] avcodec/vp9: avoid using uninitialized mutex/condition
Related show

Checks

Context Check Description
andriy/commit_msg_x86 warning Please wrap lines in the body of the commit message between 60 and 72 characters.
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/commit_msg_ppc warning Please wrap lines in the body of the commit message between 60 and 72 characters.
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Steve Lhomme Sept. 2, 2021, 9:02 a.m. UTC
When using slice decoding vp9_free_entries is called before vp9_alloc_entries
is ever called. It should destroy properly initialized variables (or check it
was never called before).

It usually works undetected as pthread implementations allows NULL as a special
value (and should return EINVAL but doesn't). But pthreadGC2 doesn't allow NULL
in pthread_mutex_destroy() and crashes when that's the case.
---
 libavcodec/vp9.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Andreas Rheinhardt Sept. 2, 2021, 9:27 a.m. UTC | #1
Steve Lhomme:
> When using slice decoding vp9_free_entries is called before vp9_alloc_entries
> is ever called. It should destroy properly initialized variables (or check it
> was never called before).
> 
> It usually works undetected as pthread implementations allows NULL as a special
> value (and should return EINVAL but doesn't). But pthreadGC2 doesn't allow NULL
> in pthread_mutex_destroy() and crashes when that's the case.
> ---
>  libavcodec/vp9.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
> index 874005a5ae..d40e90b70e 100644
> --- a/libavcodec/vp9.c
> +++ b/libavcodec/vp9.c
> @@ -1796,6 +1796,10 @@ static av_cold int vp9_decode_init(AVCodecContext *avctx)
>  
>      s->last_bpp = 0;
>      s->s.h.filter.sharpness = -1;
> +    if (avctx->active_thread_type & FF_THREAD_SLICE)  {
> +        pthread_mutex_init(&s->progress_mutex, NULL);
> +        pthread_cond_init(&s->progress_cond, NULL);
> +    }
>  
>      for (int i = 0; i < 3; i++) {
>          s->s.frames[i].tf.f = av_frame_alloc();
> 
1. progress_mutex and progress_cond don't exist if compiled without
threading support, so your patch will lead to compilation failures in
this case.
2. I don't see a reason why these mutexes need to be initialized and
destroyed when the dimensions/number of tile cols change at all. They
should be initialized once during init (if slice threading is used) and
freed during close (if they have been successfully initialized).
3. Initializing this condition and mutex is currently not checked.

I can fix this if you want.

- Andreas
Steve Lhomme Sept. 2, 2021, 9:34 a.m. UTC | #2
On 2021-09-02 11:27, Andreas Rheinhardt wrote:
> Steve Lhomme:
>> When using slice decoding vp9_free_entries is called before vp9_alloc_entries
>> is ever called. It should destroy properly initialized variables (or check it
>> was never called before).
>>
>> It usually works undetected as pthread implementations allows NULL as a special
>> value (and should return EINVAL but doesn't). But pthreadGC2 doesn't allow NULL
>> in pthread_mutex_destroy() and crashes when that's the case.
>> ---
>>   libavcodec/vp9.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
>> index 874005a5ae..d40e90b70e 100644
>> --- a/libavcodec/vp9.c
>> +++ b/libavcodec/vp9.c
>> @@ -1796,6 +1796,10 @@ static av_cold int vp9_decode_init(AVCodecContext *avctx)
>>   
>>       s->last_bpp = 0;
>>       s->s.h.filter.sharpness = -1;
>> +    if (avctx->active_thread_type & FF_THREAD_SLICE)  {
>> +        pthread_mutex_init(&s->progress_mutex, NULL);
>> +        pthread_cond_init(&s->progress_cond, NULL);
>> +    }
>>   
>>       for (int i = 0; i < 3; i++) {
>>           s->s.frames[i].tf.f = av_frame_alloc();
>>
> 1. progress_mutex and progress_cond don't exist if compiled without
> threading support, so your patch will lead to compilation failures in
> this case.

Ah yes, I missed that the 2 functions are conditionally compiled.

> 2. I don't see a reason why these mutexes need to be initialized and
> destroyed when the dimensions/number of tile cols change at all. They
> should be initialized once during init (if slice threading is used) and
> freed during close (if they have been successfully initialized).

Yes, it makes total sense.

> 3. Initializing this condition and mutex is currently not checked.
> 
> I can fix this if you want.

Sure. This is currently leading to a crash in VLC 3.0 because we use a 
pthread implementation that doesn't like this case (the only lib 
compatible with XP).
https://code.videolan.org/videolan/vlc/-/issues/26017
diff mbox series

Patch

diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
index 874005a5ae..d40e90b70e 100644
--- a/libavcodec/vp9.c
+++ b/libavcodec/vp9.c
@@ -1796,6 +1796,10 @@  static av_cold int vp9_decode_init(AVCodecContext *avctx)
 
     s->last_bpp = 0;
     s->s.h.filter.sharpness = -1;
+    if (avctx->active_thread_type & FF_THREAD_SLICE)  {
+        pthread_mutex_init(&s->progress_mutex, NULL);
+        pthread_cond_init(&s->progress_cond, NULL);
+    }
 
     for (int i = 0; i < 3; i++) {
         s->s.frames[i].tf.f = av_frame_alloc();