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

[Andreas Rheinhardt]

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.

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