| Message ID | 20201013102942.24007-1-joakim.tjernlund@infinera.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel] 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 |
Hi, On Tue, Oct 13, 2020 at 6:30 AM Joakim Tjernlund < joakim.tjernlund@infinera.com> 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> > --- > libavutil/mem.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/libavutil/mem.c b/libavutil/mem.c > index cfb6d8a..52c2b1b 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) > + max = INT_MAX; /* be compatible to older(< 4.3) versions */ > max_alloc_size = max; > } > > -- > 2.26.2 I think this is the wrong "unfix". The previous inversion of inverted behaviour (b/c of the integer underflow) in the range [0,31] is now maintained for [1,31], but not for 0. I think that we can't "silently" change behaviour for functions like this. As ugly as it is, the classic way of adding a new function (av_max_malloc2()) and deprecating the old one is much better, because people have to explicitly agree to the new function by updating their API usage. Just MHO. Ronald
On Tue, 2020-10-13 at 09:41 -0400, Ronald S. Bultje 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. > > Hi, > > On Tue, Oct 13, 2020 at 6:30 AM Joakim Tjernlund <joakim.tjernlund@infinera.com> 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> > > --- > > libavutil/mem.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/libavutil/mem.c b/libavutil/mem.c > > index cfb6d8a..52c2b1b 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) > > + max = INT_MAX; /* be compatible to older(< 4.3) versions */ > > max_alloc_size = max; > > } > > > I think this is the wrong "unfix". The previous inversion of inverted behaviour (b/c of the integer underflow) in the range [0,31] is now maintained for [1,31], but not for 0. Yes, if you want to be really picky but I doubt there are any real apps using 1-31 though. > > I think that we can't "silently" change behaviour for functions like this. As ugly as it is, the classic way of adding a new function (av_max_malloc2()) and deprecating the old > one is much better, because people have to explicitly agree to the new function by updating their API usage. For now just fixing av_malloc_max(0) will do, av_max_malloc2() etc. is beyond this patch. > > Just MHO. > > Ronald
On 13/10/2020 15:19, Joakim Tjernlund wrote:
> For now just fixing av_malloc_max(0) will do, av_max_malloc2() etc. is beyond this patch.
As Ronald mentioned, if you're going to fix the ABI/API, you should actually
fix every use of that ABI/API, not just the one you care about (0).
So max values of 1-31 should also be handled.
- Derek
On 10/13/2020 11:24 AM, Derek Buitenhuis wrote: > On 13/10/2020 15:19, Joakim Tjernlund wrote: >> For now just fixing av_malloc_max(0) will do, av_max_malloc2() etc. is beyond this patch. > > As Ronald mentioned, if you're going to fix the ABI/API, you should actually > fix every use of that ABI/API, not just the one you care about (0). > > So max values of 1-31 should also be handled. I personally don't think there's anything to fix, or if there is, it's not this. For starters av_max_alloc() was never changed. It was how av_malloc() handled the max_alloc_size variable. And then, the old behavior was both buggy (0 to 31 equaling SIZE_MAX or slightly smaller) and undocumented. I could accept reverting it for the 4.3 branch for the sake of ABI compatibility, but after a sobump 0-31 should not wrap around or be replaced by something arbitrary like INT_MAX. Chromium already fixed their code by requesting a SIZE_MAX limit (meaning effectively unlimited), which is what they wanted to begin with, and others should follow. > > - Derek > _______________________________________________ > 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". >
On Tue, 2020-10-13 at 15:24 +0100, Derek Buitenhuis wrote: > > On 13/10/2020 15:19, Joakim Tjernlund wrote: > > For now just fixing av_malloc_max(0) will do, av_max_malloc2() etc. is beyond this patch. > > As Ronald mentioned, if you're going to fix the ABI/API, you should actually > fix every use of that ABI/API, not just the one you care about (0). > > So max values of 1-31 should also be handled. OK, how far do you want to take this, will this do? if (max < 32) max = INT_MAX; Complete API fix would need to revert the patch that caused the break to begin with and I don't think you want that ? Jocke
On Tue, 2020-10-13 at 14:03 -0300, James Almer wrote: > > > On 10/13/2020 11:24 AM, Derek Buitenhuis wrote: > > On 13/10/2020 15:19, Joakim Tjernlund wrote: > > > For now just fixing av_malloc_max(0) will do, av_max_malloc2() etc. is beyond this patch. > > > > As Ronald mentioned, if you're going to fix the ABI/API, you should actually > > fix every use of that ABI/API, not just the one you care about (0). > > > > So max values of 1-31 should also be handled. > > I personally don't think there's anything to fix, or if there is, it's > not this. For starters av_max_alloc() was never changed. It was how > av_malloc() handled the max_alloc_size variable. And then, the old > behavior was both buggy (0 to 31 equaling SIZE_MAX or slightly smaller) > and undocumented. I could accept reverting it for the 4.3 branch for the > sake of ABI compatibility, but after a sobump 0-31 should not wrap > around or be replaced by something arbitrary like INT_MAX. Is av_max_alloc(0) useful ? I mean, can you do useful stuff with ffmpeg if max size is truly 0 ? If not, you could consider 0 mean set it to system maxmimum(as my first patch did) > > Chromium already fixed their code by requesting a SIZE_MAX limit > (meaning effectively unlimited), which is what they wanted to begin > with, and others should follow. Yes, but how long will that take ? the change is already out and reverting it fully serves little purpose. You could apply my patch and then revert it at a later time when you break API anyway. Jocke
Joakim Tjernlund: > On Tue, 2020-10-13 at 15:24 +0100, Derek Buitenhuis wrote: >> >> On 13/10/2020 15:19, Joakim Tjernlund wrote: >>> For now just fixing av_malloc_max(0) will do, av_max_malloc2() etc. is beyond this patch. >> >> As Ronald mentioned, if you're going to fix the ABI/API, you should actually >> fix every use of that ABI/API, not just the one you care about (0). >> >> So max values of 1-31 should also be handled. > > OK, how far do you want to take this, will this do? > if (max < 32) > max = INT_MAX; > The old behaviour was to effectively limit allocations to max - 32 in this case (near to SIZE_MAX), not to INT_MAX. And it does not restore default behaviour (the effective default limit was INT_MAX - 32). > Complete API fix would need to revert the patch that caused the break to begin with and I don't think you want that ? > Actually the old behaviour was against the documented API (or where do you see the 32 mentioned in the docs?). - Andreas PS: My rationale for this patch was to match actual behaviour and documented behaviour and to make the full range of INT available for allocations if the maximum has not been overridden by av_max_alloc(). Otherwise allocations of e.g. packets with sizes in the range (INT_MAX - 31 - AV_INPUT_BUFFER_PADDING_SIZE)..(INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE) will fail.
On Tue, 2020-10-13 at 20:11 +0200, Andreas Rheinhardt wrote: > > Joakim Tjernlund: > > On Tue, 2020-10-13 at 15:24 +0100, Derek Buitenhuis wrote: > > > > > > On 13/10/2020 15:19, Joakim Tjernlund wrote: > > > > For now just fixing av_malloc_max(0) will do, av_max_malloc2() etc. is beyond this patch. > > > > > > As Ronald mentioned, if you're going to fix the ABI/API, you should actually > > > fix every use of that ABI/API, not just the one you care about (0). > > > > > > So max values of 1-31 should also be handled. > > > > OK, how far do you want to take this, will this do? > > if (max < 32) > > max = INT_MAX; > > > > The old behaviour was to effectively limit allocations to max - 32 in > this case (near to SIZE_MAX), not to INT_MAX. And it does not restore > default behaviour (the effective default limit was INT_MAX - 32). > > > Complete API fix would need to revert the patch that caused the break to begin with and I don't think you want that ? > > > Actually the old behaviour was against the documented API (or where do > you see the 32 mentioned in the docs?). The old behaviour in code, what the docs say isn't relevant to a failing app. > > - Andreas > > PS: My rationale for this patch was to match actual behaviour and > documented behaviour and to make the full range of INT available for > allocations if the maximum has not been overridden by av_max_alloc(). > Otherwise allocations of e.g. packets with sizes in the range (INT_MAX - > 31 - AV_INPUT_BUFFER_PADDING_SIZE)..(INT_MAX - > AV_INPUT_BUFFER_PADDING_SIZE) will fail. Please suggest a patch to your liking, I am just trying to repair the breakage, I am not a ffmpeg dev. Jocke
On Tue, 2020-10-13 at 20:33 +0200, Joakim Tjernlund wrote: > On Tue, 2020-10-13 at 20:11 +0200, Andreas Rheinhardt wrote: > > > > Joakim Tjernlund: > > > On Tue, 2020-10-13 at 15:24 +0100, Derek Buitenhuis wrote: > > > > > > > > On 13/10/2020 15:19, Joakim Tjernlund wrote: > > > > > For now just fixing av_malloc_max(0) will do, av_max_malloc2() etc. is beyond this patch. > > > > > > > > As Ronald mentioned, if you're going to fix the ABI/API, you should actually > > > > fix every use of that ABI/API, not just the one you care about (0). > > > > > > > > So max values of 1-31 should also be handled. > > > > > > OK, how far do you want to take this, will this do? > > > if (max < 32) > > > max = INT_MAX; > > > > > > > The old behaviour was to effectively limit allocations to max - 32 in > > this case (near to SIZE_MAX), not to INT_MAX. And it does not restore > > default behaviour (the effective default limit was INT_MAX - 32). > > > > > Complete API fix would need to revert the patch that caused the break to begin with and I don't think you want that ? > > > > > Actually the old behaviour was against the documented API (or where do > > you see the 32 mentioned in the docs?). > > The old behaviour in code, what the docs say isn't relevant to a failing app. > > > > > - Andreas > > > > PS: My rationale for this patch was to match actual behaviour and > > documented behaviour and to make the full range of INT available for > > allocations if the maximum has not been overridden by av_max_alloc(). > > Otherwise allocations of e.g. packets with sizes in the range (INT_MAX - > > 31 - AV_INPUT_BUFFER_PADDING_SIZE)..(INT_MAX - > > AV_INPUT_BUFFER_PADDING_SIZE) will fail. > > Please suggest a patch to your liking, I am just trying to repair the breakage, I am not a ffmpeg dev. > Gentle ping Jocke
> On Oct 15, 2020, at 1:14 AM, Joakim Tjernlund <joakim.tjernlund@infinera.com> wrote: > > On Tue, 2020-10-13 at 20:33 +0200, Joakim Tjernlund wrote: >> On Tue, 2020-10-13 at 20:11 +0200, Andreas Rheinhardt wrote: >>> >>> Joakim Tjernlund: >>>> On Tue, 2020-10-13 at 15:24 +0100, Derek Buitenhuis wrote: >>>>> >>>>> On 13/10/2020 15:19, Joakim Tjernlund wrote: >>>>>> For now just fixing av_malloc_max(0) will do, av_max_malloc2() etc. is beyond this patch. >>>>> >>>>> As Ronald mentioned, if you're going to fix the ABI/API, you should actually >>>>> fix every use of that ABI/API, not just the one you care about (0). >>>>> >>>>> So max values of 1-31 should also be handled. >>>> >>>> OK, how far do you want to take this, will this do? >>>> if (max < 32) >>>> max = INT_MAX; >>>> >>> >>> The old behaviour was to effectively limit allocations to max - 32 in >>> this case (near to SIZE_MAX), not to INT_MAX. And it does not restore >>> default behaviour (the effective default limit was INT_MAX - 32). >>> >>>> Complete API fix would need to revert the patch that caused the break to begin with and I don't think you want that ? >>>> >>> Actually the old behaviour was against the documented API (or where do >>> you see the 32 mentioned in the docs?). >> >> The old behaviour in code, what the docs say isn't relevant to a failing app. >> >>> >>> - Andreas >>> >>> PS: My rationale for this patch was to match actual behaviour and >>> documented behaviour and to make the full range of INT available for >>> allocations if the maximum has not been overridden by av_max_alloc(). >>> Otherwise allocations of e.g. packets with sizes in the range (INT_MAX - >>> 31 - AV_INPUT_BUFFER_PADDING_SIZE)..(INT_MAX - >>> AV_INPUT_BUFFER_PADDING_SIZE) will fail. >> >> Please suggest a patch to your liking, I am just trying to repair the breakage, I am not a ffmpeg dev. >> > > Gentle ping There is little benefit to keep such compatibility. And I wonder if there is anyone still use av_max_alloc(0). More workaround means more burden for code maintenance. If there is no use case for av_max_alloc(0), I think we can add an assert(max), but not change the value quietly. > > Jocke > _______________________________________________ > 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".
On Thu, 2020-10-15 at 16:18 +0800, zhilizhao(赵志立) 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 Oct 15, 2020, at 1:14 AM, Joakim Tjernlund <joakim.tjernlund@infinera.com> wrote: > > > > On Tue, 2020-10-13 at 20:33 +0200, Joakim Tjernlund wrote: > > > On Tue, 2020-10-13 at 20:11 +0200, Andreas Rheinhardt wrote: > > > > > > > > Joakim Tjernlund: > > > > > On Tue, 2020-10-13 at 15:24 +0100, Derek Buitenhuis wrote: > > > > > > > > > > > > On 13/10/2020 15:19, Joakim Tjernlund wrote: > > > > > > > For now just fixing av_malloc_max(0) will do, av_max_malloc2() etc. is beyond this patch. > > > > > > > > > > > > As Ronald mentioned, if you're going to fix the ABI/API, you should actually > > > > > > fix every use of that ABI/API, not just the one you care about (0). > > > > > > > > > > > > So max values of 1-31 should also be handled. > > > > > > > > > > OK, how far do you want to take this, will this do? > > > > > if (max < 32) > > > > > max = INT_MAX; > > > > > > > > > > > > > The old behaviour was to effectively limit allocations to max - 32 in > > > > this case (near to SIZE_MAX), not to INT_MAX. And it does not restore > > > > default behaviour (the effective default limit was INT_MAX - 32). > > > > > > > > > Complete API fix would need to revert the patch that caused the break to begin with and I don't think you want that ? > > > > > > > > > Actually the old behaviour was against the documented API (or where do > > > > you see the 32 mentioned in the docs?). > > > > > > The old behaviour in code, what the docs say isn't relevant to a failing app. > > > > > > > > > > > - Andreas > > > > > > > > PS: My rationale for this patch was to match actual behaviour and > > > > documented behaviour and to make the full range of INT available for > > > > allocations if the maximum has not been overridden by av_max_alloc(). > > > > Otherwise allocations of e.g. packets with sizes in the range (INT_MAX - > > > > 31 - AV_INPUT_BUFFER_PADDING_SIZE)..(INT_MAX - > > > > AV_INPUT_BUFFER_PADDING_SIZE) will fail. > > > > > > Please suggest a patch to your liking, I am just trying to repair the breakage, I am not a ffmpeg dev. > > > > > > > Gentle ping > > There is little benefit to keep such compatibility. And I wonder if there is anyone still use av_max_alloc(0). > More workaround means more burden for code maintenance. If there is no use case for av_max_alloc(0), > I think we can add an assert(max), but not change the value quietly. Did you read the patch description ? There are apps using this, all chrome based apps is a potential one and MS Teams in particular does break. ffmpeg DID change av_max_alloc(0) quietly in 4.3 (by accident) so some restoration of previous behaviour to restore at least known breakage is what I am asking for. Jocke
diff --git a/libavutil/mem.c b/libavutil/mem.c index cfb6d8a..52c2b1b 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) + max = INT_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> --- libavutil/mem.c | 2 ++ 1 file changed, 2 insertions(+)