[FFmpeg-devel] Support limiting the number of pixels per image

Submitted by Michael Niedermayer on Dec. 9, 2016, 7:31 p.m.

Details

Message ID 20161209193140.438-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Dec. 9, 2016, 7:31 p.m.
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(-)

Comments

Marton Balint Dec. 9, 2016, 7:38 p.m.
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
Paul B Mahol Dec. 9, 2016, 7:47 p.m.
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
wm4 Dec. 10, 2016, 11:20 a.m.
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
Andreas Cadhalpun Dec. 10, 2016, 12:33 p.m.
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
Michael Niedermayer Dec. 10, 2016, 9:36 p.m.
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

[...]
Michael Niedermayer Dec. 10, 2016, 9:37 p.m.
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

[...]
Michael Niedermayer Dec. 10, 2016, 9:37 p.m.
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
wm4 Dec. 11, 2016, 12:49 p.m.
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).

Patch hide | download patch | download mbox

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