| Message ID | 20161208183025.17792-1-michael@niedermayer.cc |
|---|---|
| State | Superseded |
| Headers | show |
On 08.12.2016 19:30, Michael Niedermayer 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@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(-) That's probably OK. One caveat is that currently not every demuxer uses av_image_check_size, but I'm working on fixing that. Do you plan to reduce the default in a future patch? Best regards, Andreas
On Fri, Dec 9, 2016 at 12:45 AM, Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> wrote: > On 08.12.2016 19:30, Michael Niedermayer 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@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(-) > > That's probably OK. > One caveat is that currently not every demuxer uses av_image_check_size, > but I'm working on fixing that. > Do you plan to reduce the default in a future patch? > There is already valid high-resolution image files today that avcodec cannot open due to the technical limits checked in that function right now (which prevent integer overflows in other parts of the code that should be fixed instead). Further reducing this for all users seems like a terrible idea to me. - Hendrik
On Thu, 8 Dec 2016 19:30:25 +0100 Michael Niedermayer <michael@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@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? > + 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.
On Fri, Dec 09, 2016 at 10:14:14AM +0100, wm4 wrote: > On Thu, 8 Dec 2016 19:30:25 +0100 > Michael Niedermayer <michael@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@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. [...]
On Fri, 9 Dec 2016 14:11:30 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Fri, Dec 09, 2016 at 10:14:14AM +0100, wm4 wrote: > > On Thu, 8 Dec 2016 19:30:25 +0100 > > Michael Niedermayer <michael@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@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. This argument holds up only if they have a max_pixels AVOption, of course. There are probably not 79 of them. > 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. I agree at least about the latter. At least it would be simpler than changing linesizes to size_t and such. > > > > > > + 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. > > [...] I'm not immediately interested in that. Maybe once it bites me. But users switching from FFmpeg to other software because it fails at the limit does happen.
On Fri, Dec 09, 2016 at 12:45:22AM +0100, Andreas Cadhalpun wrote: > On 08.12.2016 19:30, Michael Niedermayer 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@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(-) > > That's probably OK. > One caveat is that currently not every demuxer uses av_image_check_size, > but I'm working on fixing that. > Do you plan to reduce the default in a future patch? Iam happy to leave that to others to do [...]
2016-12-09 19:45 GMT+01:00 wm4 <nfxjfg@googlemail.com>: > But users switching from FFmpeg to other software because > it fails at the limit does happen. Could you elaborate? Was there a bug report that we ignored? Carl Eugen
On Sat, 10 Dec 2016 11:51:04 +0100 Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2016-12-09 19:45 GMT+01:00 wm4 <nfxjfg@googlemail.com>: > > > But users switching from FFmpeg to other software because > > it fails at the limit does happen. > > Could you elaborate? > Was there a bug report that we ignored? Reporting bugs to ffmpeg is so tedious, why would anyone do that? I think in the specific case I had in mind, imagemagick was used instead, but I could be wrong. The use-case was processing high-res scans. Surely not really a worry for ffmpeg, since it's concerned about video, and 16K video is still "a bit" in the future.
2016-12-10 12:13 GMT+01:00 wm4 <nfxjfg@googlemail.com>: > On Sat, 10 Dec 2016 11:51:04 +0100 > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > >> 2016-12-09 19:45 GMT+01:00 wm4 <nfxjfg@googlemail.com>: >> >> > But users switching from FFmpeg to other software because >> > it fails at the limit does happen. >> >> Could you elaborate? >> Was there a bug report that we ignored? > > Reporting bugs to ffmpeg is so tedious, why would anyone do that? We have a unreasonable large amount of bug reports that are unlikely to get fixed (by purely technical reasons), I wonder why something as trivial as increasing image size for actual use cases is not reported. > I think in the specific case I had in mind, imagemagick was > used instead, but I could be wrong. The use-case was > processing high-res scans. > > Surely not really a worry for ffmpeg, since it's concerned > about video, and 16K video is still "a bit" in the future. I just tested 16k and it works for some cases, reports "codec isn't specified for 16k" for others and fails with x264. What exactly was the issue that could be fixed in FFmpeg? Carl Eugen
On Sat, Dec 10, 2016 at 12:43:49PM +0100, Carl Eugen Hoyos wrote: > 2016-12-10 12:13 GMT+01:00 wm4 <nfxjfg@googlemail.com>: > > On Sat, 10 Dec 2016 11:51:04 +0100 > > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > > >> 2016-12-09 19:45 GMT+01:00 wm4 <nfxjfg@googlemail.com>: > >> > >> > But users switching from FFmpeg to other software because > >> > it fails at the limit does happen. > >> > >> Could you elaborate? > >> Was there a bug report that we ignored? > > > > Reporting bugs to ffmpeg is so tedious, why would anyone do that? > > We have a unreasonable large amount of bug reports that are > unlikely to get fixed (by purely technical reasons), I wonder Somewhat off topic but if we want more bugs to be fixed than what volunteers are doing then its needed to either motivate more volunteers somehow or 1. get more sponsors and a real budget (like 100k per year) 2. put bounties on most tickets I would expect that if one can live of fixing ffmpeg bugs full time, someone would probably do exactly that [...]
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) { + 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
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 | 22 ++++++++++++++++++---- tests/ref/fate/api-mjpeg-codec-param | 2 ++ tests/ref/fate/api-png-codec-param | 2 ++ 5 files changed, 31 insertions(+), 4 deletions(-)