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

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(+)

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.)

- Andreas

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.

Best,
Thomas

>
> - Andreas
>
> _______________________________________________
> 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".

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

On Thu, Dec 2, 2021, at 15:05, Andreas Rheinhardt wrote:
> 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?)

OK, all your point are valid, I missed a precious comment from the avcodec.h API:
"For both encoders and decoders this field should be set before            
 avcodec_open2() is called and must not be written to thereafter."

Hence, the incomprehension.

So, to sum up: the reference that leaks is created from VLC, from the get_format() callback.
Looking at pthread_frame.c, it seems that get_format() is called with a AVCodecContext owned by a PerThreadContext, so not the main AVCodecContext. That is why avcodec_close() is not closing it.

I just discovered that it's not officially supported then, but I really don't see any other way to plug VAAPI between VLC and ffmpeg.

I explain: VLC can't know the hw_device that will be used when it calls avcodec_open2(). VLC has a similar struct than hw_device, called dec_device. For VAAPI, this struct hold the va_dpy that is created when the VLC avcodec module first create the video output, from the get_format() callback.

I see 2 possibilities:
1/ Patch the documentation and allow to set a hw_device_ctx from get_format(), review all use case of hw_device_ctx to check if there is no inconsistency with this new change.
2/ Use any other way to pass the va_dpy from the get_format()? I didn't see any, but maybe I missed something obvious.

Best,
Thomas

>
> - Andreas
> _______________________________________________
> 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".

Quoting Thomas Guillem (2021-12-02 20:44:33)
> 
> On Thu, Dec 2, 2021, at 15:05, Andreas Rheinhardt wrote:
> > 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?)
> 
> OK, all your point are valid, I missed a precious comment from the avcodec.h API:
> "For both encoders and decoders this field should be set before            
>  avcodec_open2() is called and must not be written to thereafter."
> 
> Hence, the incomprehension.
> 
> So, to sum up: the reference that leaks is created from VLC, from the get_format() callback.
> Looking at pthread_frame.c, it seems that get_format() is called with a AVCodecContext owned by a PerThreadContext, so not the main AVCodecContext. That is why avcodec_close() is not closing it.
> 
> I just discovered that it's not officially supported then, but I really don't see any other way to plug VAAPI between VLC and ffmpeg.
> 
> I explain: VLC can't know the hw_device that will be used when it calls avcodec_open2(). VLC has a similar struct than hw_device, called dec_device. For VAAPI, this struct hold the va_dpy that is created when the VLC avcodec module first create the video output, from the get_format() callback.
> 
> I see 2 possibilities:
> 1/ Patch the documentation and allow to set a hw_device_ctx from get_format(), review all use case of hw_device_ctx to check if there is no inconsistency with this new change.
> 2/ Use any other way to pass the va_dpy from the get_format()? I didn't see any, but maybe I missed something obvious.

2. is the right answer - you're supposed to set hw_frames_ctx, possibly
using the avcodec_get_hw_frames_parameters() helper function.

On Fri, Dec 3, 2021, at 09:52, Anton Khirnov wrote:
> Quoting Thomas Guillem (2021-12-02 20:44:33)
>> 
>> On Thu, Dec 2, 2021, at 15:05, Andreas Rheinhardt wrote:
>> > 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?)
>> 
>> OK, all your point are valid, I missed a precious comment from the avcodec.h API:
>> "For both encoders and decoders this field should be set before            
>>  avcodec_open2() is called and must not be written to thereafter."
>> 
>> Hence, the incomprehension.
>> 
>> So, to sum up: the reference that leaks is created from VLC, from the get_format() callback.
>> Looking at pthread_frame.c, it seems that get_format() is called with a AVCodecContext owned by a PerThreadContext, so not the main AVCodecContext. That is why avcodec_close() is not closing it.
>> 
>> I just discovered that it's not officially supported then, but I really don't see any other way to plug VAAPI between VLC and ffmpeg.
>> 
>> I explain: VLC can't know the hw_device that will be used when it calls avcodec_open2(). VLC has a similar struct than hw_device, called dec_device. For VAAPI, this struct hold the va_dpy that is created when the VLC avcodec module first create the video output, from the get_format() callback.
>> 
>> I see 2 possibilities:
>> 1/ Patch the documentation and allow to set a hw_device_ctx from get_format(), review all use case of hw_device_ctx to check if there is no inconsistency with this new change.
>> 2/ Use any other way to pass the va_dpy from the get_format()? I didn't see any, but maybe I missed something obvious.
>
> 2. is the right answer - you're supposed to set hw_frames_ctx, possibly
> using the avcodec_get_hw_frames_parameters() helper function.

Thanks! That is exactly what I needed.


>
> -- 
> Anton Khirnov
> _______________________________________________
> 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".