diff mbox series

[FFmpeg-devel,v4] Unbreak av_malloc_max(0) API/ABI

Message ID 20201016085722.21866-1-joakim.tjernlund@infinera.com
State New
Headers show
Series [FFmpeg-devel,v4] Unbreak av_malloc_max(0) API/ABI
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make warning Make failed

Commit Message

Joakim Tjernlund Oct. 16, 2020, 8:57 a.m. UTC
From https://bugs.chromium.org/p/chromium/issues/detail?id=1095962
----------------------------
This seems to be caused by the custom handling of "av_max_alloc(0)" in
Chromium's ffmpeg fork to mean unlimited (added in [1]).

Upstream ffmpeg doesn't treat 0 as a special value; versions before 4.3 seemingly worked
because 32 was subtracted from max_alloc_size (set to 0 by Chromium) resulting in an
integer underflow, making the effective limit be SIZE_MAX - 31.

Now that the above underflow doesn't happen, the tab just crashes. The upstream change
for no longer subtracting 32 from max_alloc_size was included in ffmpeg 4.3. [2]

[1] https://chromium-review.googlesource.com/c/chromium/third_party/ffmpeg/+/73563
[2] https://github.com/FFmpeg/FFmpeg/commit/731c77589841
---------------------------

Restore av_malloc_max(0) to MAX_INT fixing MS Teams, Discord older chromium etc.

Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
---

 v2: Cover the full API range 0-31

 v3: Closer compat with < 4.3 ffmpeg

 v4: Adjust size accoriding to Andreas Rheinhardt comments

 libavutil/mem.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Joakim Tjernlund Oct. 22, 2020, 12:17 p.m. UTC | #1
Ping ?

 Jocke

On Fri, 2020-10-16 at 10:57 +0200, Joakim Tjernlund wrote:
> From https://bugs.chromium.org/p/chromium/issues/detail?id=1095962
> ----------------------------
> This seems to be caused by the custom handling of "av_max_alloc(0)" in
> Chromium's ffmpeg fork to mean unlimited (added in [1]).
> 
> Upstream ffmpeg doesn't treat 0 as a special value; versions before 4.3 seemingly worked
> because 32 was subtracted from max_alloc_size (set to 0 by Chromium) resulting in an
> integer underflow, making the effective limit be SIZE_MAX - 31.
> 
> Now that the above underflow doesn't happen, the tab just crashes. The upstream change
> for no longer subtracting 32 from max_alloc_size was included in ffmpeg 4.3. [2]
> 
> [1] https://chromium-review.googlesource.com/c/chromium/third_party/ffmpeg/+/73563
> [2] https://github.com/FFmpeg/FFmpeg/commit/731c77589841
> ---------------------------
> 
> Restore av_malloc_max(0) to MAX_INT fixing MS Teams, Discord older chromium etc.
> 
> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> ---
> 
>  v2: Cover the full API range 0-31
> 
>  v3: Closer compat with < 4.3 ffmpeg
> 
>  v4: Adjust size accoriding to Andreas Rheinhardt comments
> 
>  libavutil/mem.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libavutil/mem.c b/libavutil/mem.c
> index cfb6d8a..44870a9 100644
> --- a/libavutil/mem.c
> +++ b/libavutil/mem.c
> @@ -71,6 +71,8 @@ void  free(void *ptr);
>  static size_t max_alloc_size= INT_MAX;
>  
> 
>  void av_max_alloc(size_t max){
> +    if (max < 32)
> +        max = SIZE_MAX - 32 + max; /* be compatible to older(< 4.3) versions */
>      max_alloc_size = max;
>  }
>  
>
Joakim Tjernlund Oct. 27, 2020, 8:48 a.m. UTC | #2
Yet a ping ...

This is a simple technical patch, it just needs a policy decision.
Can I have one ?

   Jocke

On Thu, 2020-10-22 at 14:17 +0200, Joakim Tjernlund wrote:
> Ping ?
> 
>  Jocke
> 
> On Fri, 2020-10-16 at 10:57 +0200, Joakim Tjernlund wrote:
> > From https://bugs.chromium.org/p/chromium/issues/detail?id=1095962
> > ----------------------------
> > This seems to be caused by the custom handling of "av_max_alloc(0)" in
> > Chromium's ffmpeg fork to mean unlimited (added in [1]).
> > 
> > Upstream ffmpeg doesn't treat 0 as a special value; versions before 4.3 seemingly worked
> > because 32 was subtracted from max_alloc_size (set to 0 by Chromium) resulting in an
> > integer underflow, making the effective limit be SIZE_MAX - 31.
> > 
> > Now that the above underflow doesn't happen, the tab just crashes. The upstream change
> > for no longer subtracting 32 from max_alloc_size was included in ffmpeg 4.3. [2]
> > 
> > [1] https://chromium-review.googlesource.com/c/chromium/third_party/ffmpeg/+/73563
> > [2] https://github.com/FFmpeg/FFmpeg/commit/731c77589841
> > ---------------------------
> > 
> > Restore av_malloc_max(0) to MAX_INT fixing MS Teams, Discord older chromium etc.
> > 
> > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > ---
> > 
> >  v2: Cover the full API range 0-31
> > 
> >  v3: Closer compat with < 4.3 ffmpeg
> > 
> >  v4: Adjust size accoriding to Andreas Rheinhardt comments
> > 
> >  libavutil/mem.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/libavutil/mem.c b/libavutil/mem.c
> > index cfb6d8a..44870a9 100644
> > --- a/libavutil/mem.c
> > +++ b/libavutil/mem.c
> > @@ -71,6 +71,8 @@ void  free(void *ptr);
> >  static size_t max_alloc_size= INT_MAX;
> >  
> > 
> >  void av_max_alloc(size_t max){
> > +    if (max < 32)
> > +        max = SIZE_MAX - 32 + max; /* be compatible to older(< 4.3) versions */
> >      max_alloc_size = max;
> >  }
> >  
> > 
>
Joakim Tjernlund Oct. 29, 2020, 1:46 p.m. UTC | #3
Ping ..

On Tue, 2020-10-27 at 08:48 +0000, Joakim Tjernlund wrote:
> 
> Yet a ping ...
> 
> This is a simple technical patch, it just needs a policy decision.
> Can I have one ?
> 
>    Jocke
> 
> On Thu, 2020-10-22 at 14:17 +0200, Joakim Tjernlund wrote:
> > Ping ?
> > 
> >  Jocke
> > 
> > On Fri, 2020-10-16 at 10:57 +0200, Joakim Tjernlund wrote:
> > > From https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.chromium.org%2Fp%2Fchromium%2Fissues%2Fdetail%3Fid%3D1095962&amp;data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C63237ba2b4e64972a69e08d87a550bf3%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637393852976888793%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=qIoMvpeiu5T4c6X32MRxKJI6%2BX4gOfBQeO7Cr%2FAih0o%3D&amp;reserved=0
> > > ----------------------------
> > > This seems to be caused by the custom handling of "av_max_alloc(0)" in
> > > Chromium's ffmpeg fork to mean unlimited (added in [1]).
> > > 
> > > Upstream ffmpeg doesn't treat 0 as a special value; versions before 4.3 seemingly worked
> > > because 32 was subtracted from max_alloc_size (set to 0 by Chromium) resulting in an
> > > integer underflow, making the effective limit be SIZE_MAX - 31.
> > > 
> > > Now that the above underflow doesn't happen, the tab just crashes. The upstream change
> > > for no longer subtracting 32 from max_alloc_size was included in ffmpeg 4.3. [2]
> > > 
> > > [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fchromium-review.googlesource.com%2Fc%2Fchromium%2Fthird_party%2Fffmpeg%2F%2B%2F73563&amp;data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C63237ba2b4e64972a69e08d87a550bf3%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637393852976888793%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=rQSlGjvmBZyttkhlKKvw%2FvQpPnAs1FQ6tlQk6AdIMwg%3D&amp;reserved=0
> > > [2] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FFFmpeg%2FFFmpeg%2Fcommit%2F731c77589841&amp;data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C63237ba2b4e64972a69e08d87a550bf3%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637393852976888793%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=g2PTcFB9KaFZFDDtzlABOV8lHjCG0nYAWUrgI2v%2BhPY%3D&amp;reserved=0
> > > ---------------------------
> > > 
> > > Restore av_malloc_max(0) to MAX_INT fixing MS Teams, Discord older chromium etc.
> > > 
> > > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > > ---
> > > 
> > >  v2: Cover the full API range 0-31
> > > 
> > >  v3: Closer compat with < 4.3 ffmpeg
> > > 
> > >  v4: Adjust size accoriding to Andreas Rheinhardt comments
> > > 
> > >  libavutil/mem.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/libavutil/mem.c b/libavutil/mem.c
> > > index cfb6d8a..44870a9 100644
> > > --- a/libavutil/mem.c
> > > +++ b/libavutil/mem.c
> > > @@ -71,6 +71,8 @@ void  free(void *ptr);
> > >  static size_t max_alloc_size= INT_MAX;
> > > 
> > > 
> > >  void av_max_alloc(size_t max){
> > > +    if (max < 32)
> > > +        max = SIZE_MAX - 32 + max; /* be compatible to older(< 4.3) versions */
> > >      max_alloc_size = max;
> > >  }
> > > 
> > > 
> > 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fffmpeg.org%2Fmailman%2Flistinfo%2Fffmpeg-devel&amp;data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C63237ba2b4e64972a69e08d87a550bf3%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637393852976888793%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=3O5tUmaULeUO8OXZtPbEfvEoBBNJuFsakvSoIPA9%2FnI%3D&amp;reserved=0
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Joakim Tjernlund Nov. 3, 2020, 11:39 a.m. UTC | #4
Pretty please ?

 Jocke

On Thu, 2020-10-29 at 13:46 +0000, Joakim Tjernlund wrote:
> 
> Ping ..
> 
> On Tue, 2020-10-27 at 08:48 +0000, Joakim Tjernlund wrote:
> > 
> > Yet a ping ...
> > 
> > This is a simple technical patch, it just needs a policy decision.
> > Can I have one ?
> > 
> >    Jocke
> > 
> > On Thu, 2020-10-22 at 14:17 +0200, Joakim Tjernlund wrote:
> > > Ping ?
> > > 
> > >  Jocke
> > > 
> > > On Fri, 2020-10-16 at 10:57 +0200, Joakim Tjernlund wrote:
> > > > From https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.chromium.org%2Fp%2Fchromium%2Fissues%2Fdetail%3Fid%3D1095962&amp;data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C099e7246a65a480ea5d808d87c111026%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637395760010458386%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=u%2Fr4rM4fQkwS%2Buj9D%2BF%2FJsxy1nUJBWRnrNQdn71kdhw%3D&amp;reserved=0
> > > > ----------------------------
> > > > This seems to be caused by the custom handling of "av_max_alloc(0)" in
> > > > Chromium's ffmpeg fork to mean unlimited (added in [1]).
> > > > 
> > > > Upstream ffmpeg doesn't treat 0 as a special value; versions before 4.3 seemingly worked
> > > > because 32 was subtracted from max_alloc_size (set to 0 by Chromium) resulting in an
> > > > integer underflow, making the effective limit be SIZE_MAX - 31.
> > > > 
> > > > Now that the above underflow doesn't happen, the tab just crashes. The upstream change
> > > > for no longer subtracting 32 from max_alloc_size was included in ffmpeg 4.3. [2]
> > > > 
> > > > [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fchromium-review.googlesource.com%2Fc%2Fchromium%2Fthird_party%2Fffmpeg%2F%2B%2F73563&amp;data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C099e7246a65a480ea5d808d87c111026%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637395760010458386%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=qLA%2Bda2ge5hh0AZx6xKfOBPMD3iDkfiJdiyij2fB6y0%3D&amp;reserved=0
> > > > [2] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FFFmpeg%2FFFmpeg%2Fcommit%2F731c77589841&amp;data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C099e7246a65a480ea5d808d87c111026%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637395760010458386%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=jLpWoZ%2BFPIMr8Sw56eyypQre65T9tJEzGLgRSquTUPI%3D&amp;reserved=0
> > > > ---------------------------
> > > > 
> > > > Restore av_malloc_max(0) to MAX_INT fixing MS Teams, Discord older chromium etc.
> > > > 
> > > > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > > > ---
> > > > 
> > > >  v2: Cover the full API range 0-31
> > > > 
> > > >  v3: Closer compat with < 4.3 ffmpeg
> > > > 
> > > >  v4: Adjust size accoriding to Andreas Rheinhardt comments
> > > > 
> > > >  libavutil/mem.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/libavutil/mem.c b/libavutil/mem.c
> > > > index cfb6d8a..44870a9 100644
> > > > --- a/libavutil/mem.c
> > > > +++ b/libavutil/mem.c
> > > > @@ -71,6 +71,8 @@ void  free(void *ptr);
> > > >  static size_t max_alloc_size= INT_MAX;
> > > > 
> > > > 
> > > >  void av_max_alloc(size_t max){
> > > > +    if (max < 32)
> > > > +        max = SIZE_MAX - 32 + max; /* be compatible to older(< 4.3) versions */
> > > >      max_alloc_size = max;
> > > >  }
> > > > 
> > > > 
> > > 
> >
Anton Khirnov Nov. 3, 2020, 12:12 p.m. UTC | #5
Quoting Joakim Tjernlund (2020-11-03 12:39:53)
> Pretty please ?

ok, would people who are strongly against this patch please raise their
hands.

Personally I'm ok with pushing this, even though it's ugly. Then again
the vey existence of av_malloc_max is ugly and it should be deprecated
IMO.
Timo Rothenpieler Nov. 3, 2020, 1:04 p.m. UTC | #6
Given the multitude of recent serious security issues in Chromium-Based 
Browsers, is this even still an issue?
Anything not up to date enough to have already been fixed has serious 
security issues and should be updated ASAP, which also fixes this issue 
in turn.

I'd rather see downstream users fix their stuff than introduce 
workarounds for broken downstreams into ffmpeg.
James Almer Nov. 3, 2020, 1:05 p.m. UTC | #7
On 11/3/2020 9:12 AM, Anton Khirnov wrote:
> Quoting Joakim Tjernlund (2020-11-03 12:39:53)
>> Pretty please ?
> 
> ok, would people who are strongly against this patch please raise their
> hands.

Would applying this to master now (backporting it to 4.3 too) and then
reverting it at the time of the major bump work? The idea here if i
understood correctly is to keep ABI compatibility with software that was
linked to <= 4.2 and can't be recompiled to replace av_malloc_max(0)
calls with av_malloc_max(SIZE_MAX), right?
Because requesting for example a limit of 31 resulting in av_malloc()
having practically no limit is pretty silly and should have never happened.

> Personally I'm ok with pushing this, even though it's ugly. Then again
> the vey existence of av_malloc_max is ugly and it should be deprecated
> IMO.

Is there any real world use examples of it (outside of 0/SIZE_MAX to
disable the default INT_MAX limit)? Maybe it's superfluous and can be
removed just fine.
Joakim Tjernlund Nov. 3, 2020, 1:19 p.m. UTC | #8
On Tue, 2020-11-03 at 10:05 -0300, James Almer wrote:
> 
> On 11/3/2020 9:12 AM, Anton Khirnov wrote:
> > Quoting Joakim Tjernlund (2020-11-03 12:39:53)
> > > Pretty please ?
> > 
> > ok, would people who are strongly against this patch please raise their
> > hands.
> 
> Would applying this to master now (backporting it to 4.3 too) and then
> reverting it at the time of the major bump work? The idea here if i
> understood correctly is to keep ABI compatibility with software that was
> linked to <= 4.2 and can't be recompiled to replace av_malloc_max(0)
> calls with av_malloc_max(SIZE_MAX), right?

Right.

> Because requesting for example a limit of 31 resulting in av_malloc()
> having practically no limit is pretty silly and should have never happened.

I first did the the patch for only special handling 0 but this list asked me to
be precise so I changed it.

> 
> > Personally I'm ok with pushing this, even though it's ugly. Then again
> > the vey existence of av_malloc_max is ugly and it should be deprecated
> > IMO.
> 
> Is there any real world use examples of it (outside of 0/SIZE_MAX to
> disable the default INT_MAX limit)? Maybe it's superfluous and can be
> removed just fine.
>
James Almer Nov. 3, 2020, 1:24 p.m. UTC | #9
On 11/3/2020 10:19 AM, Joakim Tjernlund wrote:
> On Tue, 2020-11-03 at 10:05 -0300, James Almer wrote:
>>
>> On 11/3/2020 9:12 AM, Anton Khirnov wrote:
>>> Quoting Joakim Tjernlund (2020-11-03 12:39:53)
>>>> Pretty please ?
>>>
>>> ok, would people who are strongly against this patch please raise their
>>> hands.
>>
>> Would applying this to master now (backporting it to 4.3 too) and then
>> reverting it at the time of the major bump work? The idea here if i
>> understood correctly is to keep ABI compatibility with software that was
>> linked to <= 4.2 and can't be recompiled to replace av_malloc_max(0)
>> calls with av_malloc_max(SIZE_MAX), right?
> 
> Right.
> 
>> Because requesting for example a limit of 31 resulting in av_malloc()
>> having practically no limit is pretty silly and should have never happened.
> 
> I first did the the patch for only special handling 0 but this list asked me to
> be precise so I changed it.

I know, I'm talking about ensuring it's removed again after a soname
bump precisely because it's absurd.
If we ultimately decide to make 4.3 ABI compatible with 4.2 as far as
av_malloc() goes, then it needs to handle 0-31 the same way.

> 
>>
>>> Personally I'm ok with pushing this, even though it's ugly. Then again
>>> the vey existence of av_malloc_max is ugly and it should be deprecated
>>> IMO.
>>
>> Is there any real world use examples of it (outside of 0/SIZE_MAX to
>> disable the default INT_MAX limit)? Maybe it's superfluous and can be
>> removed just fine.
>>
> 
> _______________________________________________
> 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".
>
Andreas Rheinhardt Nov. 3, 2020, 1:38 p.m. UTC | #10
Timo Rothenpieler:
> Given the multitude of recent serious security issues in Chromium-Based
> Browsers, is this even still an issue?
> Anything not up to date enough to have already been fixed has serious
> security issues and should be updated ASAP, which also fixes this issue
> in turn.
> 
> I'd rather see downstream users fix their stuff than introduce
> workarounds for broken downstreams into ffmpeg.
+1

- Andreas
Joakim Tjernlund Nov. 3, 2020, 1:57 p.m. UTC | #11
On Tue, 2020-11-03 at 14:38 +0100, Andreas Rheinhardt wrote:
> 
> Timo Rothenpieler:
> > Given the multitude of recent serious security issues in Chromium-Based
> > Browsers, is this even still an issue?
> > Anything not up to date enough to have already been fixed has serious
> > security issues and should be updated ASAP, which also fixes this issue
> > in turn.
> > 
> > I'd rather see downstream users fix their stuff than introduce
> > workarounds for broken downstreams into ffmpeg.
> +1

Chromium has fixed this issue(but not sure if released yet though). Then this needs to trickle into
elektron and finally into MS Teams(as an example). Will take time so it will be fixed at some point, not anytime soon though.

 Jocke
Andreas Rheinhardt Nov. 3, 2020, 2:34 p.m. UTC | #12
Joakim Tjernlund:
> On Tue, 2020-11-03 at 14:38 +0100, Andreas Rheinhardt wrote:
>>
>> Timo Rothenpieler:
>>> Given the multitude of recent serious security issues in Chromium-Based
>>> Browsers, is this even still an issue?
>>> Anything not up to date enough to have already been fixed has serious
>>> security issues and should be updated ASAP, which also fixes this issue
>>> in turn.
>>>
>>> I'd rather see downstream users fix their stuff than introduce
>>> workarounds for broken downstreams into ffmpeg.
>> +1
> 
> Chromium has fixed this issue(but not sure if released yet though). Then this needs to trickle into
> elektron and finally into MS Teams(as an example). Will take time so it will be fixed at some point, not anytime soon though.
> 
The issue was fixed on August 11:
https://chromium.googlesource.com/chromium/src/+/7f4c7ff6b0f0e74338c885b0d5e5ef80fed597c3
It is contained in releases > 86.0.4232.0.

- Andreas
Michael Niedermayer Nov. 4, 2020, 9:51 a.m. UTC | #13
On Tue, Nov 03, 2020 at 02:38:52PM +0100, Andreas Rheinhardt wrote:
> Timo Rothenpieler:
> > Given the multitude of recent serious security issues in Chromium-Based
> > Browsers, is this even still an issue?
> > Anything not up to date enough to have already been fixed has serious
> > security issues and should be updated ASAP, which also fixes this issue
> > in turn.
> > 
> > I'd rather see downstream users fix their stuff than introduce
> > workarounds for broken downstreams into ffmpeg.
> +1

I normally am in favor of helping downstreams but in this case
I think there is maybe some risk of adding code which could somehow
end up as part of an exploit.
Asking for a more restrictive limit should not disable the limit,
that feels a bit dangerous to me

thx

[...]
Joakim Tjernlund Nov. 4, 2020, 9:55 a.m. UTC | #14
On Wed, 2020-11-04 at 10:51 +0100, Michael Niedermayer wrote:
> 
> On Tue, Nov 03, 2020 at 02:38:52PM +0100, Andreas Rheinhardt wrote:
> > Timo Rothenpieler:
> > > Given the multitude of recent serious security issues in Chromium-Based
> > > Browsers, is this even still an issue?
> > > Anything not up to date enough to have already been fixed has serious
> > > security issues and should be updated ASAP, which also fixes this issue
> > > in turn.
> > > 
> > > I'd rather see downstream users fix their stuff than introduce
> > > workarounds for broken downstreams into ffmpeg.
> > +1
> 
> I normally am in favor of helping downstreams but in this case
> I think there is maybe some risk of adding code which could somehow
> end up as part of an exploit.
> Asking for a more restrictive limit should not disable the limit,
> that feels a bit dangerous to me

Not adding this forces apps to stay on known vulnerable ffmpeg

 Jocke
Timo Rothenpieler Nov. 4, 2020, 11:47 a.m. UTC | #15
On 04.11.2020 10:55, Joakim Tjernlund wrote:
> On Wed, 2020-11-04 at 10:51 +0100, Michael Niedermayer wrote:
>>
>> On Tue, Nov 03, 2020 at 02:38:52PM +0100, Andreas Rheinhardt wrote:
>>> Timo Rothenpieler:
>>>> Given the multitude of recent serious security issues in Chromium-Based
>>>> Browsers, is this even still an issue?
>>>> Anything not up to date enough to have already been fixed has serious
>>>> security issues and should be updated ASAP, which also fixes this issue
>>>> in turn.
>>>>
>>>> I'd rather see downstream users fix their stuff than introduce
>>>> workarounds for broken downstreams into ffmpeg.
>>> +1
>>
>> I normally am in favor of helping downstreams but in this case
>> I think there is maybe some risk of adding code which could somehow
>> end up as part of an exploit.
>> Asking for a more restrictive limit should not disable the limit,
>> that feels a bit dangerous to me
> 
> Not adding this forces apps to stay on known vulnerable ffmpeg

No it doesn't. It forces them to upgrade away from a known vulnerable 
old Chromium version to one that does not have the issue.
Joakim Tjernlund Nov. 4, 2020, 2:08 p.m. UTC | #16
On Wed, 2020-11-04 at 12:47 +0100, Timo Rothenpieler wrote:
> 
> On 04.11.2020 10:55, Joakim Tjernlund wrote:
> > On Wed, 2020-11-04 at 10:51 +0100, Michael Niedermayer wrote:
> > > 
> > > On Tue, Nov 03, 2020 at 02:38:52PM +0100, Andreas Rheinhardt wrote:
> > > > Timo Rothenpieler:
> > > > > Given the multitude of recent serious security issues in Chromium-Based
> > > > > Browsers, is this even still an issue?
> > > > > Anything not up to date enough to have already been fixed has serious
> > > > > security issues and should be updated ASAP, which also fixes this issue
> > > > > in turn.
> > > > > 
> > > > > I'd rather see downstream users fix their stuff than introduce
> > > > > workarounds for broken downstreams into ffmpeg.
> > > > +1
> > > 
> > > I normally am in favor of helping downstreams but in this case
> > > I think there is maybe some risk of adding code which could somehow
> > > end up as part of an exploit.
> > > Asking for a more restrictive limit should not disable the limit,
> > > that feels a bit dangerous to me
> > 
> > Not adding this forces apps to stay on known vulnerable ffmpeg
> 
> No it doesn't. It forces them to upgrade away from a known vulnerable
> old Chromium version to one that does not have the issue.

I was referring to what is out/released now. Eventually all SW will upgrade for one reason or another.

 Jocke
Ronald S. Bultje Nov. 4, 2020, 6:38 p.m. UTC | #17
Hi Joakim,

On Tue, Nov 3, 2020 at 8:57 AM Joakim Tjernlund <
Joakim.Tjernlund@infinera.com> wrote:

> On Tue, 2020-11-03 at 14:38 +0100, Andreas Rheinhardt wrote:
> >
> > Timo Rothenpieler:
> > > Given the multitude of recent serious security issues in Chromium-Based
> > > Browsers, is this even still an issue?
> > > Anything not up to date enough to have already been fixed has serious
> > > security issues and should be updated ASAP, which also fixes this issue
> > > in turn.
> > >
> > > I'd rather see downstream users fix their stuff than introduce
> > > workarounds for broken downstreams into ffmpeg.
> > +1
>
> Chromium has fixed this issue(but not sure if released yet though). Then
> this needs to trickle into
> elektron and finally into MS Teams(as an example). Will take time so it
> will be fixed at some point, not anytime soon though.


For clarity, could you explain how releasing this fix would not have to go
through the same release funnel in Chrome? Or is the Chrome dependency in
elektron/teams not the same as the FFmpeg dependency?

Ronald
Joakim Tjernlund Nov. 4, 2020, 9:01 p.m. UTC | #18
On Wed, 2020-11-04 at 13:38 -0500, Ronald S. Bultje wrote:
> 
> Hi Joakim,
> 
> On Tue, Nov 3, 2020 at 8:57 AM Joakim Tjernlund <
> Joakim.Tjernlund@infinera.com> wrote:
> 
> > On Tue, 2020-11-03 at 14:38 +0100, Andreas Rheinhardt wrote:
> > > 
> > > Timo Rothenpieler:
> > > > Given the multitude of recent serious security issues in Chromium-Based
> > > > Browsers, is this even still an issue?
> > > > Anything not up to date enough to have already been fixed has serious
> > > > security issues and should be updated ASAP, which also fixes this issue
> > > > in turn.
> > > > 
> > > > I'd rather see downstream users fix their stuff than introduce
> > > > workarounds for broken downstreams into ffmpeg.
> > > +1
> > 
> > Chromium has fixed this issue(but not sure if released yet though). Then
> > this needs to trickle into
> > elektron and finally into MS Teams(as an example). Will take time so it
> > will be fixed at some point, not anytime soon though.
> 
> 
> For clarity, could you explain how releasing this fix would not have to go
> through the same release funnel in Chrome? Or is the Chrome dependency in
> elektron/teams not the same as the FFmpeg dependency?

I don't understand what you are asking for...

  Jocke
Moritz Barsnick Nov. 6, 2020, 2:05 p.m. UTC | #19
On Wed, Nov 04, 2020 at 14:08:23 +0000, Joakim Tjernlund wrote:
> > No it doesn't. It forces them to upgrade away from a known vulnerable
> > old Chromium version to one that does not have the issue.
>
> I was referring to what is out/released now. Eventually all SW will upgrade for one reason or another.

Are you saying rolled out (or released) applications can update the
bundled ffmpeg (I guess libffmpeg which is part of Chromium) to include
this fix, but not update the bundled Chromium? Would updates not switch
to version 86 because 85 is discontinued?

Wondering,
Moritz
Joakim Tjernlund Nov. 6, 2020, 4:10 p.m. UTC | #20
On Fri, 2020-11-06 at 15:05 +0100, Moritz Barsnick wrote:
> 
> On Wed, Nov 04, 2020 at 14:08:23 +0000, Joakim Tjernlund wrote:
> > > No it doesn't. It forces them to upgrade away from a known vulnerable
> > > old Chromium version to one that does not have the issue.
> > 
> > I was referring to what is out/released now. Eventually all SW will upgrade for one reason or another.
> 
> Are you saying rolled out (or released) applications can update the
> bundled ffmpeg (I guess libffmpeg which is part of Chromium) to include
> this fix, but not update the bundled Chromium? Would updates not switch
> to version 86 because 85 is discontinued?

We unbundle the ffmpeg/mesa and use system mesa/ffmpeg, god knows what versions they ship but they are older.
At some point apps will update and won't need this hack, I have no control over when.

Anyhow, ffmpeg breaks the ABI(by accident) and it breaks some apps.
Question is, do you care or not?

If yes, this patch (or reverting the cleanup patch that broke ABI) is needed.
I don't want to discuss this matter forever, I just want a decision now.

 Jocke
Ronald S. Bultje Nov. 7, 2020, 6:19 p.m. UTC | #21
Hi,

On Fri, Nov 6, 2020 at 11:10 AM Joakim Tjernlund <
Joakim.Tjernlund@infinera.com> wrote:

> We unbundle the ffmpeg/mesa and use system mesa/ffmpeg


That's the answer to my earlier question also, thank you. I'm fine with
applying this, I would suggest that the "new" (correct) behaviour should
live under a major update or under a different function name, but I'll
leave it to others to make a final decision there...

Ronald
Michael Niedermayer Nov. 8, 2020, 10:07 a.m. UTC | #22
On Fri, Oct 16, 2020 at 10:57:22AM +0200, Joakim Tjernlund wrote:
> From https://bugs.chromium.org/p/chromium/issues/detail?id=1095962
> ----------------------------
> This seems to be caused by the custom handling of "av_max_alloc(0)" in
> Chromium's ffmpeg fork to mean unlimited (added in [1]).
> 
> Upstream ffmpeg doesn't treat 0 as a special value; versions before 4.3 seemingly worked
> because 32 was subtracted from max_alloc_size (set to 0 by Chromium) resulting in an
> integer underflow, making the effective limit be SIZE_MAX - 31.
> 
> Now that the above underflow doesn't happen, the tab just crashes. The upstream change
> for no longer subtracting 32 from max_alloc_size was included in ffmpeg 4.3. [2]
> 
> [1] https://chromium-review.googlesource.com/c/chromium/third_party/ffmpeg/+/73563
> [2] https://github.com/FFmpeg/FFmpeg/commit/731c77589841
> ---------------------------
> 
> Restore av_malloc_max(0) to MAX_INT fixing MS Teams, Discord older chromium etc.
> 
> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> ---
> 
>  v2: Cover the full API range 0-31
> 
>  v3: Closer compat with < 4.3 ffmpeg
> 
>  v4: Adjust size accoriding to Andreas Rheinhardt comments
> 
>  libavutil/mem.c | 2 ++
>  1 file changed, 2 insertions(+)

"Unbreak av_malloc_max(0) API/ABI"
The commit message of this is incorrect

The API before and after this documented the current git master behavior
more correct would be
"Break API of av_malloc_max() for 0-31, to restore ABI behavior so applications using this do not need to be changed"

Also id like to raise awareness that this function had a big warning:
    *
    * @warning Exercise extreme caution when using this function. Don't touch
    *          this if you do not understand the full consequence of doing so.
    */
    void av_max_alloc(size_t max);

And also id like to raise awareness that the default limit is INT_MAX
while with 0 as argument it becomes SIZE_MAX. I would expect that is
not safe everywhere and could open security issues.
If anything 0 should be interpreted as the default INT_MAX
If its not obvious where SIZE_MAX can be an issue, consider what we
use to index arrays, int, and that doesnt go to SIZE_MAX but instead
hits undefined behavior maybe becomes negative and accesses out of array

thx


[...]
Andreas Rheinhardt Nov. 8, 2020, 10:35 a.m. UTC | #23
Michael Niedermayer:
> On Fri, Oct 16, 2020 at 10:57:22AM +0200, Joakim Tjernlund wrote:
>> From https://bugs.chromium.org/p/chromium/issues/detail?id=1095962
>> ----------------------------
>> This seems to be caused by the custom handling of "av_max_alloc(0)" in
>> Chromium's ffmpeg fork to mean unlimited (added in [1]).
>>
>> Upstream ffmpeg doesn't treat 0 as a special value; versions before 4.3 seemingly worked
>> because 32 was subtracted from max_alloc_size (set to 0 by Chromium) resulting in an
>> integer underflow, making the effective limit be SIZE_MAX - 31.
>>
>> Now that the above underflow doesn't happen, the tab just crashes. The upstream change
>> for no longer subtracting 32 from max_alloc_size was included in ffmpeg 4.3. [2]
>>
>> [1] https://chromium-review.googlesource.com/c/chromium/third_party/ffmpeg/+/73563
>> [2] https://github.com/FFmpeg/FFmpeg/commit/731c77589841
>> ---------------------------
>>
>> Restore av_malloc_max(0) to MAX_INT fixing MS Teams, Discord older chromium etc.
>>
>> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
>> ---
>>
>>  v2: Cover the full API range 0-31
>>
>>  v3: Closer compat with < 4.3 ffmpeg
>>
>>  v4: Adjust size accoriding to Andreas Rheinhardt comments
>>
>>  libavutil/mem.c | 2 ++
>>  1 file changed, 2 insertions(+)
> 
> "Unbreak av_malloc_max(0) API/ABI"
> The commit message of this is incorrect
> 
> The API before and after this documented the current git master behavior
> more correct would be
> "Break API of av_malloc_max() for 0-31, to restore ABI behavior so applications using this do not need to be changed"
> 

I agree.

> Also id like to raise awareness that this function had a big warning:
>     *
>     * @warning Exercise extreme caution when using this function. Don't touch
>     *          this if you do not understand the full consequence of doing so.
>     */
>     void av_max_alloc(size_t max);
> 
> And also id like to raise awareness that the default limit is INT_MAX
> while with 0 as argument it becomes SIZE_MAX. 

No: With the old version 0 was effectively SIZE_MAX - 31, with current
git head 0 means that all allocations of > 0 fail (yet requests of size
0 still result in an allocation of size 1); with the proposed version
av_max_alloc(0) would set the limit to SIZE_MAX - 32. This off-by-one is
surely unintentional.

I would expect that is
> not safe everywhere and could open security issues.
> If anything 0 should be interpreted as the default INT_MAX

That would be an API break.

> If its not obvious where SIZE_MAX can be an issue, consider what we
> use to index arrays, int, and that doesnt go to SIZE_MAX but instead
> hits undefined behavior maybe becomes negative and accesses out of array

Any code that relies on allocations > INT_MAX to fail is buggy and must
be fixed; and so is any code that uses an index parameter of type int
and uses a comparison with a value of type size_t as its loop condition.

- Andreas

*: Btw: AVFormatContext.nb_streams is unsigned, yet it is common to use
an int to loop over the streams. This is not dangerous now, as the
max_streams option is limited to INT_MAX. But we should probably change
this habit.
Michael Niedermayer Nov. 8, 2020, 2:13 p.m. UTC | #24
On Sun, Nov 08, 2020 at 11:35:43AM +0100, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > On Fri, Oct 16, 2020 at 10:57:22AM +0200, Joakim Tjernlund wrote:
> >> From https://bugs.chromium.org/p/chromium/issues/detail?id=1095962
> >> ----------------------------
> >> This seems to be caused by the custom handling of "av_max_alloc(0)" in
> >> Chromium's ffmpeg fork to mean unlimited (added in [1]).
> >>
> >> Upstream ffmpeg doesn't treat 0 as a special value; versions before 4.3 seemingly worked
> >> because 32 was subtracted from max_alloc_size (set to 0 by Chromium) resulting in an
> >> integer underflow, making the effective limit be SIZE_MAX - 31.
> >>
> >> Now that the above underflow doesn't happen, the tab just crashes. The upstream change
> >> for no longer subtracting 32 from max_alloc_size was included in ffmpeg 4.3. [2]
> >>
> >> [1] https://chromium-review.googlesource.com/c/chromium/third_party/ffmpeg/+/73563
> >> [2] https://github.com/FFmpeg/FFmpeg/commit/731c77589841
> >> ---------------------------
> >>
> >> Restore av_malloc_max(0) to MAX_INT fixing MS Teams, Discord older chromium etc.
> >>
> >> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> >> ---
> >>
> >>  v2: Cover the full API range 0-31
> >>
> >>  v3: Closer compat with < 4.3 ffmpeg
> >>
> >>  v4: Adjust size accoriding to Andreas Rheinhardt comments
> >>
> >>  libavutil/mem.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> > 
> > "Unbreak av_malloc_max(0) API/ABI"
> > The commit message of this is incorrect
> > 
> > The API before and after this documented the current git master behavior
> > more correct would be
> > "Break API of av_malloc_max() for 0-31, to restore ABI behavior so applications using this do not need to be changed"
> > 
> 
> I agree.
> 
> > Also id like to raise awareness that this function had a big warning:
> >     *
> >     * @warning Exercise extreme caution when using this function. Don't touch
> >     *          this if you do not understand the full consequence of doing so.
> >     */
> >     void av_max_alloc(size_t max);
> > 
> > And also id like to raise awareness that the default limit is INT_MAX
> > while with 0 as argument it becomes SIZE_MAX. 
> 
> No: With the old version 0 was effectively SIZE_MAX - 31, with current
> git head 0 means that all allocations of > 0 fail (yet requests of size
> 0 still result in an allocation of size 1); with the proposed version
> av_max_alloc(0) would set the limit to SIZE_MAX - 32. This off-by-one is
> surely unintentional.
> 

> I would expect that is
> > not safe everywhere and could open security issues.
> > If anything 0 should be interpreted as the default INT_MAX
> 
> That would be an API break.

yes, that was meant as a "less bad" alternative to SIZE_MAX - 31


> 
> > If its not obvious where SIZE_MAX can be an issue, consider what we
> > use to index arrays, int, and that doesnt go to SIZE_MAX but instead
> > hits undefined behavior maybe becomes negative and accesses out of array
> 
> Any code that relies on allocations > INT_MAX to fail is buggy and must
> be fixed;

YES


> and so is any code that uses an index parameter of type int
> and uses a comparison with a value of type size_t as its loop condition.

YES
though there are more complex failure cases
for example a
array[i + j*C]
here the 2 loops for i and j can have int limits and int indexes and still
if the array is not limited to INT_MAX this could go bad


> 
> - Andreas
> 
> *: Btw: AVFormatContext.nb_streams is unsigned, yet it is common to use
> an int to loop over the streams. This is not dangerous now, as the
> max_streams option is limited to INT_MAX. But we should probably change
> this habit.

yes

thx

[...]
Joakim Tjernlund Nov. 9, 2020, 10:53 p.m. UTC | #25
On Sun, 2020-11-08 at 15:13 +0100, Michael Niedermayer wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> On Sun, Nov 08, 2020 at 11:35:43AM +0100, Andreas Rheinhardt wrote:
> > Michael Niedermayer:
> > > On Fri, Oct 16, 2020 at 10:57:22AM +0200, Joakim Tjernlund wrote:
> > > > From https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.chromium.org%2Fp%2Fchromium%2Fissues%2Fdetail%3Fid%3D1095962&amp;data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C0c52cb182fa941b874c908d883f07c2c%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637404416183627186%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=7wEFER5KUD1KEnTJ7mr2xnfmOzctpDsb0Bld%2BfAvjAc%3D&amp;reserved=0
> > > > ----------------------------
> > > > This seems to be caused by the custom handling of "av_max_alloc(0)" in
> > > > Chromium's ffmpeg fork to mean unlimited (added in [1]).
> > > > 
> > > > Upstream ffmpeg doesn't treat 0 as a special value; versions before 4.3 seemingly worked
> > > > because 32 was subtracted from max_alloc_size (set to 0 by Chromium) resulting in an
> > > > integer underflow, making the effective limit be SIZE_MAX - 31.
> > > > 
> > > > Now that the above underflow doesn't happen, the tab just crashes. The upstream change
> > > > for no longer subtracting 32 from max_alloc_size was included in ffmpeg 4.3. [2]
> > > > 
> > > > [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fchromium-review.googlesource.com%2Fc%2Fchromium%2Fthird_party%2Fffmpeg%2F%2B%2F73563&amp;data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C0c52cb182fa941b874c908d883f07c2c%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637404416183627186%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=s%2BnujuFPv6QwNlIKjuXQNcDR3wvA%2BY7Uu4Qnr%2FsHjnE%3D&amp;reserved=0
> > > > [2] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FFFmpeg%2FFFmpeg%2Fcommit%2F731c77589841&amp;data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C0c52cb182fa941b874c908d883f07c2c%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637404416183637145%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=ftg9ysBzC5YD9uEe%2BgJwZIecW8a6fAXXNS9kGqDMnp8%3D&amp;reserved=0
> > > > ---------------------------
> > > > 
> > > > Restore av_malloc_max(0) to MAX_INT fixing MS Teams, Discord older chromium etc.
> > > > 
> > > > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > > > ---
> > > > 
> > > >  v2: Cover the full API range 0-31
> > > > 
> > > >  v3: Closer compat with < 4.3 ffmpeg
> > > > 
> > > >  v4: Adjust size accoriding to Andreas Rheinhardt comments
> > > > 
> > > >  libavutil/mem.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > 
> > > "Unbreak av_malloc_max(0) API/ABI"
> > > The commit message of this is incorrect
> > > 
> > > The API before and after this documented the current git master behavior
> > > more correct would be
> > > "Break API of av_malloc_max() for 0-31, to restore ABI behavior so applications using this do not need to be changed"
> > > 
> > 
> > I agree.
> > 
> > > Also id like to raise awareness that this function had a big warning:
> > >     *
> > >     * @warning Exercise extreme caution when using this function. Don't touch
> > >     *          this if you do not understand the full consequence of doing so.
> > >     */
> > >     void av_max_alloc(size_t max);
> > > 
> > > And also id like to raise awareness that the default limit is INT_MAX
> > > while with 0 as argument it becomes SIZE_MAX.
> > 
> > No: With the old version 0 was effectively SIZE_MAX - 31, with current
> > git head 0 means that all allocations of > 0 fail (yet requests of size
> > 0 still result in an allocation of size 1); with the proposed version
> > av_max_alloc(0) would set the limit to SIZE_MAX - 32. This off-by-one is
> > surely unintentional.
> > 
> 
> > I would expect that is
> > > not safe everywhere and could open security issues.
> > > If anything 0 should be interpreted as the default INT_MAX
> > 
> > That would be an API break.
> 
> yes, that was meant as a "less bad" alternative to SIZE_MAX - 31
> 
> 
> > 
> > > If its not obvious where SIZE_MAX can be an issue, consider what we
> > > use to index arrays, int, and that doesnt go to SIZE_MAX but instead
> > > hits undefined behavior maybe becomes negative and accesses out of array
> > 
> > Any code that relies on allocations > INT_MAX to fail is buggy and must
> > be fixed;
> 
> YES
> 
> 
> > and so is any code that uses an index parameter of type int
> > and uses a comparison with a value of type size_t as its loop condition.
> 
> YES
> though there are more complex failure cases
> for example a
> array[i + j*C]
> here the 2 loops for i and j can have int limits and int indexes and still
> if the array is not limited to INT_MAX this could go bad
> 
> 
> > 
> > - Andreas
> > 
> > *: Btw: AVFormatContext.nb_streams is unsigned, yet it is common to use
> > an int to loop over the streams. This is not dangerous now, as the
> > max_streams option is limited to INT_MAX. But we should probably change
> > this habit.
> 
> yes
> 
> thx

Did you reach a conclusion ? Go back to my original patch ?

     Jocke
Joakim Tjernlund Nov. 12, 2020, 7:08 p.m. UTC | #26
On Mon, 2020-11-09 at 22:53 +0000, Joakim Tjernlund wrote:
> On Sun, 2020-11-08 at 15:13 +0100, Michael Niedermayer wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > 
> > 
> > On Sun, Nov 08, 2020 at 11:35:43AM +0100, Andreas Rheinhardt wrote:
> > > Michael Niedermayer:
> > > > On Fri, Oct 16, 2020 at 10:57:22AM +0200, Joakim Tjernlund wrote:
> > > > > From https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.chromium.org%2Fp%2Fchromium%2Fissues%2Fdetail%3Fid%3D1095962&amp;data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C9d88692519584c6d00e908d8850257f0%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637405592400704104%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=WhXCZX6Cne5U7E7GvCWpL%2FjV2Z80sqU1nPe81gk724A%3D&amp;reserved=0
> > > > > ----------------------------
> > > > > This seems to be caused by the custom handling of "av_max_alloc(0)" in
> > > > > Chromium's ffmpeg fork to mean unlimited (added in [1]).
> > > > > 
> > > > > Upstream ffmpeg doesn't treat 0 as a special value; versions before 4.3 seemingly worked
> > > > > because 32 was subtracted from max_alloc_size (set to 0 by Chromium) resulting in an
> > > > > integer underflow, making the effective limit be SIZE_MAX - 31.
> > > > > 
> > > > > Now that the above underflow doesn't happen, the tab just crashes. The upstream change
> > > > > for no longer subtracting 32 from max_alloc_size was included in ffmpeg 4.3. [2]
> > > > > 
> > > > > [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fchromium-review.googlesource.com%2Fc%2Fchromium%2Fthird_party%2Fffmpeg%2F%2B%2F73563&amp;data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C9d88692519584c6d00e908d8850257f0%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637405592400704104%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=5oUMLsjhJWGduxmQTEdD33BuJwX1H7oCaMUKVnf5L%2Bw%3D&amp;reserved=0
> > > > > [2] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FFFmpeg%2FFFmpeg%2Fcommit%2F731c77589841&amp;data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C9d88692519584c6d00e908d8850257f0%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637405592400714100%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=eAbvDHG8ivjArQOQSCjS7KO%2B2nqIqBa2vl9JV76uLM0%3D&amp;reserved=0
> > > > > ---------------------------
> > > > > 
> > > > > Restore av_malloc_max(0) to MAX_INT fixing MS Teams, Discord older chromium etc.
> > > > > 
> > > > > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > > > > ---
> > > > > 
> > > > >  v2: Cover the full API range 0-31
> > > > > 
> > > > >  v3: Closer compat with < 4.3 ffmpeg
> > > > > 
> > > > >  v4: Adjust size accoriding to Andreas Rheinhardt comments
> > > > > 
> > > > >  libavutil/mem.c | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > "Unbreak av_malloc_max(0) API/ABI"
> > > > The commit message of this is incorrect
> > > > 
> > > > The API before and after this documented the current git master behavior
> > > > more correct would be
> > > > "Break API of av_malloc_max() for 0-31, to restore ABI behavior so applications using this do not need to be changed"
> > > > 
> > > 
> > > I agree.
> > > 
> > > > Also id like to raise awareness that this function had a big warning:
> > > >     *
> > > >     * @warning Exercise extreme caution when using this function. Don't touch
> > > >     *          this if you do not understand the full consequence of doing so.
> > > >     */
> > > >     void av_max_alloc(size_t max);
> > > > 
> > > > And also id like to raise awareness that the default limit is INT_MAX
> > > > while with 0 as argument it becomes SIZE_MAX.
> > > 
> > > No: With the old version 0 was effectively SIZE_MAX - 31, with current
> > > git head 0 means that all allocations of > 0 fail (yet requests of size
> > > 0 still result in an allocation of size 1); with the proposed version
> > > av_max_alloc(0) would set the limit to SIZE_MAX - 32. This off-by-one is
> > > surely unintentional.
> > > 
> > 
> > > I would expect that is
> > > > not safe everywhere and could open security issues.
> > > > If anything 0 should be interpreted as the default INT_MAX
> > > 
> > > That would be an API break.
> > 
> > yes, that was meant as a "less bad" alternative to SIZE_MAX - 31
> > 
> > 
> > > 
> > > > If its not obvious where SIZE_MAX can be an issue, consider what we
> > > > use to index arrays, int, and that doesnt go to SIZE_MAX but instead
> > > > hits undefined behavior maybe becomes negative and accesses out of array
> > > 
> > > Any code that relies on allocations > INT_MAX to fail is buggy and must
> > > be fixed;
> > 
> > YES
> > 
> > 
> > > and so is any code that uses an index parameter of type int
> > > and uses a comparison with a value of type size_t as its loop condition.
> > 
> > YES
> > though there are more complex failure cases
> > for example a
> > array[i + j*C]
> > here the 2 loops for i and j can have int limits and int indexes and still
> > if the array is not limited to INT_MAX this could go bad
> > 
> > 
> > > 
> > > - Andreas
> > > 
> > > *: Btw: AVFormatContext.nb_streams is unsigned, yet it is common to use
> > > an int to loop over the streams. This is not dangerous now, as the
> > > max_streams option is limited to INT_MAX. But we should probably change
> > > this habit.
> > 
> > yes
> > 
> > thx
> 
> Did you reach a conclusion ? Go back to my original patch ?
> 

Ping ? I need to get off this list as I cannot handle the mail volume.

 Jocke
diff mbox series

Patch

diff --git a/libavutil/mem.c b/libavutil/mem.c
index cfb6d8a..44870a9 100644
--- a/libavutil/mem.c
+++ b/libavutil/mem.c
@@ -71,6 +71,8 @@  void  free(void *ptr);
 static size_t max_alloc_size= INT_MAX;
 
 void av_max_alloc(size_t max){
+    if (max < 32)
+        max = SIZE_MAX - 32 + max; /* be compatible to older(< 4.3) versions */
     max_alloc_size = max;
 }