[FFmpeg-devel] [PATCH 1/7] avformat/mov_chan: Check for FF_SANE_NB_CHANNELS

[Anton Khirnov]

Quoting Michael Niedermayer (2024-09-13 19:48:46)
> On Fri, Sep 13, 2024 at 12:08:45PM +0200, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2024-09-13 01:33:31)
> > > We do not support more channels. For example avcodec_open2() limits channels this way too
> > > 
> > > The example file contains multiple chunks with over 16 million channels
> > 
> > We had this discussion already.
> 
> I remembered something too, but couldnt find the thread within teh time i was looking for it
> 
> 
> > Ad-hoc checks like this are only
> > addressing a symptom (probably one of many), and hide the actual bug.
> 
> If you have a better fix, submit it.
> If you want me to implement this differently, the first step is to describe
> what you have in mind, that the implementation should look like.
> 
> But if one
> 1. allocates an attacker specified amount of memory
> 2. iterate over it by an attacker specified number of times

The root of my objection is exactly this - the patch does not make it
at all clear what code is allocating excessive amounts of memory.

> > 
> > > +#include "libavcodec/internal.h"
> > 
> > I dislike this as well.
> 
> I am fine with it.
> 
> But if you dont, then maybe you can suggest another way to check
> for the number that we support.

My second objection, in case it is not clear, is to libavformat
including libavcodec internal headers that define libavcodec internal
structures. That is a slippery slope that easily leads to people
actually accessing those internal structures.

But also the number 512 is entirely arbitrary, and if we decide to
arbitrarily restrict the channel count everywhere then it should be done
at the level of AVChannelLayout. E.g. by making nb_channels and uint8 or
uint16, and/or adding the check to av_channel_layout_check().

-- 
Anton Khirnov