[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
> Carl Eugen Hoyos
> Sent: Sunday, 8 August 2021 03:03
> 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 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.

Yes, that's UINT32_MAX, but  UINT32_MAX * 2 + 22 can only be allocated
on 64bit platforms. See

value = av_malloc(2 * len + LEN);

in get_tag