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

[Joakim Tjernlund]

On Mon, 2020-11-09 at 22:53 +0000, Joakim Tjernlund wrote:
> On Sun, 2020-11-08 at 15:13 +0100, Michael Niedermayer 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 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.chromium.org%2Fp%2Fchromium%2Fissues%2Fdetail%3Fid%3D1095962&data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C9d88692519584c6d00e908d8850257f0%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637405592400704104%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=WhXCZX6Cne5U7E7GvCWpL%2FjV2Z80sqU1nPe81gk724A%3D&reserved=0
> > > > > ----------------------------
> > > > > 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fchromium-review.googlesource.com%2Fc%2Fchromium%2Fthird_party%2Fffmpeg%2F%2B%2F73563&data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C9d88692519584c6d00e908d8850257f0%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637405592400704104%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=5oUMLsjhJWGduxmQTEdD33BuJwX1H7oCaMUKVnf5L%2Bw%3D&reserved=0
> > > > > [2] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FFFmpeg%2FFFmpeg%2Fcommit%2F731c77589841&data=04%7C01%7Cjoakim.tjernlund%40infinera.com%7C9d88692519584c6d00e908d8850257f0%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637405592400714100%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=eAbvDHG8ivjArQOQSCjS7KO%2B2nqIqBa2vl9JV76uLM0%3D&reserved=0
> > > > > ---------------------------
> > > > > 
> > > > > 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
> 
> Did you reach a conclusion ? Go back to my original patch ?
> 

Ping ? I need to get off this list as I cannot handle the mail volume.

 Jocke