Andreas Rheinhardt: > Up until now, ff_frame_thread_init had several bugs: > 1. It did not check whether the condition and mutexes > could be successfully created. > 2. In case an error happened when setting up the child threads, > ff_frame_thread_free is called to clean up all threads set up so far, > including the current one. But a half-allocated context needs > special handling which ff_frame_thread_frame_free doesn't provide. > Notably, if allocating the AVCodecInternal, the codec's private data > or setting the options fails, the codec's close function will be > called (if there is one); it will also be called if the codec's init > function fails, regardless of whether the FF_CODEC_CAP_INIT_CLEANUP > is set. This is not supported by all codecs; in ticket #9099 (which > this commit fixes) it led to a crash. > > This commit fixes both of these issues. Given that it is not documented > to be save to destroy mutexes/conditions that have only been zeroed, but > not initialized (or whose initialization has failed), one either needs to > duplicate the code to destroy them in ff_frame_thread_init or record > which mutexes/condition variables need to be destroyed and check for this > in ff_frame_thread_free. This patch uses the former way for the > mutexes/condition variables, but lets ff_frame_thread_free take > over for all threads whose AVCodecContext could be allocated. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinha...@gmail.com> > --- > After several unsuccessful attempts to make pthread_cond/mutex_init > fail, I looked at the source [1] and it seems that the glibc versions of > these functions can't fail at all (unless one sets nondefault attributes). > Therefore this part of the code is untested, unfortunately. > > (Removing all pthread_mutex/cond_destroy calls does not result in any > complaints from Valgrind/ASAN either; seems the glibc implementation > doesn't need allocations.) > > [1]: https://github.com/bminor/glibc/blob/master/nptl/pthread_cond_init.c > https://github.com/bminor/glibc/blob/master/nptl/pthread_mutex_init.c > > libavcodec/pthread_frame.c | 175 ++++++++++++++++++++++--------------- > 1 file changed, 106 insertions(+), 69 deletions(-) > > diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c > index 4429a4d59c..a10d8c1266 100644 > --- a/libavcodec/pthread_frame.c > +++ b/libavcodec/pthread_frame.c > @@ -64,6 +64,12 @@ enum { > STATE_SETUP_FINISHED, > }; > > +enum { > + UNINITIALIZED, ///< Thread has not been created, AVCodec->close mustn't > be called > + NEEDS_CLOSE, ///< AVCodec->close needs to be called > + INITIALIZED, ///< Thread has been properly set up > +}; > + > /** > * Context used by codec threads and stored in their AVCodecInternal > thread_ctx. > */ > @@ -698,27 +704,40 @@ void ff_frame_thread_free(AVCodecContext *avctx, int > thread_count) > > for (i = 0; i < thread_count; i++) { > PerThreadContext *p = &fctx->threads[i]; > + AVCodecContext *ctx = p->avctx; > > - pthread_mutex_lock(&p->mutex); > - p->die = 1; > - pthread_cond_signal(&p->input_cond); > - pthread_mutex_unlock(&p->mutex); > - > - if (p->thread_init) > - pthread_join(p->thread, NULL); > - p->thread_init=0; > + if (ctx->internal) { > + if (p->thread_init == INITIALIZED) { > + pthread_mutex_lock(&p->mutex); > + p->die = 1; > + pthread_cond_signal(&p->input_cond); > + pthread_mutex_unlock(&p->mutex); > > - if (codec->close && p->avctx) > - codec->close(p->avctx); > + pthread_join(p->thread, NULL); > + } > + if (codec->close && p->thread_init != UNINITIALIZED) > + codec->close(ctx); > > #if FF_API_THREAD_SAFE_CALLBACKS > - release_delayed_buffers(p); > + release_delayed_buffers(p); > + for (int j = 0; j < p->released_buffers_allocated; j++) > + av_frame_free(&p->released_buffers[j]); > + av_freep(&p->released_buffers); > #endif > - av_frame_free(&p->frame); > - } > + if (ctx->priv_data) { > + if (codec->priv_class) > + av_opt_free(ctx->priv_data); > + av_freep(&ctx->priv_data); > + } > > - for (i = 0; i < thread_count; i++) { > - PerThreadContext *p = &fctx->threads[i]; > + av_freep(&ctx->slice_offset); > + > + av_buffer_unref(&ctx->internal->pool); > + av_freep(&ctx->internal); > + av_buffer_unref(&ctx->hw_frames_ctx); > + } > + > + av_frame_free(&p->frame); > > pthread_mutex_destroy(&p->mutex); > pthread_mutex_destroy(&p->progress_mutex); > @@ -727,26 +746,6 @@ void ff_frame_thread_free(AVCodecContext *avctx, int > thread_count) > pthread_cond_destroy(&p->output_cond); > av_packet_unref(&p->avpkt); > > -#if FF_API_THREAD_SAFE_CALLBACKS > - for (int j = 0; j < p->released_buffers_allocated; j++) > - av_frame_free(&p->released_buffers[j]); > - av_freep(&p->released_buffers); > -#endif > - > - if (p->avctx) { > - if (codec->priv_class) > - av_opt_free(p->avctx->priv_data); > - av_freep(&p->avctx->priv_data); > - > - av_freep(&p->avctx->slice_offset); > - } > - > - if (p->avctx) { > - av_buffer_unref(&p->avctx->internal->pool); > - av_freep(&p->avctx->internal); > - av_buffer_unref(&p->avctx->hw_frames_ctx); > - } > - > av_freep(&p->avctx); > } > > @@ -791,14 +790,19 @@ int ff_frame_thread_init(AVCodecContext *avctx) > > fctx->threads = av_mallocz_array(thread_count, sizeof(PerThreadContext)); > if (!fctx->threads) { > - av_freep(&avctx->internal->thread_ctx); > - return AVERROR(ENOMEM); > + err = AVERROR(ENOMEM); > + goto threads_alloc_fail; > } > > - pthread_mutex_init(&fctx->buffer_mutex, NULL); > - pthread_mutex_init(&fctx->hwaccel_mutex, NULL); > - pthread_mutex_init(&fctx->async_mutex, NULL); > - pthread_cond_init(&fctx->async_cond, NULL); > +#define PTHREAD_INIT(type, ctx, field) \ > + if (err = pthread_ ## type ## _init(&ctx->field, NULL)) { \ > + err = AVERROR(err); \ > + goto field ## _fail; \ > + } > + PTHREAD_INIT(mutex, fctx, buffer_mutex) > + PTHREAD_INIT(mutex, fctx, hwaccel_mutex) > + PTHREAD_INIT(mutex, fctx, async_mutex) > + PTHREAD_INIT(cond, fctx, async_cond) > > fctx->async_lock = 1; > fctx->delaying = 1; > @@ -806,40 +810,37 @@ int ff_frame_thread_init(AVCodecContext *avctx) > if (codec->type == AVMEDIA_TYPE_VIDEO) > avctx->delay = src->thread_count - 1; > > - for (i = 0; i < thread_count; i++) { > - AVCodecContext *copy = av_malloc(sizeof(AVCodecContext)); > + for (i = 0; i < thread_count; ) { > PerThreadContext *p = &fctx->threads[i]; > + AVCodecContext *copy; > + int first = !i; > > - pthread_mutex_init(&p->mutex, NULL); > - pthread_mutex_init(&p->progress_mutex, NULL); > - pthread_cond_init(&p->input_cond, NULL); > - pthread_cond_init(&p->progress_cond, NULL); > - pthread_cond_init(&p->output_cond, NULL); > - > - p->frame = av_frame_alloc(); > - if (!p->frame) { > - av_freep(©); > - err = AVERROR(ENOMEM); > - goto error; > - } > + PTHREAD_INIT(mutex, p, mutex) > + PTHREAD_INIT(mutex, p, progress_mutex) > + PTHREAD_INIT(cond, p, input_cond) > + PTHREAD_INIT(cond, p, progress_cond) > + PTHREAD_INIT(cond, p, output_cond) > > - p->parent = fctx; > - p->avctx = copy; > + atomic_init(&p->state, STATE_INPUT_READY); > > + copy = av_memdup(src, sizeof(*src)); > if (!copy) { > err = AVERROR(ENOMEM); > - goto error; > + goto copy_fail; > } > + /* From now on, this PerThreadContext will be cleaned up by > + * ff_frame_thread_free in case of errors. */ > + i++; > > - *copy = *src; > + p->parent = fctx; > + p->avctx = copy; > > - copy->internal = av_malloc(sizeof(AVCodecInternal)); > + copy->internal = av_memdup(src->internal, sizeof(*src->internal)); > if (!copy->internal) { > copy->priv_data = NULL; > err = AVERROR(ENOMEM); > goto error; > } > - *copy->internal = *src->internal; > copy->internal->thread_ctx = p; > copy->internal->last_pkt_props = &p->avpkt; > > @@ -860,30 +861,66 @@ int ff_frame_thread_init(AVCodecContext *avctx) > } > } > > - if (i) > + p->frame = av_frame_alloc(); > + if (!p->frame) { > + err = AVERROR(ENOMEM); > + goto error; > + } > + > + if (!first) > copy->internal->is_copy = 1; > > - if (codec->init) > + if (codec->init) { > err = codec->init(copy); > + if (err) { > + if (codec->caps_internal & FF_CODEC_CAP_INIT_CLEANUP) > + p->thread_init = NEEDS_CLOSE; > + goto error; > + } > + } > + p->thread_init = NEEDS_CLOSE; > > - if (err) goto error; > - > - if (!i) > + if (first) > update_context_from_thread(avctx, copy, 1);
A first version of this patch here added a check for this, but in the end I removed it: Both pool as well as hw_frames_ctx must be NULL at this point (or we would be trying to unref the same reference more than once lateron, leading to use-after-free and corrupted refcounts) and therefore this call can't fail. > > atomic_init(&p->debug_threads, (copy->debug & FF_DEBUG_THREADS) != > 0); > > err = AVERROR(pthread_create(&p->thread, NULL, frame_worker_thread, > p)); > - p->thread_init= !err; > - if(!p->thread_init) > + if (err) > goto error; > + p->thread_init = INITIALIZED; > + continue; > + > + copy_fail: > + pthread_cond_destroy(&p->output_cond); > + output_cond_fail: > + pthread_cond_destroy(&p->progress_cond); > + progress_cond_fail: > + pthread_cond_destroy(&p->input_cond); > + input_cond_fail: > + pthread_mutex_destroy(&p->progress_mutex); > + progress_mutex_fail: > + pthread_mutex_destroy(&p->mutex); > + mutex_fail: > + goto error; > } > > return 0; > > error: > - ff_frame_thread_free(avctx, i+1); > + ff_frame_thread_free(avctx, i); > + return err; > > +async_cond_fail: > + pthread_mutex_destroy(&fctx->async_mutex); > +async_mutex_fail: > + pthread_mutex_destroy(&fctx->hwaccel_mutex); > +hwaccel_mutex_fail: > + pthread_mutex_destroy(&fctx->buffer_mutex); > +buffer_mutex_fail: > + av_freep(&fctx->threads); > +threads_alloc_fail: > + av_freep(&avctx->internal->thread_ctx); > return err; > } > > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
