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

[James Almer]

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.

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

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