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

[Soft Works]

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

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

softworkz