[FFmpeg-devel] [PATCH v2] libavformat/asfdec: Fix regression bug when reading image attachments

[James Almer]

On 8/7/2021 10:00 PM, Soft Works wrote:
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
>> James Almer
>> Sent: Sunday, 8 August 2021 02:47
>> To: ffmpeg-devel at ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH v2] libavformat/asfdec: Fix regression
>> bug when reading image attachments
>>
>> On 8/7/2021 9:38 PM, Soft Works wrote:
>>>> -----Original Message-----
>>>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
>>>> Carl Eugen Hoyos
>>>> Sent: Sunday, 8 August 2021 01:58
>>>> To: FFmpeg development discussions and patches <ffmpeg-
>>>> devel at ffmpeg.org>
>>>> Subject: Re: [FFmpeg-devel] [PATCH v2] libavformat/asfdec: Fix
>>>> regression bug when reading image attachments
>>>>
>>>> Am So., 8. Aug. 2021 um 01:53 Uhr schrieb Soft Works
>>>> <softworkz at hotmail.com>:
>>>>>
>>>>> Commit c8140fe7324f264faacf7395b27e12531d1f13f7 had introduced a
>>>> check for value_len > UINT16_MAX.
>>>>> As a consequence, attached images of sizes larger than UINT16_MAX
>>>>> could
>>>> no longer be read.
>>>>>
>>>>> Signed-off-by: softworkz <softworkz at hotmail.com>
>>>>> ---
>>>>> v2: Fix without changing variable type  libavformat/asfdec_f.c | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c index
>>>>> ff6ddfb967..b9f3918495 100644
>>>>> --- a/libavformat/asfdec_f.c
>>>>> +++ b/libavformat/asfdec_f.c
>>>>> @@ -614,7 +614,7 @@ static int asf_read_metadata(AVFormatContext
>> *s,
>>>> int64_t size)
>>>>>            value_type = avio_rl16(pb); /* value_type */
>>>>>            value_len  = avio_rl32(pb);
>>>>>
>>>>> -        if (value_len < 0 || value_len > UINT16_MAX)
>>>>> +        if (value_len < 0)
>>>>>                return AVERROR_INVALIDDATA;
>>>>
>>>> I may misread the code but it appears to me that an assert can be
>>>> triggered now, no?
>>>
>>> Right, for these 11 discrete size values only, though:
>>>
>>> 2147483647, 2147483646, 2147483645, 2147483644, 2147483643,
>>> 2147483642, 2147483641, 2147483640, 2147483639, 2147483638
>>> 2147483636
>>>
>>> A chance of 11 in 4 Billions :-)
>>
>> len < (INT_MAX - LEN) / 2 is more than half the range of an int.
> 
> I've been one step ahead in calculation:
> 
> av_malloc takes size_t and on 32bit platforms, this is uint32, which means
> that it can take a maximum of 4.294.967.295.
> (4.294.967.295 - 22) / 2 = 2.147.483.636  (+ 11 makes INT32_MAX)

The assert right now checks strictly for INT_MAX, not UINT_MAX. If len 
is >= 1073741812, it will trigger. Whatever av_malloc() could handle 
afterwards does not play a part in that.

> 
>>
>>>
>>> TBH, I don't understand this assert. When the attachment is too large
>>> to handle, we should rather log a warning and goto finish instead IMO.
>>
>> The assert is to ensure get_tag() is not called with out of range values, so the
>> relevant checks should happen before the call.
> 
> I'm not sure whether I'd agree with that. It shouldn't be the caller
> needing to know what the function can handle and what it can't handle.
> 
> The INT32/2 is not only wrong, it also does _only_ apply to reading
> unicode. For ASCII and byte arrays, there is no need for this restriction.
> And on 64bit platforms, there's no need for this that restriction at all.

If you want to change this and allow values higher than (INT_MAX - LEN) 
/ 2 or even replace the assert with a normal check then feel free to, 
but your patch as is will trigger this assert on more than half the 
possible values of len.

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