On Fri, 9 Dec 2016 14:11:30 +0100
Michael Niedermayer <mich...@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 <mich...@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 <mich...@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
