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

[Carl Eugen Hoyos]

Am So., 8. Aug. 2021 um 03:00 Uhr schrieb Soft Works <softworkz at hotmail.com>:
>
> > -----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.

There is:
An allocation >32bit for an attachment in an asf file is always wrong.

Carl Eugen