[FFmpeg-devel] [PATCH] Support limiting the number of pixels per image
Michael Niedermayer
michael at niedermayer.cc
Sat Dec 10 23:36:50 EET 2016
On Sat, Dec 10, 2016 at 12:20:01PM +0100, wm4 wrote:
> On Fri, 9 Dec 2016 20:31:40 +0100
> Michael Niedermayer <michael at niedermayer.cc> wrote:
>
> > Adds av_image_check_size2() with max_pixels and pix_fmt parameters.
> > pix_fmt is unused and is added to avoid a 2nd API change later
> >
> > The old function uses AVOptions to obtain the max_pixels value to simplify
> > the transition.
> >
> > 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 | 31 ++++++++++++++++++++++++++-----
> > libavutil/imgutils.h | 14 ++++++++++++++
> > tests/ref/fate/api-mjpeg-codec-param | 2 ++
> > tests/ref/fate/api-png-codec-param | 2 ++
> > 6 files changed, 53 insertions(+), 5 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..96f2bbdc4f 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)
> > @@ -248,7 +249,7 @@ static const AVClass imgutils_class = {
> > .parent_log_context_offset = offsetof(ImgUtils, log_ctx),
> > };
> >
> > -int av_image_check_size(unsigned int w, unsigned int h, int log_offset, void *log_ctx)
> > +int av_image_check_size2(unsigned int w, unsigned int h, int64_t max_pixels, enum AVPixelFormat pix_fmt, int log_offset, void *log_ctx)
> > {
> > ImgUtils imgutils = {
> > .class = &imgutils_class,
> > @@ -256,11 +257,31 @@ 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 (max_pixels < INT64_MAX) {
> > + if (w*(int64_t)h > max_pixels) {
> > + av_log(&imgutils, AV_LOG_ERROR,
> > + "Picture size %ux%u exceeds specified max pixel count %"PRId64"\n",
> > + w, h, max_pixels);
> > + return AVERROR(EINVAL);
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int av_image_check_size(unsigned int w, unsigned int h, int log_offset, void *log_ctx)
> > +{
> > + int64_t max = INT64_MAX;
> > +
> > + if (log_ctx)
> > + av_opt_get_int(log_ctx, "max_pixels", AV_OPT_SEARCH_CHILDREN, &max);
> > +
> > + return av_image_check_size2(w, h, max, AV_PIX_FMT_NONE, log_offset, log_ctx);
> > }
>
> Still against implicitly using an AVOption. Explicitly passing limits is
> better. Also, while your suggestion is convenient, it doesn't guarantee
> that ever caller will pass the correct context to it (i.e. an
> AVCodecContext that happens to have max_pixels somehow set from the
> CLI), or that someone inspecting the code on the callee side is aware
> about the AVOption retrieval and does not fix it. Someone new to the
> codebase will also be puzzled what the seemingly unused max_pixels
> field does at all.
ok, applied without AVOptions
please update the code that now as a consequence requires an update
and also backport it to all supported releases.
Ill backport the av_image_check_size2() addition
Personally if i would be doing this i would use avoptions as
that means less code to backport that is less work
[...]
--
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/20161210/cf0d891b/attachment.sig>
More information about the ffmpeg-devel
mailing list