On 8/7/2021 10:00 PM, Soft Works wrote:
-----Original Message-----
From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of
James Almer
Sent: Sunday, 8 August 2021 02:47
To: ffmpeg-devel@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-boun...@ffmpeg.org> On Behalf Of
Carl Eugen Hoyos
Sent: Sunday, 8 August 2021 01:58
To: FFmpeg development discussions and patches <ffmpeg-
de...@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
<softwo...@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 <softwo...@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@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
