[FFmpeg-devel] [PATCH] Support limiting the number of pixels per image

wm4 nfxjfg at googlemail.com
Fri Dec 9 20:45:47 EET 2016


On Fri, 9 Dec 2016 14:11:30 +0100
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Fri, Dec 09, 2016 at 10:14:14AM +0100, wm4 wrote:
> > On Thu,  8 Dec 2016 19:30:25 +0100
> > Michael Niedermayer <michael at niedermayer.cc> wrote:
> >   
> > > TODO: split into 2 patches (one per lib), docs & bump
> > > 
> > > This allows preventing some OOM and "slow decoding" cases by limiting the maximum resolution
> > > this may be useful to avoid fuzzers getting stuck in boring cases
> > > 
> > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > ---
> > >  libavcodec/avcodec.h                 |  8 ++++++++
> > >  libavcodec/options_table.h           |  1 +
> > >  libavutil/imgutils.c                 | 22 ++++++++++++++++++----
> > >  tests/ref/fate/api-mjpeg-codec-param |  2 ++
> > >  tests/ref/fate/api-png-codec-param   |  2 ++
> > >  5 files changed, 31 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > > index 7ac2adaf66..81052b10ef 100644
> > > --- a/libavcodec/avcodec.h
> > > +++ b/libavcodec/avcodec.h
> > > @@ -3570,6 +3570,14 @@ typedef struct AVCodecContext {
> > >       */
> > >      int trailing_padding;
> > >  
> > > +    /**
> > > +     * The number of pixels per image to maximally accept.
> > > +     *
> > > +     * - decoding: set by user
> > > +     * - encoding: unused
> > > +     */
> > > +    int max_pixels;
> > > +
> > >  } AVCodecContext;
> > >  
> > >  AVRational av_codec_get_pkt_timebase         (const AVCodecContext *avctx);
> > > diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h
> > > index ee79859953..2f5eb252fe 100644
> > > --- a/libavcodec/options_table.h
> > > +++ b/libavcodec/options_table.h
> > > @@ -570,6 +570,7 @@ static const AVOption avcodec_options[] = {
> > >  {"codec_whitelist", "List of decoders that are allowed to be used", OFFSET(codec_whitelist), AV_OPT_TYPE_STRING, { .str = NULL },  CHAR_MIN, CHAR_MAX, A|V|S|D },
> > >  {"pixel_format", "set pixel format", OFFSET(pix_fmt), AV_OPT_TYPE_PIXEL_FMT, {.i64=AV_PIX_FMT_NONE}, -1, INT_MAX, 0 },
> > >  {"video_size", "set video size", OFFSET(width), AV_OPT_TYPE_IMAGE_SIZE, {.str=NULL}, 0, INT_MAX, 0 },
> > > +{"max_pixels", "Maximum number of pixels", OFFSET(max_pixels), AV_OPT_TYPE_INT, {.i64 = INT_MAX }, 0, INT_MAX, A|V|S|D },
> > >  {NULL},
> > >  };
> > >  
> > > diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c
> > > index 37808e53d0..7c42ec7e17 100644
> > > --- a/libavutil/imgutils.c
> > > +++ b/libavutil/imgutils.c
> > > @@ -30,6 +30,7 @@
> > >  #include "mathematics.h"
> > >  #include "pixdesc.h"
> > >  #include "rational.h"
> > > +#include "opt.h"
> > >  
> > >  void av_image_fill_max_pixsteps(int max_pixsteps[4], int max_pixstep_comps[4],
> > >                                  const AVPixFmtDescriptor *pixdesc)
> > > @@ -256,11 +257,24 @@ int av_image_check_size(unsigned int w, unsigned int h, int log_offset, void *lo
> > >          .log_ctx    = log_ctx,
> > >      };
> > >  
> > > -    if ((int)w>0 && (int)h>0 && (w+128)*(uint64_t)(h+128) < INT_MAX/8)
> > > -        return 0;
> > > +    if ((int)w<=0 || (int)h<=0 || (w+128)*(uint64_t)(h+128) >= INT_MAX/8) {
> > > +        av_log(&imgutils, AV_LOG_ERROR, "Picture size %ux%u is invalid\n", w, h);
> > > +        return AVERROR(EINVAL);
> > > +    }
> > >  
> > > -    av_log(&imgutils, AV_LOG_ERROR, "Picture size %ux%u is invalid\n", w, h);
> > > -    return AVERROR(EINVAL);
> > > +    if (log_ctx) {
> > > +        int64_t max = INT_MAX;
> > > +        if (av_opt_get_int(log_ctx, "max_pixels",  AV_OPT_SEARCH_CHILDREN, &max) >= 0) {  
> > 
> > IMHO terrible implementation. Just add a new function that takes an
> > honest argument?  
> 
> adding a new function is possible but more complex, there are
> currently 79 uses of this, all need to be changed eventually.

This argument holds up only if they have a max_pixels AVOption, of
course. There are probably not 79 of them.

> and if we add max width and height this would start over again.
> on top of this, this function should probably have a pixel format
> parameter too. So if we change it that should be added too.

I agree at least about the latter. At least it would be simpler than
changing linesizes to size_t and such.

> 
> >   
> > > +            if (w*h > max) {
> > > +                av_log(&imgutils, AV_LOG_ERROR,
> > > +                       "Picture size %ux%u exceeds specified max pixel count %"PRId64"\n",
> > > +                       w, h, max);
> > > +                return AVERROR(EINVAL);
> > > +            }
> > > +        }
> > > +    }
> > > +
> > > +    return 0;
> > >  }
> > >  
> > >  int av_image_check_sar(unsigned int w, unsigned int h, AVRational sar)
> > > diff --git a/tests/ref/fate/api-mjpeg-codec-param b/tests/ref/fate/api-mjpeg-codec-param
> > > index c67d1b1ab0..08313adab3 100644
> > > --- a/tests/ref/fate/api-mjpeg-codec-param
> > > +++ b/tests/ref/fate/api-mjpeg-codec-param
> > > @@ -155,6 +155,7 @@ stream=0, decode=0
> > >      codec_whitelist=
> > >      pixel_format=yuvj422p
> > >      video_size=400x225
> > > +    max_pixels=2147483647
> > >  stream=0, decode=1
> > >      b=0
> > >      ab=0
> > > @@ -312,3 +313,4 @@ stream=0, decode=1
> > >      codec_whitelist=
> > >      pixel_format=yuvj422p
> > >      video_size=400x225
> > > +    max_pixels=2147483647
> > > diff --git a/tests/ref/fate/api-png-codec-param b/tests/ref/fate/api-png-codec-param
> > > index bd53441894..7a9a921461 100644
> > > --- a/tests/ref/fate/api-png-codec-param
> > > +++ b/tests/ref/fate/api-png-codec-param
> > > @@ -155,6 +155,7 @@ stream=0, decode=0
> > >      codec_whitelist=
> > >      pixel_format=rgba
> > >      video_size=128x128
> > > +    max_pixels=2147483647
> > >  stream=0, decode=1
> > >      b=0
> > >      ab=0
> > > @@ -312,3 +313,4 @@ stream=0, decode=1
> > >      codec_whitelist=
> > >      pixel_format=rgba
> > >      video_size=128x128
> > > +    max_pixels=2147483647  
> > 
> > In general I'd rather have the current pixel limit _removed_. It causes
> > problems with processing high-res images.  
> 
> You can open a feature request on trac or submit a patch.
> 
> With a pixel_format parameter the limit can be lifted a bit, for
> further lifting the use of int to address bytes would need to be
> checked for carefully and changed, that also will be exclusive to 64bit
> archs as 32bit ones wont have enough address space for larger images
> 
> If you want to work on either of this, its probably easier if i leave
> the max_pixels parameter addition to you too as either needs a new
> function and it should be easier to do both changes together.
> 
> [...]

I'm not immediately interested in that. Maybe once it bites me.
But users switching from FFmpeg to other software because it fails at
the limit does happen.


More information about the ffmpeg-devel mailing list