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

Soft Works softworkz at hotmail.com
Sun Aug 8 04:31:02 EEST 2021



> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> James Almer
> Sent: Sunday, 8 August 2021 03:19
> 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 10:00 PM, Soft Works wrote:
> >> -----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)
> 
> 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.

Yea that's true of course. Honestly, I don't know what to do. There more I
look around the more flawed do things appear. Just look at the type2_size
parameter of get_tag and all callers supply values like 32 and 16, but actually
that parameter should not exist at all: it is only used for the types bool, qword,
dword and word, and for these cases, the length is defined by the spec and
should not be specified by callers.

Or look at the byte_array type, where it is allocating the double size of the
byte array without actually using it.

Adding that value of 22 is actually not needed at all. It's just developer laziness
for having at least 22 bytes in case that the give length is zero.

A little bit frightening all this. :-) 
I guess I stay on the simple side for now..


More information about the ffmpeg-devel mailing list