| Message ID | 20201016085722.21866-1-joakim.tjernlund@infinera.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v4] Unbreak av_malloc_max(0) API/ABI | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | warning | Make failed |
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; > } > >
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; > > } > > > > >
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&data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C63237ba2b4e64972a69e08d87a550bf3%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637393852976888793%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=qIoMvpeiu5T4c6X32MRxKJI6%2BX4gOfBQeO7Cr%2FAih0o%3D&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&data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C63237ba2b4e64972a69e08d87a550bf3%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637393852976888793%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=rQSlGjvmBZyttkhlKKvw%2FvQpPnAs1FQ6tlQk6AdIMwg%3D&reserved=0 > > > [2] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FFFmpeg%2FFFmpeg%2Fcommit%2F731c77589841&data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C63237ba2b4e64972a69e08d87a550bf3%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637393852976888793%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=g2PTcFB9KaFZFDDtzlABOV8lHjCG0nYAWUrgI2v%2BhPY%3D&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&data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C63237ba2b4e64972a69e08d87a550bf3%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637393852976888793%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=3O5tUmaULeUO8OXZtPbEfvEoBBNJuFsakvSoIPA9%2FnI%3D&reserved=0 > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
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&data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C099e7246a65a480ea5d808d87c111026%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637395760010458386%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=u%2Fr4rM4fQkwS%2Buj9D%2BF%2FJsxy1nUJBWRnrNQdn71kdhw%3D&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&data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C099e7246a65a480ea5d808d87c111026%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637395760010458386%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=qLA%2Bda2ge5hh0AZx6xKfOBPMD3iDkfiJdiyij2fB6y0%3D&reserved=0 > > > > [2] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FFFmpeg%2FFFmpeg%2Fcommit%2F731c77589841&data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C099e7246a65a480ea5d808d87c111026%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637395760010458386%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=jLpWoZ%2BFPIMr8Sw56eyypQre65T9tJEzGLgRSquTUPI%3D&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; > > > > } > > > > > > > > > > > > >
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.
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.
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.
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. >
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". >
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
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
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
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 [...]
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
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.
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
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
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
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
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
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
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 [...]
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.
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 [...]
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&data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C0c52cb182fa941b874c908d883f07c2c%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637404416183627186%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=7wEFER5KUD1KEnTJ7mr2xnfmOzctpDsb0Bld%2BfAvjAc%3D&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&data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C0c52cb182fa941b874c908d883f07c2c%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637404416183627186%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=s%2BnujuFPv6QwNlIKjuXQNcDR3wvA%2BY7Uu4Qnr%2FsHjnE%3D&reserved=0 > > > > [2] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FFFmpeg%2FFFmpeg%2Fcommit%2F731c77589841&data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C0c52cb182fa941b874c908d883f07c2c%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637404416183637145%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=ftg9ysBzC5YD9uEe%2BgJwZIecW8a6fAXXNS9kGqDMnp8%3D&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
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&data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C9d88692519584c6d00e908d8850257f0%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637405592400704104%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=WhXCZX6Cne5U7E7GvCWpL%2FjV2Z80sqU1nPe81gk724A%3D&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&data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C9d88692519584c6d00e908d8850257f0%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637405592400704104%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=5oUMLsjhJWGduxmQTEdD33BuJwX1H7oCaMUKVnf5L%2Bw%3D&reserved=0 > > > > > [2] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FFFmpeg%2FFFmpeg%2Fcommit%2F731c77589841&data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C9d88692519584c6d00e908d8850257f0%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637405592400714100%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=eAbvDHG8ivjArQOQSCjS7KO%2B2nqIqBa2vl9JV76uLM0%3D&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 --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; }
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(+)