[FFmpeg-devel] [PATCH] avcodec/pthread_frame: fix hw_device_ctx leak

[Andreas Rheinhardt]

Thomas Guillem:
> 
> 
> On Thu, Dec 2, 2021, at 14:38, Andreas Rheinhardt wrote:
>> Thomas Guillem:
>>> Reproduced when using the VAAPI va module on VLC 4.0. No leaks when
>>> setting thread count to 1.
>>> ---
>>>  libavcodec/pthread_frame.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
>>> index 73b1b7d7d9..4c578aa44a 100644
>>> --- a/libavcodec/pthread_frame.c
>>> +++ b/libavcodec/pthread_frame.c
>>> @@ -747,6 +747,7 @@ void ff_frame_thread_free(AVCodecContext *avctx, int thread_count)
>>>              av_buffer_unref(&ctx->internal->pool);
>>>              av_freep(&ctx->internal);
>>>              av_buffer_unref(&ctx->hw_frames_ctx);
>>> +            av_buffer_unref(&ctx->hw_device_ctx);
>>>          }
>>>  
>>>          av_frame_free(&p->frame);
>>>
>>
>> The AVCodecContext that is freed here is not a full AVCodecContext: It
>> never received a reference to hw_device_ctx of its own. Unreferencing
>> this here will therefore mess up the refcount and lead to use-after-frees.
>> (What is the reference count of hw_device_ctx at this point? Libavcodec
>> should only hold one reference at that point, namely the one in the main
>> (user-facing) AVCodecContext; this reference will be unreferenced when
>> avcodec_close()/avcodec_free_context() is called for the main context.)
> 
> Hello Andreas, thanks for the review.
> 
> The problem is that hw_device_ctx is NULL when avcodec_close() is called in that particular usecase. My first guess is that the get_format callback can be called from any avctx (and not necessarily the main one that will be closed by avcodec_close().
> 
> I'm new to this code, but I thought that only one FrameThreadContext could meet the if (ctx->internal) condition, so there should be only one reference to the hw_device_ctx.
> 

1. hw_device_ctx is in the public AVCodecContext, not in the private
AVCodecInternal.
2. ctx->internal is intended to be allocated for all the internal
AVCodecContexts; the check is only there to ensure that we don't crash
if an allocation error happens.
3. libavcodec only ever modifies avctx->hw_device_ctx at one place: in
avcodec_close() where is unreferences the reference (if any). So if it
was set in avcodec_open2() and NULL in avcodec_close() (i.e. immediately
before avcodec_close()), it has been modified somehow and you need to
find out who did it.
4. Some parts of the code use hw_device_ctx to derive references from
it. Maybe one of these references did not get freed?
(5. What do Valgrind/ASAN say? When was the reference that leaked created?)

- Andreas