[FFmpeg-devel] [PATCH] avcodec: Require avoptions for the user to set max_pixels.

[wm4]

On Sun, 11 Dec 2016 16:06:41 +0100
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Sun, Dec 11, 2016 at 04:02:27PM +0100, Hendrik Leppkes wrote:
> > On Sun, Dec 11, 2016 at 3:59 PM, Michael Niedermayer
> > <michael at niedermayer.cc> wrote:  
> > > On Sun, Dec 11, 2016 at 01:54:28PM +0100, wm4 wrote:  
> > >> On Sat, 10 Dec 2016 23:01:04 +0100
> > >> Michael Niedermayer <michael at niedermayer.cc> wrote:
> > >>  
> > >> > When we will backport this, it will be inevitably in a different location
> > >> > in AVCodecContext in each release and master. 3.0, 3.1, 3.2 and master
> > >> > use the same soname though and must have a binary compatible interface.
> > >> > It thus can only saftely be accessed through AVOptions
> > >> >
> > >> > It may be enough to require this only in the releases but that could be
> > >> > rather confusing.
> > >> >
> > >> > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > >> > ---
> > >> >  libavcodec/avcodec.h | 4 ++--
> > >> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >> >
> > >> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > >> > index 02234aee67..8123092ac0 100644
> > >> > --- a/libavcodec/avcodec.h
> > >> > +++ b/libavcodec/avcodec.h
> > >> > @@ -3573,8 +3573,8 @@ typedef struct AVCodecContext {
> > >> >      /**
> > >> >       * The number of pixels per image to maximally accept.
> > >> >       *
> > >> > -     * - decoding: set by user
> > >> > -     * - encoding: set by user
> > >> > +     * - decoding: set by user through AVOptions (NO direct access)
> > >> > +     * - encoding: set by user through AVOptions (NO direct access)
> > >> >       */
> > >> >      int64_t max_pixels;
> > >> >  
> > >>
> > >> Why? We gave up the Libav ABI compat. abomination.  
> > >
> > > Its explained in the patch comment above
> > >
> > > max_pixels should to be backported as it allows users to control memory
> > > use from large images better, avoid some OOMs and fixes issues which
> > > some people consider security bugs
> > > if its backported it will not be in the same location relative to the
> > > start of AVCodecContext in master, 3.2, 3.1, 3.0
> > > master, 3.2, 3.1, 3.0 all have the same soname
> > > libs using the same soname need to be binary compatible
> > > direct access to one location will not work and thus be binary
> > > incompatible if the field is at a different location
> > >
> > > Thus releases cannot use direct access to the max_pixels field.
> > > One could say it differently in that if 3.0.X would access max_pixels
> > > directly at byte offset 123 then an application built against that
> > > and linked to 3.2 will access another field before max_pixels in 3.2
> > >  
> > 
> > Thats why one doesn't backport features. It just causes headaches when
> > structs change.  
> 
> Security fixes require such changes sometimes
> 
> the whitelists, if someone did the rather massive work to backport
> them would be similar
> 
> [...]

It's not a security fix. It merely triggers OOM, and there are
literally thousands of other things that can trigger OOM.

Let me repeat: calling max_pixels a security fix is 100% bullshit.