[FFmpeg-devel] [PATCH] Support limiting the number of pixels per image
wm4
nfxjfg at googlemail.com
Sat Dec 10 13:20:01 EET 2016
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.
>
> 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
More information about the ffmpeg-devel
mailing list