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

[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"

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     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

"I am not trying to be anyone's saviour, I'm trying to think about the
 future and not be sad" - Elon Musk

-------------- 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/c750752c/attachment.sig>