[FFmpeg-devel] [PATCH] avutil/hwcontext: check the null pointer input value before use it

[FFmpeg-devel] [PATCH] avutil/hwcontext: check the null pointer input value before use it

[Steven Liu]

because the src, src->hw_frames_ctx and src->hw_frames_ctx->data can be
set to null when the user calling av_hwframe_transfer_data, this will
get crash if they are null.

Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
---
 libavutil/hwcontext.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
index 31c7840dba..b42a3a6d4d 100644
--- a/libavutil/hwcontext.c
+++ b/libavutil/hwcontext.c
@@ -396,10 +396,13 @@ int av_hwframe_transfer_get_formats(AVBufferRef *hwframe_ref,
 
 static int transfer_data_alloc(AVFrame *dst, const AVFrame *src, int flags)
 {
-    AVHWFramesContext *ctx = (AVHWFramesContext*)src->hw_frames_ctx->data;
+    AVHWFramesContext *ctx = NULL;
     AVFrame *frame_tmp;
     int ret = 0;
 
+    if (!src || !src->hw_frames_ctx || !src->hw_frames_ctx->data)
+        return AVERROR(EINVAL);
+    ctx = (AVHWFramesContext*)src->hw_frames_ctx->data;
     frame_tmp = av_frame_alloc();
     if (!frame_tmp)
         return AVERROR(ENOMEM);
-- 
2.25.0

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

Re: [FFmpeg-devel] [PATCH] avutil/hwcontext: check the null pointer input value before use it

[Steven Liu]

> 2022年2月10日 下午8:20，Steven Liu <lq@chinaffmpeg.org> 写道：
> 
> because the src, src->hw_frames_ctx and src->hw_frames_ctx->data can be
> set to null when the user calling av_hwframe_transfer_data, this will
> get crash if they are null.
> 
Reported-by: TOTE Robot <oslab@tsinghua.edu.cn>
> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> ---
> libavutil/hwcontext.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
> index 31c7840dba..b42a3a6d4d 100644
> --- a/libavutil/hwcontext.c
> +++ b/libavutil/hwcontext.c
> @@ -396,10 +396,13 @@ int av_hwframe_transfer_get_formats(AVBufferRef *hwframe_ref,
> 
> static int transfer_data_alloc(AVFrame *dst, const AVFrame *src, int flags)
> {
> -    AVHWFramesContext *ctx = (AVHWFramesContext*)src->hw_frames_ctx->data;
> +    AVHWFramesContext *ctx = NULL;
>     AVFrame *frame_tmp;
>     int ret = 0;
> 
> +    if (!src || !src->hw_frames_ctx || !src->hw_frames_ctx->data)
> +        return AVERROR(EINVAL);
> +    ctx = (AVHWFramesContext*)src->hw_frames_ctx->data;
>     frame_tmp = av_frame_alloc();
>     if (!frame_tmp)
>         return AVERROR(ENOMEM);
> -- 
> 2.25.0
> 

Thanks

Steven Liu

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

Re: [FFmpeg-devel] [PATCH] avutil/hwcontext: check the null pointer input value before use it

[James Almer]

On 2/10/2022 9:20 AM, Steven Liu wrote:
> because the src, src->hw_frames_ctx and src->hw_frames_ctx->data can be
> set to null when the user calling av_hwframe_transfer_data, this will
> get crash if they are null.

src can not be NULL. The doxy doesn't allow it.

And if transfer_data_alloc() is called, it's because dst is "clean", and 
src must then have a hw_frames_ctx (The doxy explicitly states "At least 
one of dst/src must have an AVHWFramesContext attached").

> 
> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> ---
>   libavutil/hwcontext.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
> index 31c7840dba..b42a3a6d4d 100644
> --- a/libavutil/hwcontext.c
> +++ b/libavutil/hwcontext.c
> @@ -396,10 +396,13 @@ int av_hwframe_transfer_get_formats(AVBufferRef *hwframe_ref,
>   
>   static int transfer_data_alloc(AVFrame *dst, const AVFrame *src, int flags)
>   {
> -    AVHWFramesContext *ctx = (AVHWFramesContext*)src->hw_frames_ctx->data;
> +    AVHWFramesContext *ctx = NULL;
>       AVFrame *frame_tmp;
>       int ret = 0;
>   
> +    if (!src || !src->hw_frames_ctx || !src->hw_frames_ctx->data)
> +        return AVERROR(EINVAL);
> +    ctx = (AVHWFramesContext*)src->hw_frames_ctx->data;
>       frame_tmp = av_frame_alloc();
>       if (!frame_tmp)
>           return AVERROR(ENOMEM);
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avutil/hwcontext: check the null pointer input value before use it

[Steven Liu]

> 2022年2月10日 下午8:27，James Almer <jamrial@gmail.com> 写道：
> 
> On 2/10/2022 9:20 AM, Steven Liu wrote:
>> because the src, src->hw_frames_ctx and src->hw_frames_ctx->data can be
>> set to null when the user calling av_hwframe_transfer_data, this will
>> get crash if they are null.
> 
> src can not be NULL. The doxy doesn't allow it.

Hi James,

User call av_hwframe_transfer_data like this:

av_hwframe_transfer_data(dst, NULL, 0);

It will crash when dst->buf[0] is null.
Because dst->buf[0] is null and src is null, it will call transfer_data_alloc, but the first line is ctx = (AVHWFramesContext*)src->hw_frames_ctx->data; in transfer_data_alloc,
It using src->hw_frames_ctx. 

av_hwframe_transfer_data is av_*, it is API to user.
Maybe this is not logic problem, looks like a security problem.
> 
> And if transfer_data_alloc() is called, it's because dst is "clean", and src must then have a hw_frames_ctx (The doxy explicitly states "At least one of dst/src must have an AVHWFramesContext attached").
> 
>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
>> ---
>>  libavutil/hwcontext.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>> diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
>> index 31c7840dba..b42a3a6d4d 100644
>> --- a/libavutil/hwcontext.c
>> +++ b/libavutil/hwcontext.c
>> @@ -396,10 +396,13 @@ int av_hwframe_transfer_get_formats(AVBufferRef *hwframe_ref,
>>    static int transfer_data_alloc(AVFrame *dst, const AVFrame *src, int flags)
>>  {
>> -    AVHWFramesContext *ctx = (AVHWFramesContext*)src->hw_frames_ctx->data;
>> +    AVHWFramesContext *ctx = NULL;
>>      AVFrame *frame_tmp;
>>      int ret = 0;
>>  +    if (!src || !src->hw_frames_ctx || !src->hw_frames_ctx->data)
>> +        return AVERROR(EINVAL);
>> +    ctx = (AVHWFramesContext*)src->hw_frames_ctx->data;
>>      frame_tmp = av_frame_alloc();
>>      if (!frame_tmp)
>>          return AVERROR(ENOMEM);
> _______________________________________________
> 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".
> 

Thanks

Steven Liu

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

Re: [FFmpeg-devel] [PATCH] avutil/hwcontext: check the null pointer input value before use it

[James Almer]

On 2/10/2022 10:43 PM, Steven Liu wrote:
> 
> 
>> 2022年2月10日 下午8:27，James Almer <jamrial@gmail.com> 写道：
>>
>> On 2/10/2022 9:20 AM, Steven Liu wrote:
>>> because the src, src->hw_frames_ctx and src->hw_frames_ctx->data can be
>>> set to null when the user calling av_hwframe_transfer_data, this will
>>> get crash if they are null.
>>
>> src can not be NULL. The doxy doesn't allow it.
> 
> Hi James,
> 
> User call av_hwframe_transfer_data like this:
> 
> av_hwframe_transfer_data(dst, NULL, 0);
> 
> It will crash when dst->buf[0] is null.
> Because dst->buf[0] is null and src is null, it will call transfer_data_alloc, but the first line is ctx = (AVHWFramesContext*)src->hw_frames_ctx->data; in transfer_data_alloc,
> It using src->hw_frames_ctx.
> 
> av_hwframe_transfer_data is av_*, it is API to user.
> Maybe this is not logic problem, looks like a security problem.

I know what happens when you pass NULL as src argument. My point is that 
it's not a security problem because that's an API violation and an 
explicitly forbidden scenario: Neither src or dst can be NULL, and at 
least one of them must have an AVHWFramesContext attached. Any 
application not following that is faulty and buggy, and needs to be fixed.

And you can get crashes by passing NULL arguments to lots of public 
libav* functions, not just this one.

>>
>> And if transfer_data_alloc() is called, it's because dst is "clean", and src must then have a hw_frames_ctx (The doxy explicitly states "At least one of dst/src must have an AVHWFramesContext attached").
>>
>>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
>>> ---
>>>   libavutil/hwcontext.c | 5 ++++-
>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>> diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
>>> index 31c7840dba..b42a3a6d4d 100644
>>> --- a/libavutil/hwcontext.c
>>> +++ b/libavutil/hwcontext.c
>>> @@ -396,10 +396,13 @@ int av_hwframe_transfer_get_formats(AVBufferRef *hwframe_ref,
>>>     static int transfer_data_alloc(AVFrame *dst, const AVFrame *src, int flags)
>>>   {
>>> -    AVHWFramesContext *ctx = (AVHWFramesContext*)src->hw_frames_ctx->data;
>>> +    AVHWFramesContext *ctx = NULL;
>>>       AVFrame *frame_tmp;
>>>       int ret = 0;
>>>   +    if (!src || !src->hw_frames_ctx || !src->hw_frames_ctx->data)
>>> +        return AVERROR(EINVAL);
>>> +    ctx = (AVHWFramesContext*)src->hw_frames_ctx->data;
>>>       frame_tmp = av_frame_alloc();
>>>       if (!frame_tmp)
>>>           return AVERROR(ENOMEM);
>> _______________________________________________
>> 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".
>>
> 
> Thanks
> 
> Steven Liu
> 
> _______________________________________________
> 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".
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avutil/hwcontext: check the null pointer input value before use it

[Steven Liu]

> 2022年2月11日 上午10:01，James Almer <jamrial@gmail.com> 写道：
> 
> On 2/10/2022 10:43 PM, Steven Liu wrote:
>>> 2022年2月10日 下午8:27，James Almer <jamrial@gmail.com> 写道：
>>> 
>>> On 2/10/2022 9:20 AM, Steven Liu wrote:
>>>> because the src, src->hw_frames_ctx and src->hw_frames_ctx->data can be
>>>> set to null when the user calling av_hwframe_transfer_data, this will
>>>> get crash if they are null.
>>> 
>>> src can not be NULL. The doxy doesn't allow it.
>> Hi James,
>> User call av_hwframe_transfer_data like this:
>> av_hwframe_transfer_data(dst, NULL, 0);
>> It will crash when dst->buf[0] is null.
>> Because dst->buf[0] is null and src is null, it will call transfer_data_alloc, but the first line is ctx = (AVHWFramesContext*)src->hw_frames_ctx->data; in transfer_data_alloc,
>> It using src->hw_frames_ctx.
>> av_hwframe_transfer_data is av_*, it is API to user.
>> Maybe this is not logic problem, looks like a security problem.
> 
> I know what happens when you pass NULL as src argument. My point is that it's not a security problem because that's an API violation and an explicitly forbidden scenario: Neither src or dst can be NULL, and at least one of them must have an AVHWFramesContext attached. Any application not following that is faulty and buggy, and needs to be fixed.
> 
> And you can get crashes by passing NULL arguments to lots of public libav* functions, not just this one.
Won’t we fix them?


Thanks

Steven Liu

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

Re: [FFmpeg-devel] [PATCH] avutil/hwcontext: check the null pointer input value before use it

[James Almer]

On 2/10/2022 11:03 PM, Steven Liu wrote:
> 
> 
>> 2022年2月11日 上午10:01，James Almer <jamrial@gmail.com> 写道：
>>
>> On 2/10/2022 10:43 PM, Steven Liu wrote:
>>>> 2022年2月10日 下午8:27，James Almer <jamrial@gmail.com> 写道：
>>>>
>>>> On 2/10/2022 9:20 AM, Steven Liu wrote:
>>>>> because the src, src->hw_frames_ctx and src->hw_frames_ctx->data can be
>>>>> set to null when the user calling av_hwframe_transfer_data, this will
>>>>> get crash if they are null.
>>>>
>>>> src can not be NULL. The doxy doesn't allow it.
>>> Hi James,
>>> User call av_hwframe_transfer_data like this:
>>> av_hwframe_transfer_data(dst, NULL, 0);
>>> It will crash when dst->buf[0] is null.
>>> Because dst->buf[0] is null and src is null, it will call transfer_data_alloc, but the first line is ctx = (AVHWFramesContext*)src->hw_frames_ctx->data; in transfer_data_alloc,
>>> It using src->hw_frames_ctx.
>>> av_hwframe_transfer_data is av_*, it is API to user.
>>> Maybe this is not logic problem, looks like a security problem.
>>
>> I know what happens when you pass NULL as src argument. My point is that it's not a security problem because that's an API violation and an explicitly forbidden scenario: Neither src or dst can be NULL, and at least one of them must have an AVHWFramesContext attached. Any application not following that is faulty and buggy, and needs to be fixed.
>>
>> And you can get crashes by passing NULL arguments to lots of public libav* functions, not just this one.
> Won’t we fix them?

We have nothing to fix. If someone writes an application that calls 
avcodec_open2(NULL, NULL, NULL) despite it being strictly forbidden, 
then it's their fault and they deserve the segfaults they will get.

> 
> 
> Thanks
> 
> Steven Liu
> 
> _______________________________________________
> 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".
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avutil/hwcontext: check the null pointer input value before use it

[Steven Liu]

> 2022年2月11日 上午10:10，James Almer <jamrial@gmail.com> 写道：
> 
> 
> 
> On 2/10/2022 11:03 PM, Steven Liu wrote:
>>> 2022年2月11日 上午10:01，James Almer <jamrial@gmail.com> 写道：
>>> 
>>> On 2/10/2022 10:43 PM, Steven Liu wrote:
>>>>> 2022年2月10日 下午8:27，James Almer <jamrial@gmail.com> 写道：
>>>>> 
>>>>> On 2/10/2022 9:20 AM, Steven Liu wrote:
>>>>>> because the src, src->hw_frames_ctx and src->hw_frames_ctx->data can be
>>>>>> set to null when the user calling av_hwframe_transfer_data, this will
>>>>>> get crash if they are null.
>>>>> 
>>>>> src can not be NULL. The doxy doesn't allow it.
>>>> Hi James,
>>>> User call av_hwframe_transfer_data like this:
>>>> av_hwframe_transfer_data(dst, NULL, 0);
>>>> It will crash when dst->buf[0] is null.
>>>> Because dst->buf[0] is null and src is null, it will call transfer_data_alloc, but the first line is ctx = (AVHWFramesContext*)src->hw_frames_ctx->data; in transfer_data_alloc,
>>>> It using src->hw_frames_ctx.
>>>> av_hwframe_transfer_data is av_*, it is API to user.
>>>> Maybe this is not logic problem, looks like a security problem.
>>> 
>>> I know what happens when you pass NULL as src argument. My point is that it's not a security problem because that's an API violation and an explicitly forbidden scenario: Neither src or dst can be NULL, and at least one of them must have an AVHWFramesContext attached. Any application not following that is faulty and buggy, and needs to be fixed.
>>> 
>>> And you can get crashes by passing NULL arguments to lots of public libav* functions, not just this one.
>> Won’t we fix them?
> 
> We have nothing to fix. If someone writes an application that calls avcodec_open2(NULL, NULL, NULL) despite it being strictly forbidden, then it's their fault and they deserve the segfaults they will get.
I think API should give the caller an error message or failed value, because segfault is an exception and that means API is not robust, the return value should EINVAL if the user input arguments incorrect.


Thanks

Steven Liu

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

Re: [FFmpeg-devel] [PATCH] avutil/hwcontext: check the null pointer input value before use it

[James Almer]

On 2/10/2022 11:16 PM, Steven Liu wrote:
> 
> 
>> 2022年2月11日 上午10:10，James Almer <jamrial@gmail.com> 写道：
>>
>>
>>
>> On 2/10/2022 11:03 PM, Steven Liu wrote:
>>>> 2022年2月11日 上午10:01，James Almer <jamrial@gmail.com> 写道：
>>>>
>>>> On 2/10/2022 10:43 PM, Steven Liu wrote:
>>>>>> 2022年2月10日 下午8:27，James Almer <jamrial@gmail.com> 写道：
>>>>>>
>>>>>> On 2/10/2022 9:20 AM, Steven Liu wrote:
>>>>>>> because the src, src->hw_frames_ctx and src->hw_frames_ctx->data can be
>>>>>>> set to null when the user calling av_hwframe_transfer_data, this will
>>>>>>> get crash if they are null.
>>>>>>
>>>>>> src can not be NULL. The doxy doesn't allow it.
>>>>> Hi James,
>>>>> User call av_hwframe_transfer_data like this:
>>>>> av_hwframe_transfer_data(dst, NULL, 0);
>>>>> It will crash when dst->buf[0] is null.
>>>>> Because dst->buf[0] is null and src is null, it will call transfer_data_alloc, but the first line is ctx = (AVHWFramesContext*)src->hw_frames_ctx->data; in transfer_data_alloc,
>>>>> It using src->hw_frames_ctx.
>>>>> av_hwframe_transfer_data is av_*, it is API to user.
>>>>> Maybe this is not logic problem, looks like a security problem.
>>>>
>>>> I know what happens when you pass NULL as src argument. My point is that it's not a security problem because that's an API violation and an explicitly forbidden scenario: Neither src or dst can be NULL, and at least one of them must have an AVHWFramesContext attached. Any application not following that is faulty and buggy, and needs to be fixed.
>>>>
>>>> And you can get crashes by passing NULL arguments to lots of public libav* functions, not just this one.
>>> Won’t we fix them?
>>
>> We have nothing to fix. If someone writes an application that calls avcodec_open2(NULL, NULL, NULL) despite it being strictly forbidden, then it's their fault and they deserve the segfaults they will get.
> I think API should give the caller an error message or failed value, because segfault is an exception and that means API is not robust, the return value should EINVAL if the user input arguments incorrect.

I disagree. Returning EINVAL at runtime lets the user know they probably 
did something wrong, like trying to open a decoder context using a 
decoder that's not in the whitelist. That's not API misuse, that's an 
invalid argument that could realistically happen in an application 
exposing such settings to the user.
But passing a NULL AVCodecContext? That's not the user doing something 
wrong, that's the application developer doing something wrong. A 
graceful failure in that case is misleading.

With av_hwframe_transfer_data() you transfer data from something to 
something else. If src is NULL, what are you transferring data from? Why 
did your application ever try to do that? You need to fix it so it never 
happens.

> 
> 
> Thanks
> 
> Steven Liu
> 
> _______________________________________________
> 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".
_______________________________________________
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".