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

[Michael Niedermayer]

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Does the universe only have a finite lifespan? No, its going to go on
forever, its just that you wont like living in it. -- Hiranya Peiri
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20201108/4091709d/attachment.sig>