diff mbox series

[FFmpeg-devel,12/13] avcodec/omx: Check initializing mutexes/conditions

Message ID AM7PR03MB66608C6FBA333655E55167098FCE9@AM7PR03MB6660.eurprd03.prod.outlook.com
State Accepted
Commit 5c0e2f839af1654c65170d336640d260bfffdefa
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 fail Make fate failed
andriy/make_ppc success Make finished
andriy/make_fate_ppc fail Make fate failed

Commit Message

Andreas Rheinhardt Sept. 2, 2021, 3:41 p.m. UTC
The earlier code did not properly check these initializations:
It only recorded whether the part of init where these initializations
are has been reached, but it did not check whether the initializations
were successful, although destroying them would be undefined behaviour
if they had not been initialized successfully.
Furthermore cleanup() always locked a mutex regardless of whether there
was any attempt to initialize these mutexes at all.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
This is mostly untested: I only tested whether it compiles.

 libavcodec/omx.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

Comments

Martin Storsjö Sept. 2, 2021, 8:42 p.m. UTC | #1
On Thu, 2 Sep 2021, Andreas Rheinhardt wrote:

> The earlier code did not properly check these initializations:
> It only recorded whether the part of init where these initializations
> are has been reached, but it did not check whether the initializations
> were successful, although destroying them would be undefined behaviour
> if they had not been initialized successfully.
> Furthermore cleanup() always locked a mutex regardless of whether there
> was any attempt to initialize these mutexes at all.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> This is mostly untested: I only tested whether it compiles.
>
> libavcodec/omx.c | 34 +++++++++++++++++-----------------
> 1 file changed, 17 insertions(+), 17 deletions(-)

Seems to work in very brief testing, so ok with me.

// Martin
Steve Lhomme Sept. 3, 2021, 6:55 a.m. UTC | #2
On 2021-09-02 17:41, Andreas Rheinhardt wrote:
> The earlier code did not properly check these initializations:
> It only recorded whether the part of init where these initializations
> are has been reached, but it did not check whether the initializations
> were successful, although destroying them would be undefined behaviour
> if they had not been initialized successfully.
> Furthermore cleanup() always locked a mutex regardless of whether there
> was any attempt to initialize these mutexes at all.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> This is mostly untested: I only tested whether it compiles.
> 
>   libavcodec/omx.c | 34 +++++++++++++++++-----------------
>   1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/libavcodec/omx.c b/libavcodec/omx.c
> index 9597c60057..7086ddd3a4 100644
> --- a/libavcodec/omx.c
> +++ b/libavcodec/omx.c
> @@ -43,6 +43,7 @@
>   #include "avcodec.h"
>   #include "h264.h"
>   #include "internal.h"
> +#include "pthread_internal.h"
>   
>   #ifdef OMX_SKIP64BIT
>   static OMX_TICKS to_omx_ticks(int64_t value)
> @@ -218,7 +219,7 @@ typedef struct OMXCodecContext {
>       OMX_STATETYPE state;
>       OMX_ERRORTYPE error;
>   
> -    int mutex_cond_inited;
> +    unsigned mutex_cond_inited_cnt;
>   
>       int eos_sent, got_eos;
>   
> @@ -229,6 +230,12 @@ typedef struct OMXCodecContext {
>       int profile;
>   } OMXCodecContext;
>   
> +#define NB_MUTEX_CONDS 6
> +#define OFF(field) offsetof(OMXCodecContext, field)
> +DEFINE_OFFSET_ARRAY(OMXCodecContext, omx_codec_context, mutex_cond_inited_cnt,
> +                    (OFF(input_mutex), OFF(output_mutex), OFF(state_mutex)),
> +                    (OFF(input_cond),  OFF(output_cond),  OFF(state_cond)));
> +
>   static void append_buffer(pthread_mutex_t *mutex, pthread_cond_t *cond,
>                             int* array_size, OMX_BUFFERHEADERTYPE **array,
>                             OMX_BUFFERHEADERTYPE *buffer)
> @@ -591,6 +598,9 @@ static av_cold void cleanup(OMXCodecContext *s)
>   {
>       int i, executing;
>   
> +    /* If the mutexes/condition variables have not been properly initialized,
> +     * nothing has been initialized and locking the mutex might be unsafe. */
> +    if (s->mutex_cond_inited_cnt == NB_MUTEX_CONDS) {
>       pthread_mutex_lock(&s->state_mutex);
>       executing = s->state == OMX_StateExecuting;
>       pthread_mutex_unlock(&s->state_mutex);
> @@ -620,20 +630,13 @@ static av_cold void cleanup(OMXCodecContext *s)
>   
>       omx_deinit(s->omx_context);
>       s->omx_context = NULL;
> -    if (s->mutex_cond_inited) {
> -        pthread_cond_destroy(&s->state_cond);
> -        pthread_mutex_destroy(&s->state_mutex);
> -        pthread_cond_destroy(&s->input_cond);
> -        pthread_mutex_destroy(&s->input_mutex);
> -        pthread_cond_destroy(&s->output_cond);
> -        pthread_mutex_destroy(&s->output_mutex);
> -        s->mutex_cond_inited = 0;
> -    }
>       av_freep(&s->in_buffer_headers);
>       av_freep(&s->out_buffer_headers);
>       av_freep(&s->free_in_buffers);
>       av_freep(&s->done_out_buffers);
>       av_freep(&s->output_buf);
> +    }
> +    ff_pthread_free(s, omx_codec_context_offsets);
>   }
>   
>   static av_cold int omx_encode_init(AVCodecContext *avctx)
> @@ -644,17 +647,14 @@ static av_cold int omx_encode_init(AVCodecContext *avctx)
>       OMX_BUFFERHEADERTYPE *buffer;
>       OMX_ERRORTYPE err;
>   
> +    /* cleanup relies on the mutexes/conditions being initialized first. */
> +    ret = ff_pthread_init(s, omx_codec_context_offsets);
> +    if (ret < 0)
> +        return ret;
>       s->omx_context = omx_init(avctx, s->libname, s->libprefix);
>       if (!s->omx_context)

Shouldn't you call ff_pthread_free() here ?

>           return AVERROR_ENCODER_NOT_FOUND;
>   
> -    pthread_mutex_init(&s->state_mutex, NULL);
> -    pthread_cond_init(&s->state_cond, NULL);
> -    pthread_mutex_init(&s->input_mutex, NULL);
> -    pthread_cond_init(&s->input_cond, NULL);
> -    pthread_mutex_init(&s->output_mutex, NULL);
> -    pthread_cond_init(&s->output_cond, NULL);
> -    s->mutex_cond_inited = 1;
>       s->avctx = avctx;
>       s->state = OMX_StateLoaded;
>       s->error = OMX_ErrorNone;
> -- 
> 2.30.2
> 
> _______________________________________________
> 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".
>
Andreas Rheinhardt Sept. 3, 2021, 7:03 a.m. UTC | #3
Steve Lhomme:
> On 2021-09-02 17:41, Andreas Rheinhardt wrote:
>> The earlier code did not properly check these initializations:
>> It only recorded whether the part of init where these initializations
>> are has been reached, but it did not check whether the initializations
>> were successful, although destroying them would be undefined behaviour
>> if they had not been initialized successfully.
>> Furthermore cleanup() always locked a mutex regardless of whether there
>> was any attempt to initialize these mutexes at all.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>> This is mostly untested: I only tested whether it compiles.
>>
>>   libavcodec/omx.c | 34 +++++++++++++++++-----------------
>>   1 file changed, 17 insertions(+), 17 deletions(-)
>>
>> diff --git a/libavcodec/omx.c b/libavcodec/omx.c
>> index 9597c60057..7086ddd3a4 100644
>> --- a/libavcodec/omx.c
>> +++ b/libavcodec/omx.c
>> @@ -43,6 +43,7 @@
>>   #include "avcodec.h"
>>   #include "h264.h"
>>   #include "internal.h"
>> +#include "pthread_internal.h"
>>     #ifdef OMX_SKIP64BIT
>>   static OMX_TICKS to_omx_ticks(int64_t value)
>> @@ -218,7 +219,7 @@ typedef struct OMXCodecContext {
>>       OMX_STATETYPE state;
>>       OMX_ERRORTYPE error;
>>   -    int mutex_cond_inited;
>> +    unsigned mutex_cond_inited_cnt;
>>         int eos_sent, got_eos;
>>   @@ -229,6 +230,12 @@ typedef struct OMXCodecContext {
>>       int profile;
>>   } OMXCodecContext;
>>   +#define NB_MUTEX_CONDS 6
>> +#define OFF(field) offsetof(OMXCodecContext, field)
>> +DEFINE_OFFSET_ARRAY(OMXCodecContext, omx_codec_context,
>> mutex_cond_inited_cnt,
>> +                    (OFF(input_mutex), OFF(output_mutex),
>> OFF(state_mutex)),
>> +                    (OFF(input_cond),  OFF(output_cond), 
>> OFF(state_cond)));
>> +
>>   static void append_buffer(pthread_mutex_t *mutex, pthread_cond_t *cond,
>>                             int* array_size, OMX_BUFFERHEADERTYPE
>> **array,
>>                             OMX_BUFFERHEADERTYPE *buffer)
>> @@ -591,6 +598,9 @@ static av_cold void cleanup(OMXCodecContext *s)
>>   {
>>       int i, executing;
>>   +    /* If the mutexes/condition variables have not been properly
>> initialized,
>> +     * nothing has been initialized and locking the mutex might be
>> unsafe. */
>> +    if (s->mutex_cond_inited_cnt == NB_MUTEX_CONDS) {
>>       pthread_mutex_lock(&s->state_mutex);
>>       executing = s->state == OMX_StateExecuting;
>>       pthread_mutex_unlock(&s->state_mutex);
>> @@ -620,20 +630,13 @@ static av_cold void cleanup(OMXCodecContext *s)
>>         omx_deinit(s->omx_context);
>>       s->omx_context = NULL;
>> -    if (s->mutex_cond_inited) {
>> -        pthread_cond_destroy(&s->state_cond);
>> -        pthread_mutex_destroy(&s->state_mutex);
>> -        pthread_cond_destroy(&s->input_cond);
>> -        pthread_mutex_destroy(&s->input_mutex);
>> -        pthread_cond_destroy(&s->output_cond);
>> -        pthread_mutex_destroy(&s->output_mutex);
>> -        s->mutex_cond_inited = 0;
>> -    }
>>       av_freep(&s->in_buffer_headers);
>>       av_freep(&s->out_buffer_headers);
>>       av_freep(&s->free_in_buffers);
>>       av_freep(&s->done_out_buffers);
>>       av_freep(&s->output_buf);
>> +    }
>> +    ff_pthread_free(s, omx_codec_context_offsets);
>>   }
>>     static av_cold int omx_encode_init(AVCodecContext *avctx)
>> @@ -644,17 +647,14 @@ static av_cold int
>> omx_encode_init(AVCodecContext *avctx)
>>       OMX_BUFFERHEADERTYPE *buffer;
>>       OMX_ERRORTYPE err;
>>   +    /* cleanup relies on the mutexes/conditions being initialized
>> first. */
>> +    ret = ff_pthread_init(s, omx_codec_context_offsets);
>> +    if (ret < 0)
>> +        return ret;
>>       s->omx_context = omx_init(avctx, s->libname, s->libprefix);
>>       if (!s->omx_context)
> 
> Shouldn't you call ff_pthread_free() here ?
> 
These encoders have the FF_CODEC_CAP_INIT_CLEANUP set which means that
omx_encode_end() will be called automatically if omx_encode_init()
fails. (This is why the current code always locked the state_mutex
regardless of whether its initialization has ever been reached.)

- Andreas
diff mbox series

Patch

diff --git a/libavcodec/omx.c b/libavcodec/omx.c
index 9597c60057..7086ddd3a4 100644
--- a/libavcodec/omx.c
+++ b/libavcodec/omx.c
@@ -43,6 +43,7 @@ 
 #include "avcodec.h"
 #include "h264.h"
 #include "internal.h"
+#include "pthread_internal.h"
 
 #ifdef OMX_SKIP64BIT
 static OMX_TICKS to_omx_ticks(int64_t value)
@@ -218,7 +219,7 @@  typedef struct OMXCodecContext {
     OMX_STATETYPE state;
     OMX_ERRORTYPE error;
 
-    int mutex_cond_inited;
+    unsigned mutex_cond_inited_cnt;
 
     int eos_sent, got_eos;
 
@@ -229,6 +230,12 @@  typedef struct OMXCodecContext {
     int profile;
 } OMXCodecContext;
 
+#define NB_MUTEX_CONDS 6
+#define OFF(field) offsetof(OMXCodecContext, field)
+DEFINE_OFFSET_ARRAY(OMXCodecContext, omx_codec_context, mutex_cond_inited_cnt,
+                    (OFF(input_mutex), OFF(output_mutex), OFF(state_mutex)),
+                    (OFF(input_cond),  OFF(output_cond),  OFF(state_cond)));
+
 static void append_buffer(pthread_mutex_t *mutex, pthread_cond_t *cond,
                           int* array_size, OMX_BUFFERHEADERTYPE **array,
                           OMX_BUFFERHEADERTYPE *buffer)
@@ -591,6 +598,9 @@  static av_cold void cleanup(OMXCodecContext *s)
 {
     int i, executing;
 
+    /* If the mutexes/condition variables have not been properly initialized,
+     * nothing has been initialized and locking the mutex might be unsafe. */
+    if (s->mutex_cond_inited_cnt == NB_MUTEX_CONDS) {
     pthread_mutex_lock(&s->state_mutex);
     executing = s->state == OMX_StateExecuting;
     pthread_mutex_unlock(&s->state_mutex);
@@ -620,20 +630,13 @@  static av_cold void cleanup(OMXCodecContext *s)
 
     omx_deinit(s->omx_context);
     s->omx_context = NULL;
-    if (s->mutex_cond_inited) {
-        pthread_cond_destroy(&s->state_cond);
-        pthread_mutex_destroy(&s->state_mutex);
-        pthread_cond_destroy(&s->input_cond);
-        pthread_mutex_destroy(&s->input_mutex);
-        pthread_cond_destroy(&s->output_cond);
-        pthread_mutex_destroy(&s->output_mutex);
-        s->mutex_cond_inited = 0;
-    }
     av_freep(&s->in_buffer_headers);
     av_freep(&s->out_buffer_headers);
     av_freep(&s->free_in_buffers);
     av_freep(&s->done_out_buffers);
     av_freep(&s->output_buf);
+    }
+    ff_pthread_free(s, omx_codec_context_offsets);
 }
 
 static av_cold int omx_encode_init(AVCodecContext *avctx)
@@ -644,17 +647,14 @@  static av_cold int omx_encode_init(AVCodecContext *avctx)
     OMX_BUFFERHEADERTYPE *buffer;
     OMX_ERRORTYPE err;
 
+    /* cleanup relies on the mutexes/conditions being initialized first. */
+    ret = ff_pthread_init(s, omx_codec_context_offsets);
+    if (ret < 0)
+        return ret;
     s->omx_context = omx_init(avctx, s->libname, s->libprefix);
     if (!s->omx_context)
         return AVERROR_ENCODER_NOT_FOUND;
 
-    pthread_mutex_init(&s->state_mutex, NULL);
-    pthread_cond_init(&s->state_cond, NULL);
-    pthread_mutex_init(&s->input_mutex, NULL);
-    pthread_cond_init(&s->input_cond, NULL);
-    pthread_mutex_init(&s->output_mutex, NULL);
-    pthread_cond_init(&s->output_cond, NULL);
-    s->mutex_cond_inited = 1;
     s->avctx = avctx;
     s->state = OMX_StateLoaded;
     s->error = OMX_ErrorNone;