| Message ID | 20161209193140.438-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
On Fri, 9 Dec 2016, Michael Niedermayer 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@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; Don't we want to make this an int64, for future extendability? > + > } 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 }, INT_MAX is probably OK, as who knows what will crash for bigger sizes, right? Thanks, Marton
On 12/9/16, Michael Niedermayer <michael@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@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; int64_t please
On Fri, 9 Dec 2016 20:31:40 +0100 Michael Niedermayer <michael@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@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. > > int av_image_check_sar(unsigned int w, unsigned int h, AVRational sar) > diff --git a/libavutil/imgutils.h b/libavutil/imgutils.h > index 23282a38fa..19f34deced 100644 > --- a/libavutil/imgutils.h > +++ b/libavutil/imgutils.h > @@ -192,6 +192,20 @@ int av_image_copy_to_buffer(uint8_t *dst, int dst_size, > int av_image_check_size(unsigned int w, unsigned int h, int log_offset, void *log_ctx); > > /** > + * Check if the given dimension of an image is valid, meaning that all > + * bytes of the image can be addressed with a signed int. > + * > + * @param w the width of the picture > + * @param h the height of the picture > + * @param max_pixels the maximum number of pixels the user wants to accept > + * @param pix_fmt the pixel format, can be AV_PIX_FMT_NONE if unknown. > + * @param log_offset the offset to sum to the log level for logging with log_ctx > + * @param log_ctx the parent logging context, it may be NULL > + * @return >= 0 if valid, a negative error code otherwise > + */ > +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); > + > +/** > * Check if the given sample aspect ratio of an image is valid. > * > * It is considered invalid if the denominator is 0 or if applying the ratio > 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
On 09.12.2016 20:31, Michael Niedermayer 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 Setting this option to a reasonably low value like 1000000 fixes a significant amount of my slow samples. So having it is definitely useful. > Signed-off-by: Michael Niedermayer <michael@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 It should work for encoding as well, doesn't it? > + */ > + 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 }, Setting the default to INT_MAX is a bit misleading, because the existing check doesn't allow anything larger than 264241280. Best regards, Andreas
On Sat, Dec 10, 2016 at 12:20:01PM +0100, wm4 wrote: > On Fri, 9 Dec 2016 20:31:40 +0100 > Michael Niedermayer <michael@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@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 [...]
On Sat, Dec 10, 2016 at 01:33:59PM +0100, Andreas Cadhalpun wrote: > On 09.12.2016 20:31, Michael Niedermayer 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 > > Setting this option to a reasonably low value like 1000000 fixes a > significant amount of my slow samples. So having it is definitely useful. > > > Signed-off-by: Michael Niedermayer <michael@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 > > It should work for encoding as well, doesn't it? enabled it for encoding too thx [...]
On Fri, Dec 09, 2016 at 08:47:00PM +0100, Paul B Mahol wrote: > On 12/9/16, Michael Niedermayer <michael@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@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; > > int64_t please changed [...] thx
On Sat, 10 Dec 2016 22:36:50 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > Personally if i would be doing this i would use avoptions as > that means less code to backport that is less work Less changes doesn't make it right. Anyway, you don't need to listen to me. I'm just arguing this way because I think using AVOptions will make the code less readable and more confusing and fragile (both of which we don't need more).
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); } int av_image_check_sar(unsigned int w, unsigned int h, AVRational sar) diff --git a/libavutil/imgutils.h b/libavutil/imgutils.h index 23282a38fa..19f34deced 100644 --- a/libavutil/imgutils.h +++ b/libavutil/imgutils.h @@ -192,6 +192,20 @@ int av_image_copy_to_buffer(uint8_t *dst, int dst_size, int av_image_check_size(unsigned int w, unsigned int h, int log_offset, void *log_ctx); /** + * Check if the given dimension of an image is valid, meaning that all + * bytes of the image can be addressed with a signed int. + * + * @param w the width of the picture + * @param h the height of the picture + * @param max_pixels the maximum number of pixels the user wants to accept + * @param pix_fmt the pixel format, can be AV_PIX_FMT_NONE if unknown. + * @param log_offset the offset to sum to the log level for logging with log_ctx + * @param log_ctx the parent logging context, it may be NULL + * @return >= 0 if valid, a negative error code otherwise + */ +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); + +/** * Check if the given sample aspect ratio of an image is valid. * * It is considered invalid if the denominator is 0 or if applying the ratio 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
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@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(-)