[FFmpeg-devel] [PATCH] Support limiting the number of pixels per image
Michael Niedermayer
michael at niedermayer.cc
Fri Dec 9 15:11:30 EET 2016
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.
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.
>
> > + 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.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Modern terrorism, a quick summary: Need oil, start war with country that
has oil, kill hundread thousand in war. Let country fall into chaos,
be surprised about raise of fundamantalists. Drop more bombs, kill more
people, be surprised about them taking revenge and drop even more bombs
and strip your own citizens of their rights and freedoms. to be continued
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20161209/e2dfd3b0/attachment.sig>
More information about the ffmpeg-devel
mailing list