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

[Joakim Tjernlund]

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 at 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 at 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