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

[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 at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>