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

When I do this:

        if (value_len < 0 || value_len >= (INT_MAX - LEN) / 2)
            return AVERROR_INVALIDDATA;

Shouldn't we return a different error code? Actually this is not about invalid
data but about a limitation in the implementation..