| Message ID | MN2PR04MB5981821091BF0E289EDB41CBBAF49@MN2PR04MB5981.namprd04.prod.outlook.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [FFmpeg-devel,v2] libavformat/asfdec: Fix regression bug when reading image attachments | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Carl Eugen Hoyos > Sent: Sunday, 8 August 2021 01:58 > To: FFmpeg development discussions and patches <ffmpeg- > devel@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@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@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 :-) 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. softworkz
On 8/7/2021 9:38 PM, Soft Works wrote: >> -----Original Message----- >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of >> Carl Eugen Hoyos >> Sent: Sunday, 8 August 2021 01:58 >> To: FFmpeg development discussions and patches <ffmpeg- >> devel@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@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@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. > > 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. > > softworkz > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > James Almer > Sent: Sunday, 8 August 2021 02:47 > To: ffmpeg-devel@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@ffmpeg.org> On Behalf Of > >> Carl Eugen Hoyos > >> Sent: Sunday, 8 August 2021 01:58 > >> To: FFmpeg development discussions and patches <ffmpeg- > >> devel@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@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@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
Am So., 8. Aug. 2021 um 03:00 Uhr schrieb Soft Works <softworkz@hotmail.com>: > > > -----Original Message----- > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > James Almer > > Sent: Sunday, 8 August 2021 02:47 > > To: ffmpeg-devel@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@ffmpeg.org> On Behalf Of > > >> Carl Eugen Hoyos > > >> Sent: Sunday, 8 August 2021 01:58 > > >> To: FFmpeg development discussions and patches <ffmpeg- > > >> devel@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@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@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
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Carl Eugen Hoyos > Sent: Sunday, 8 August 2021 03:03 > To: FFmpeg development discussions and patches <ffmpeg- > devel@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@hotmail.com>: > > > > > -----Original Message----- > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > > James Almer > > > Sent: Sunday, 8 August 2021 02:47 > > > To: ffmpeg-devel@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@ffmpeg.org> On Behalf > Of > > > >> Carl Eugen Hoyos > > > >> Sent: Sunday, 8 August 2021 01:58 > > > >> To: FFmpeg development discussions and patches <ffmpeg- > > > >> devel@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@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@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
On 8/7/2021 10:00 PM, Soft Works wrote: >> -----Original Message----- >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of >> James Almer >> Sent: Sunday, 8 August 2021 02:47 >> To: ffmpeg-devel@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@ffmpeg.org> On Behalf Of >>>> Carl Eugen Hoyos >>>> Sent: Sunday, 8 August 2021 01:58 >>>> To: FFmpeg development discussions and patches <ffmpeg- >>>> devel@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@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@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. > > softworkz > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > James Almer > Sent: Sunday, 8 August 2021 03:19 > To: ffmpeg-devel@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@ffmpeg.org> On Behalf Of > >> James Almer > >> Sent: Sunday, 8 August 2021 02:47 > >> To: ffmpeg-devel@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@ffmpeg.org> On Behalf > Of > >>>> Carl Eugen Hoyos > >>>> Sent: Sunday, 8 August 2021 01:58 > >>>> To: FFmpeg development discussions and patches <ffmpeg- > >>>> devel@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@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@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..
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > James Almer > Sent: Sunday, 8 August 2021 03:19 > To: ffmpeg-devel@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@ffmpeg.org> On Behalf Of > >> James Almer > >> Sent: Sunday, 8 August 2021 02:47 > >> To: ffmpeg-devel@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@ffmpeg.org> On Behalf > Of > >>>> Carl Eugen Hoyos > >>>> Sent: Sunday, 8 August 2021 01:58 > >>>> To: FFmpeg development discussions and patches <ffmpeg- > >>>> devel@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@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@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..
Am So., 8. Aug. 2021 um 03:40 Uhr schrieb Soft Works <softworkz@hotmail.com>: > 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.. It is definitely invalid data. Carl Eugen
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; name_len_utf8 = 2*name_len_utf16 + 1;
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@hotmail.com> --- v2: Fix without changing variable type libavformat/asfdec_f.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)