| Message ID | 20161208173742.11040-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
Hi, On Thu, Dec 8, 2016 at 12:37 PM, Michael Niedermayer <michael@niedermayer.cc > wrote: > Suggested-by: Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/options_table.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/options_table.h b/libavformat/options_table.h > index d5448e503f..19cb87ae4e 100644 > --- a/libavformat/options_table.h > +++ b/libavformat/options_table.h > @@ -105,7 +105,7 @@ static const AVOption avformat_options[] = { > {"format_whitelist", "List of demuxers that are allowed to be used", > OFFSET(format_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, > CHAR_MAX, D }, > {"protocol_whitelist", "List of protocols that are allowed to be used", > OFFSET(protocol_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, > CHAR_MAX, D }, > {"protocol_blacklist", "List of protocols that are not allowed to be > used", OFFSET(protocol_blacklist), AV_OPT_TYPE_STRING, { .str = NULL }, > CHAR_MIN, CHAR_MAX, D }, > -{"max_streams", "maximum number of streams", OFFSET(max_streams), > AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 0, INT_MAX, D }, > +{"max_streams", "maximum number of streams", OFFSET(max_streams), > AV_OPT_TYPE_INT, { .i64 = 100 }, 0, INT_MAX, D }, > {NULL}, > }; Isn't this - in some way - an ABI break? Ronald
On 12/8/2016 2:44 PM, Ronald S. Bultje wrote: > Hi, > > On Thu, Dec 8, 2016 at 12:37 PM, Michael Niedermayer <michael@niedermayer.cc >> wrote: > >> Suggested-by: Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> --- >> libavformat/options_table.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavformat/options_table.h b/libavformat/options_table.h >> index d5448e503f..19cb87ae4e 100644 >> --- a/libavformat/options_table.h >> +++ b/libavformat/options_table.h >> @@ -105,7 +105,7 @@ static const AVOption avformat_options[] = { >> {"format_whitelist", "List of demuxers that are allowed to be used", >> OFFSET(format_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, >> CHAR_MAX, D }, >> {"protocol_whitelist", "List of protocols that are allowed to be used", >> OFFSET(protocol_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, >> CHAR_MAX, D }, >> {"protocol_blacklist", "List of protocols that are not allowed to be >> used", OFFSET(protocol_blacklist), AV_OPT_TYPE_STRING, { .str = NULL }, >> CHAR_MIN, CHAR_MAX, D }, >> -{"max_streams", "maximum number of streams", OFFSET(max_streams), >> AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 0, INT_MAX, D }, >> +{"max_streams", "maximum number of streams", OFFSET(max_streams), >> AV_OPT_TYPE_INT, { .i64 = 100 }, 0, INT_MAX, D }, >> {NULL}, >> }; > > > Isn't this - in some way - an ABI break? > > Ronald The option in question was added an hour ago, so no.
On Thu, 8 Dec 2016 18:37:42 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > Suggested-by: Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/options_table.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/options_table.h b/libavformat/options_table.h > index d5448e503f..19cb87ae4e 100644 > --- a/libavformat/options_table.h > +++ b/libavformat/options_table.h > @@ -105,7 +105,7 @@ static const AVOption avformat_options[] = { > {"format_whitelist", "List of demuxers that are allowed to be used", OFFSET(format_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, CHAR_MAX, D }, > {"protocol_whitelist", "List of protocols that are allowed to be used", OFFSET(protocol_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, CHAR_MAX, D }, > {"protocol_blacklist", "List of protocols that are not allowed to be used", OFFSET(protocol_blacklist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, CHAR_MAX, D }, > -{"max_streams", "maximum number of streams", OFFSET(max_streams), AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 0, INT_MAX, D }, > +{"max_streams", "maximum number of streams", OFFSET(max_streams), AV_OPT_TYPE_INT, { .i64 = 100 }, 0, INT_MAX, D }, > {NULL}, > }; > That seems awfully low. Why limit stream count to 100, while allowing e.g. 2GB large font attachments?
On Thu, Dec 08, 2016 at 07:25:59PM +0100, wm4 wrote: > On Thu, 8 Dec 2016 18:37:42 +0100 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > Suggested-by: Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavformat/options_table.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/libavformat/options_table.h b/libavformat/options_table.h > > index d5448e503f..19cb87ae4e 100644 > > --- a/libavformat/options_table.h > > +++ b/libavformat/options_table.h > > @@ -105,7 +105,7 @@ static const AVOption avformat_options[] = { > > {"format_whitelist", "List of demuxers that are allowed to be used", OFFSET(format_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, CHAR_MAX, D }, > > {"protocol_whitelist", "List of protocols that are allowed to be used", OFFSET(protocol_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, CHAR_MAX, D }, > > {"protocol_blacklist", "List of protocols that are not allowed to be used", OFFSET(protocol_blacklist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, CHAR_MAX, D }, > > -{"max_streams", "maximum number of streams", OFFSET(max_streams), AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 0, INT_MAX, D }, > > +{"max_streams", "maximum number of streams", OFFSET(max_streams), AV_OPT_TYPE_INT, { .i64 = 100 }, 0, INT_MAX, D }, > > {NULL}, > > }; > > > > That seems awfully low. Why limit stream count to 100, Is this too little for real world streams ? what limit would not interfere with a positive user experience ? > while allowing > e.g. 2GB large font attachments? theres no limit on attachments currently except the natural int size we may want to have a tighter limit there too
On Thu, 8 Dec 2016 19:36:20 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Thu, Dec 08, 2016 at 07:25:59PM +0100, wm4 wrote: > > On Thu, 8 Dec 2016 18:37:42 +0100 > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > Suggested-by: Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavformat/options_table.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/libavformat/options_table.h b/libavformat/options_table.h > > > index d5448e503f..19cb87ae4e 100644 > > > --- a/libavformat/options_table.h > > > +++ b/libavformat/options_table.h > > > @@ -105,7 +105,7 @@ static const AVOption avformat_options[] = { > > > {"format_whitelist", "List of demuxers that are allowed to be used", OFFSET(format_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, CHAR_MAX, D }, > > > {"protocol_whitelist", "List of protocols that are allowed to be used", OFFSET(protocol_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, CHAR_MAX, D }, > > > {"protocol_blacklist", "List of protocols that are not allowed to be used", OFFSET(protocol_blacklist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, CHAR_MAX, D }, > > > -{"max_streams", "maximum number of streams", OFFSET(max_streams), AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 0, INT_MAX, D }, > > > +{"max_streams", "maximum number of streams", OFFSET(max_streams), AV_OPT_TYPE_INT, { .i64 = 100 }, 0, INT_MAX, D }, > > > {NULL}, > > > }; > > > > > > > That seems awfully low. Why limit stream count to 100, > > Is this too little for real world streams ? > what limit would not interfere with a positive user experience ? > > > > while allowing > > e.g. 2GB large font attachments? > > theres no limit on attachments currently except the natural int size > we may want to have a tighter limit there too > > This will lead to thousands of options tuning various limits that 1. nobody wants to use, 2. even if they want, will not find, and 3. are ugly and intrusive. And then there'll probably still be a way to easily OOM ffmpeg, and using a sandbox will still be superior over setting these options.
Hi, On Thu, Dec 8, 2016 at 1:25 PM, James Almer <jamrial@gmail.com> wrote: > On 12/8/2016 2:44 PM, Ronald S. Bultje wrote: > > Hi, > > > > On Thu, Dec 8, 2016 at 12:37 PM, Michael Niedermayer > <michael@niedermayer.cc > >> wrote: > > > >> Suggested-by: Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> > >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >> --- > >> libavformat/options_table.h | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/libavformat/options_table.h b/libavformat/options_table.h > >> index d5448e503f..19cb87ae4e 100644 > >> --- a/libavformat/options_table.h > >> +++ b/libavformat/options_table.h > >> @@ -105,7 +105,7 @@ static const AVOption avformat_options[] = { > >> {"format_whitelist", "List of demuxers that are allowed to be used", > >> OFFSET(format_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, > CHAR_MIN, > >> CHAR_MAX, D }, > >> {"protocol_whitelist", "List of protocols that are allowed to be used", > >> OFFSET(protocol_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, > CHAR_MIN, > >> CHAR_MAX, D }, > >> {"protocol_blacklist", "List of protocols that are not allowed to be > >> used", OFFSET(protocol_blacklist), AV_OPT_TYPE_STRING, { .str = NULL }, > >> CHAR_MIN, CHAR_MAX, D }, > >> -{"max_streams", "maximum number of streams", OFFSET(max_streams), > >> AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 0, INT_MAX, D }, > >> +{"max_streams", "maximum number of streams", OFFSET(max_streams), > >> AV_OPT_TYPE_INT, { .i64 = 100 }, 0, INT_MAX, D }, > >> {NULL}, > >> }; > > > > > > Isn't this - in some way - an ABI break? > > > > Ronald > > The option in question was added an hour ago, so no. I meant that legal input with 101 streams stop working. That's not crazy IMO. IOW, 100 is kinda low. Ronald
On Thu, Dec 08, 2016 at 07:48:46PM +0100, wm4 wrote: > On Thu, 8 Dec 2016 19:36:20 +0100 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > On Thu, Dec 08, 2016 at 07:25:59PM +0100, wm4 wrote: > > > On Thu, 8 Dec 2016 18:37:42 +0100 > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > Suggested-by: Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > --- > > > > libavformat/options_table.h | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/libavformat/options_table.h b/libavformat/options_table.h > > > > index d5448e503f..19cb87ae4e 100644 > > > > --- a/libavformat/options_table.h > > > > +++ b/libavformat/options_table.h > > > > @@ -105,7 +105,7 @@ static const AVOption avformat_options[] = { > > > > {"format_whitelist", "List of demuxers that are allowed to be used", OFFSET(format_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, CHAR_MAX, D }, > > > > {"protocol_whitelist", "List of protocols that are allowed to be used", OFFSET(protocol_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, CHAR_MAX, D }, > > > > {"protocol_blacklist", "List of protocols that are not allowed to be used", OFFSET(protocol_blacklist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, CHAR_MAX, D }, > > > > -{"max_streams", "maximum number of streams", OFFSET(max_streams), AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 0, INT_MAX, D }, > > > > +{"max_streams", "maximum number of streams", OFFSET(max_streams), AV_OPT_TYPE_INT, { .i64 = 100 }, 0, INT_MAX, D }, > > > > {NULL}, > > > > }; > > > > > > > > > > That seems awfully low. Why limit stream count to 100, > > > > Is this too little for real world streams ? > > what limit would not interfere with a positive user experience ? > > > > > > > while allowing > > > e.g. 2GB large font attachments? > > > > theres no limit on attachments currently except the natural int size > > we may want to have a tighter limit there too > > > > > > This will lead to thousands of options tuning various limits that > 1. nobody wants to use, 2. even if they want, will not find, and 3. are > ugly and intrusive. > > And then there'll probably still be a way to easily OOM ffmpeg, and > using a sandbox will still be superior over setting these options. Ive implemented the generic solution with one parameter in [FFmpeg-devel] [PATCH 3/3] avutil/mem: Support limiting the heap usage which you and others dont like i understand why this isnt liked and i understand why many seperate options arent liked but Either of them is needed or FFmpeg cannot saftely be used via normal lib mechanisms and must be run as a seperate process that the main app has to expect to crash with out of memory. (if the input is untrusted media) This is something the community must decide. A. Is a heap limit for av_*alloc*() acceptable ? B. Are case based limits acceptable ? C. document that libavcodec, libavformat and ffmpeg must be run as a seperate process if the media comes from untrusted sources. one of these options has to be choosen. also even if C is choosen, a small set of limits on the main parameters still is needed to allow efficient fuzzing, all issues reported by oss-fuzz recently are "hangs" due to slow decoding, with the pixel max patch it instead of being slow hits this: Picture size 512x65520 exceeds specified max pixel count 414719 in the case i tried [...]
On Thu, Dec 08, 2016 at 02:05:19PM -0500, Ronald S. Bultje wrote: > Hi, > > On Thu, Dec 8, 2016 at 1:25 PM, James Almer <jamrial@gmail.com> wrote: > > > On 12/8/2016 2:44 PM, Ronald S. Bultje wrote: > > > Hi, > > > > > > On Thu, Dec 8, 2016 at 12:37 PM, Michael Niedermayer > > <michael@niedermayer.cc > > >> wrote: > > > > > >> Suggested-by: Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> > > >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > >> --- > > >> libavformat/options_table.h | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/libavformat/options_table.h b/libavformat/options_table.h > > >> index d5448e503f..19cb87ae4e 100644 > > >> --- a/libavformat/options_table.h > > >> +++ b/libavformat/options_table.h > > >> @@ -105,7 +105,7 @@ static const AVOption avformat_options[] = { > > >> {"format_whitelist", "List of demuxers that are allowed to be used", > > >> OFFSET(format_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, > > CHAR_MIN, > > >> CHAR_MAX, D }, > > >> {"protocol_whitelist", "List of protocols that are allowed to be used", > > >> OFFSET(protocol_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, > > CHAR_MIN, > > >> CHAR_MAX, D }, > > >> {"protocol_blacklist", "List of protocols that are not allowed to be > > >> used", OFFSET(protocol_blacklist), AV_OPT_TYPE_STRING, { .str = NULL }, > > >> CHAR_MIN, CHAR_MAX, D }, > > >> -{"max_streams", "maximum number of streams", OFFSET(max_streams), > > >> AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 0, INT_MAX, D }, > > >> +{"max_streams", "maximum number of streams", OFFSET(max_streams), > > >> AV_OPT_TYPE_INT, { .i64 = 100 }, 0, INT_MAX, D }, > > >> {NULL}, > > >> }; > > > > > > > > > Isn't this - in some way - an ABI break? > > > > > > Ronald > > > > The option in question was added an hour ago, so no. > > > I meant that legal input with 101 streams stop working. That's not crazy > IMO. IOW, 100 is kinda low. what value would you suggest as default ? [...]
2016.12.08. 19:36 keltezéssel, Michael Niedermayer írta: > On Thu, Dec 08, 2016 at 07:25:59PM +0100, wm4 wrote: >> On Thu, 8 Dec 2016 18:37:42 +0100 >> Michael Niedermayer <michael@niedermayer.cc> wrote: >> >>> Suggested-by: Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavformat/options_table.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/libavformat/options_table.h b/libavformat/options_table.h >>> index d5448e503f..19cb87ae4e 100644 >>> --- a/libavformat/options_table.h >>> +++ b/libavformat/options_table.h >>> @@ -105,7 +105,7 @@ static const AVOption avformat_options[] = { >>> {"format_whitelist", "List of demuxers that are allowed to be used", OFFSET(format_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, CHAR_MAX, D }, >>> {"protocol_whitelist", "List of protocols that are allowed to be used", OFFSET(protocol_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, CHAR_MAX, D }, >>> {"protocol_blacklist", "List of protocols that are not allowed to be used", OFFSET(protocol_blacklist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, CHAR_MAX, D }, >>> -{"max_streams", "maximum number of streams", OFFSET(max_streams), AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 0, INT_MAX, D }, >>> +{"max_streams", "maximum number of streams", OFFSET(max_streams), AV_OPT_TYPE_INT, { .i64 = 100 }, 0, INT_MAX, D }, >>> {NULL}, >>> }; >>> >> That seems awfully low. Why limit stream count to 100, > Is this too little for real world streams ? > what limit would not interfere with a positive user experience ? > 255 is acceptable for me. I saw several times more than 100 streams in one input/output but never more than 255. >> while allowing >> e.g. 2GB large font attachments? > theres no limit on attachments currently except the natural int size > we may want to have a tighter limit there too > > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel bb
L'octidi 18 frimaire, an CCXXV, Michael Niedermayer a écrit : > A. Is a heap limit for av_*alloc*() acceptable ? > B. Are case based limits acceptable ? No. This is the task of the operating system. > also even if C is choosen, a small set of limits on the main parameters > still is needed to allow efficient fuzzing, all issues reported > by oss-fuzz recently are "hangs" due to slow decoding, Then set a limit at the operating system level. Regards,
2016-12-08 18:37 GMT+01:00 Michael Niedermayer <michael@niedermayer.cc>: > -{"max_streams", "maximum number of streams", OFFSET(max_streams), AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 0, INT_MAX, D }, > +{"max_streams", "maximum number of streams", OFFSET(max_streams), AV_OPT_TYPE_INT, { .i64 = 100 }, 0, INT_MAX, D }, I wanted to suggest 1000 which is still a magnitude less than the provided crashing sample but 255 also sounds ok to me. Carl Eugen
2016-12-08 18:44 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: >> -{"max_streams", "maximum number of streams", OFFSET(max_streams), >> AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 0, INT_MAX, D }, >> +{"max_streams", "maximum number of streams", OFFSET(max_streams), >> AV_OPT_TYPE_INT, { .i64 = 100 }, 0, INT_MAX, D }, >> {NULL}, >> }; > > > Isn't this - in some way - an ABI break? Like ticket #5998? Carl Eugen
On 08.12.2016 20:57, Michael Niedermayer wrote: > This is something the community must decide. > A. Is a heap limit for av_*alloc*() acceptable ? I'm not sure that's a good idea, because it can't limit what other libraries called by libavcodec etc. allocate. > B. Are case based limits acceptable ? For the common cases, yes. However I doubt it's useful to limit every possible case. > C. document that libavcodec, libavformat and ffmpeg must be run as a > seperate process if the media comes from untrusted sources. Anything handling untrusted media files should have appropriate resource limits, anyway. Best regards, Andreas
On 08.12.2016 22:59, Carl Eugen Hoyos wrote: > 2016-12-08 18:37 GMT+01:00 Michael Niedermayer <michael@niedermayer.cc>: > >> -{"max_streams", "maximum number of streams", OFFSET(max_streams), AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 0, INT_MAX, D }, >> +{"max_streams", "maximum number of streams", OFFSET(max_streams), AV_OPT_TYPE_INT, { .i64 = 100 }, 0, INT_MAX, D }, > > I wanted to suggest 1000 which is still a magnitude less than the provided > crashing sample but 255 also sounds ok to me. Either value is OK. The important thing is that it's several orders of magnitude lower than INT_MAX. Best regards, Andreas
On Thu, Dec 08, 2016 at 09:47:53PM +0100, Nicolas George wrote: > L'octidi 18 frimaire, an CCXXV, Michael Niedermayer a écrit : > > A. Is a heap limit for av_*alloc*() acceptable ? > > B. Are case based limits acceptable ? > > No. This is the task of the operating system. > > > also even if C is choosen, a small set of limits on the main parameters > > still is needed to allow efficient fuzzing, all issues reported > > by oss-fuzz recently are "hangs" due to slow decoding, > > Then set a limit at the operating system level. You are misunderstanding the problem i think The goal of a fuzzer is to find bugs, crashes, undefined, bad things, OOM, hangs. If the code under test can allocate arbitrary amounts of memory and take arbitrary amounts of time in a significant number of non-bug cases then the fuzzer cannot reliably find the corresponding bugs. moving the threshold of where to declare something OOM or hang around will not solve this. blocking high resolution, high channel count, high stream count cases OTOH should improve the rate of false positives. also, secondary, resources spent on waiting for hangs to separate from slow decoding and real OOM to separate from cases just needing alot of memory, are resources that could be used for other things like fuzzing more seperate cases. but either way, iam the wrong one to disscuss changes to oss-fuzz with if you do have ideas that would improve it ... [...]
On Fri, 9 Dec 2016 02:44:11 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Thu, Dec 08, 2016 at 09:47:53PM +0100, Nicolas George wrote: > > L'octidi 18 frimaire, an CCXXV, Michael Niedermayer a écrit : > > > A. Is a heap limit for av_*alloc*() acceptable ? > > > B. Are case based limits acceptable ? > > > > No. This is the task of the operating system. > > > > > > also even if C is choosen, a small set of limits on the main parameters > > > still is needed to allow efficient fuzzing, all issues reported > > > by oss-fuzz recently are "hangs" due to slow decoding, > > > > Then set a limit at the operating system level. > > You are misunderstanding the problem i think > > The goal of a fuzzer is to find bugs, crashes, undefined, bad things, > OOM, hangs. > > If the code under test can allocate arbitrary amounts of memory and > take arbitrary amounts of time in a significant number of non-bug > cases then the fuzzer cannot reliably find the corresponding bugs. > > moving the threshold of where to declare something OOM or hang around > will not solve this. > blocking high resolution, high channel count, high stream count > cases OTOH should improve the rate of false positives. > > also, secondary, resources spent on waiting for hangs to separate from > slow decoding and real OOM to separate from cases just needing alot > of memory, are resources that could be used for other things like > fuzzing more seperate cases. > > but either way, iam the wrong one to disscuss changes to oss-fuzz with > if you do have ideas that would improve it ... > > [...] > I'm not sure why we need to accommodate the very special needs of fuzzers now, instead of fuzzers finding ways to avoid these situations. Fuzzers could for example just inject their own malloc impl into the process, that limits allocations, or something.
On Thu, 8 Dec 2016 20:57:05 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Thu, Dec 08, 2016 at 07:48:46PM +0100, wm4 wrote: > > On Thu, 8 Dec 2016 19:36:20 +0100 > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > On Thu, Dec 08, 2016 at 07:25:59PM +0100, wm4 wrote: > > > > On Thu, 8 Dec 2016 18:37:42 +0100 > > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > > > Suggested-by: Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > > --- > > > > > libavformat/options_table.h | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/libavformat/options_table.h b/libavformat/options_table.h > > > > > index d5448e503f..19cb87ae4e 100644 > > > > > --- a/libavformat/options_table.h > > > > > +++ b/libavformat/options_table.h > > > > > @@ -105,7 +105,7 @@ static const AVOption avformat_options[] = { > > > > > {"format_whitelist", "List of demuxers that are allowed to be used", OFFSET(format_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, CHAR_MAX, D }, > > > > > {"protocol_whitelist", "List of protocols that are allowed to be used", OFFSET(protocol_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, CHAR_MAX, D }, > > > > > {"protocol_blacklist", "List of protocols that are not allowed to be used", OFFSET(protocol_blacklist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, CHAR_MAX, D }, > > > > > -{"max_streams", "maximum number of streams", OFFSET(max_streams), AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 0, INT_MAX, D }, > > > > > +{"max_streams", "maximum number of streams", OFFSET(max_streams), AV_OPT_TYPE_INT, { .i64 = 100 }, 0, INT_MAX, D }, > > > > > {NULL}, > > > > > }; > > > > > > > > > > > > > That seems awfully low. Why limit stream count to 100, > > > > > > Is this too little for real world streams ? > > > what limit would not interfere with a positive user experience ? > > > > > > > > > > while allowing > > > > e.g. 2GB large font attachments? > > > > > > theres no limit on attachments currently except the natural int size > > > we may want to have a tighter limit there too > > > > > > > > > > This will lead to thousands of options tuning various limits that > > 1. nobody wants to use, 2. even if they want, will not find, and 3. are > > ugly and intrusive. > > > > And then there'll probably still be a way to easily OOM ffmpeg, and > > using a sandbox will still be superior over setting these options. > > Ive implemented the generic solution with one parameter in > [FFmpeg-devel] [PATCH 3/3] avutil/mem: Support limiting the heap usage > > which you and others dont like > i understand why this isnt liked and i understand why many seperate > options arent liked but > > Either of them is needed or FFmpeg cannot saftely be used via normal > lib mechanisms and must be run as a seperate process that the main > app has to expect to crash with out of memory. (if the input is > untrusted media) > > This is something the community must decide. > A. Is a heap limit for av_*alloc*() acceptable ? > B. Are case based limits acceptable ? > C. document that libavcodec, libavformat and ffmpeg must be run as a > seperate process if the media comes from untrusted sources. > > one of these options has to be choosen. > > also even if C is choosen, a small set of limits on the main parameters > still is needed to allow efficient fuzzing, all issues reported > by oss-fuzz recently are "hangs" due to slow decoding, > with the pixel max patch it instead of being slow hits this: > Picture size 512x65520 exceeds specified max pixel count 414719 > in the case i tried > > [...] No to A and B. Those are hacks - which means they work for some cases, but will cause more problems than they solve eventually. A has unacceptable global state, B would require more and more options to control uninteresting details to enforce memory allocation limits consequently and would never be complete. In theory it would be nice if you could pass some sort of custom allocation context to the libs, which would provide memory allocation callbacks. Then an API user could limit the amount of memory some ffmpeg component allocates. This would of course not be global state, but a context passed to AVCodecContext or individual functions. On one hand, this would be very intrusive - requiring all allocating functions to use such a context. On the other hand, we need something similar to remove the global av_log callback, so there might be an argument there. But in general this seems like something for per-OS or per-user limits. Btw. I doubt there's any serious service out there that does not run ffmpeg in a sandbox for processing user uploads and such.
Le nonidi 19 frimaire, an CCXXV, Michael Niedermayer a écrit : > > > A. Is a heap limit for av_*alloc*() acceptable ? > moving the threshold of where to declare something OOM or hang around > will not solve this. Yet this is your "A" proposal. Or am I misunderstanding you? Regards,
Hi, On Thu, Dec 8, 2016 at 7:03 PM, Andreas Cadhalpun < andreas.cadhalpun@googlemail.com> wrote: > On 08.12.2016 22:59, Carl Eugen Hoyos wrote: > > 2016-12-08 18:37 GMT+01:00 Michael Niedermayer <michael@niedermayer.cc>: > > > >> -{"max_streams", "maximum number of streams", OFFSET(max_streams), > AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 0, INT_MAX, D }, > >> +{"max_streams", "maximum number of streams", OFFSET(max_streams), > AV_OPT_TYPE_INT, { .i64 = 100 }, 0, INT_MAX, D }, > > > > I wanted to suggest 1000 which is still a magnitude less than the > provided > > crashing sample but 255 also sounds ok to me. > > Either value is OK. The important thing is that it's several orders of > magnitude lower than INT_MAX. On IRC, we discussed at what values OOM start occurring, which seems to be around 30k-60k, so from there I suggested a value like 10k or 5k. 1000 seems a little low but I think I can live with it (I doubt ATM I can come up with legit use cases that use 1000 streams). If people hit the limit (whatever value we choose), I would propose that we make the error message very specific, something similar to AVERROR_PATCHWELCOME. This way, people understand this is not a hard limitation and can be changed easily; fuzzers will obviously ignore this message. Ronald
On Fri, Dec 09, 2016 at 11:35:08AM +0100, Nicolas George wrote: > Le nonidi 19 frimaire, an CCXXV, Michael Niedermayer a écrit : > > > > A. Is a heap limit for av_*alloc*() acceptable ? > > > moving the threshold of where to declare something OOM or hang around > > will not solve this. > > Yet this is your "A" proposal. Or am I misunderstanding you? no, i missed that "A" alone would not solve the fuzzer aspect of this somewere in what i wrote ... if thats what you mean then you are correct "A" would mainly help for user app being crashed with the lib OOM "B" would mainly help the fuzzer issue but also in a more limited sense reduce the crash issue. Here even with seperate processes one probably prefers processes not maxing out their OS limits. A webpage with thousand images (not unlikely) would either have a thousand processes or one taking down all the image decoding. Whichever way its implemented more robustness against OOM and hangs is a good thing Also we could declare some decoders with capability flags as safe to be used in the same process. For example the simple image decoders can surely be made to be safe with just a max_pixel limit, and that should have users who would prefer not to need a seperate process. [...]
On Fri, 9 Dec 2016, Michael Niedermayer wrote: > On Thu, Dec 08, 2016 at 09:47:53PM +0100, Nicolas George wrote: >> L'octidi 18 frimaire, an CCXXV, Michael Niedermayer a écrit : >>> A. Is a heap limit for av_*alloc*() acceptable ? >>> B. Are case based limits acceptable ? >> >> No. This is the task of the operating system. >> > >>> also even if C is choosen, a small set of limits on the main parameters >>> still is needed to allow efficient fuzzing, all issues reported >>> by oss-fuzz recently are "hangs" due to slow decoding, >> >> Then set a limit at the operating system level. > > You are misunderstanding the problem i think > > The goal of a fuzzer is to find bugs, crashes, undefined, bad things, > OOM, hangs. > > If the code under test can allocate arbitrary amounts of memory and > take arbitrary amounts of time in a significant number of non-bug > cases then the fuzzer cannot reliably find the corresponding bugs. > > moving the threshold of where to declare something OOM or hang around > will not solve this. > blocking high resolution, high channel count, high stream count > cases OTOH should improve the rate of false positives. Then you should run the fuzzer with the limits you find optimal, no? Reducing the defaults which causes rare-but-existing files to stop working does not make much sense to me. I particularly don't like the stream count limits, because parsing possibly corrupted sources (e.g. mpegts) can easily generate a high number of mostly harmless empty streams as far as I remember. It just might make more sense to create a section in the documentation or a wiki page which describes that if you are working with untrusted files, you should use a sandbox, use system resource limits, and you might use these options as well. If we still want a default limit, that limit should be IMHO _insanely_ high, tens of thousands, not hundreds. Regards, Marton
On Fri, Dec 09, 2016 at 08:33:29PM +0100, Marton Balint wrote: > > On Fri, 9 Dec 2016, Michael Niedermayer wrote: > > >On Thu, Dec 08, 2016 at 09:47:53PM +0100, Nicolas George wrote: > >>L'octidi 18 frimaire, an CCXXV, Michael Niedermayer a écrit : > >>>A. Is a heap limit for av_*alloc*() acceptable ? > >>>B. Are case based limits acceptable ? > >> > >>No. This is the task of the operating system. > >> > > > >>>also even if C is choosen, a small set of limits on the main parameters > >>>still is needed to allow efficient fuzzing, all issues reported > >>>by oss-fuzz recently are "hangs" due to slow decoding, > >> > >>Then set a limit at the operating system level. > > > >You are misunderstanding the problem i think > > > >The goal of a fuzzer is to find bugs, crashes, undefined, bad things, > >OOM, hangs. > > > >If the code under test can allocate arbitrary amounts of memory and > >take arbitrary amounts of time in a significant number of non-bug > >cases then the fuzzer cannot reliably find the corresponding bugs. > > > >moving the threshold of where to declare something OOM or hang around > >will not solve this. > >blocking high resolution, high channel count, high stream count > >cases OTOH should improve the rate of false positives. > > Then you should run the fuzzer with the limits you find optimal, no? > > Reducing the defaults which causes rare-but-existing files to stop > working does not make much sense to me. I particularly don't like > the stream count limits, because parsing possibly corrupted sources > (e.g. mpegts) can easily generate a high number of mostly harmless > empty streams as far as I remember. > > It just might make more sense to create a section in the > documentation or a wiki page which describes that if you are working > with untrusted files, you should use a sandbox, use system resource > limits, and you might use these options as well. > > If we still want a default limit, that limit should be IMHO > _insanely_ high, tens of thousands, not hundreds. The OOM case that ive debuged was in the tens of thousands so a limit in that range will likely not stop much [...]
2016-12-09 12:56 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > On IRC, we discussed at what values OOM start occurring, which seems to be > around 30k-60k This is not true, why do you think so? Carl Eugen
2016-12-09 20:33 GMT+01:00 Marton Balint <cus@passwd.hu>: > If we still want a default limit, that limit should be IMHO > _insanely_ high, tens of thousands, not hundreds. Do you have a file that has hundreds of streams? Note that I suggested 1000 as limit. Carl Eugen
On Sat, 10 Dec 2016, Carl Eugen Hoyos wrote: > 2016-12-09 20:33 GMT+01:00 Marton Balint <cus@passwd.hu>: >> If we still want a default limit, that limit should be IMHO >> _insanely_ high, tens of thousands, not hundreds. > > Do you have a file that has hundreds of streams? No I don't, but that does not mean that nobody has such a file, and we should not support it by default. Also I mentioned that corrupted sources can generate dummy streams. > Note that I suggested 1000 as limit. Even 1000 does not seems so unthinkable to me, so I suggested tens of thousands. Regards, Marton
2016-12-10 13:33 GMT+01:00 Marton Balint <cus@passwd.hu>: > > On Sat, 10 Dec 2016, Carl Eugen Hoyos wrote: > >> 2016-12-09 20:33 GMT+01:00 Marton Balint <cus@passwd.hu>: >>> >>> If we still want a default limit, that limit should be IMHO >>> _insanely_ high, tens of thousands, not hundreds. >> >> Do you have a file that has hundreds of streams? > > No I don't, but that does not mean that nobody has such a file, > and we should not support it by default. Also I mentioned that > corrupted sources can generate dummy streams. I thought that I am normally on the user's (and broken stream's) side (and I believe that 16k is not so unusual) but 100 streams seem already unrealistic to me, 1000 even more so. >> Note that I suggested 1000 as limit. > > Even 1000 does not seems so unthinkable to me, so I > suggested tens of thousands. Then we can leave the option at INT_MAX imo. Carl Eugen
Hi, On Sat, Dec 10, 2016 at 7:11 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2016-12-09 12:56 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > > > On IRC, we discussed at what values OOM start occurring, which seems to > be > > around 30k-60k > > This is not true, why do you think so? http://lists.ffmpeg.org/pipermail/ffmpeg-devel-irc/2016-December/003980.html : [21:01:43 CET] <BBB> michaelni: can you provide a graph with memory usage depending on the number of streams? [21:01:52 CET] <BBB> michaelni: I think based on that we can define a sensible default [21:02:25 CET] <BBB> if we use 100GB with 10 streams, we should probably limit it to <10, but if memory usage doesn t change much between 10k and 100k streams, maybe a limit of 1M streams is acceptable [21:02:35 CET] <BBB> michaelni: assuming the goal is to prevent OOM scenarios [21:11:41 CET] <michaelni> i think one actual OOM case was with around 30k-60k streams, the exact usage likely depends on the demuxer Ronald
On Fri, Dec 09, 2016 at 06:56:53AM -0500, Ronald S. Bultje wrote: > Hi, > > On Thu, Dec 8, 2016 at 7:03 PM, Andreas Cadhalpun < > andreas.cadhalpun@googlemail.com> wrote: > > > On 08.12.2016 22:59, Carl Eugen Hoyos wrote: > > > 2016-12-08 18:37 GMT+01:00 Michael Niedermayer <michael@niedermayer.cc>: > > > > > >> -{"max_streams", "maximum number of streams", OFFSET(max_streams), > > AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 0, INT_MAX, D }, > > >> +{"max_streams", "maximum number of streams", OFFSET(max_streams), > > AV_OPT_TYPE_INT, { .i64 = 100 }, 0, INT_MAX, D }, > > > > > > I wanted to suggest 1000 which is still a magnitude less than the > > provided > > > crashing sample but 255 also sounds ok to me. > > > > Either value is OK. The important thing is that it's several orders of > > magnitude lower than INT_MAX. > > > On IRC, we discussed at what values OOM start occurring, which seems to be > around 30k-60k, so from there I suggested a value like 10k or 5k. 1000 > seems a little low but I think I can live with it (I doubt ATM I can come > up with legit use cases that use 1000 streams). > > If people hit the limit (whatever value we choose), I would propose that we > make the error message very specific, something similar to > AVERROR_PATCHWELCOME. This way, people understand this is not a hard > limitation and can be changed easily; fuzzers will obviously ignore this > message. new patchset with higher limit, error messsage and reference to the CVE# posted [...]
2016-12-10 14:07 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > Hi, > > On Sat, Dec 10, 2016 at 7:11 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> > wrote: > >> 2016-12-09 12:56 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: >> >> > On IRC, we discussed at what values OOM start occurring, which >> > seems to be around 30k-60k >> >> This is not true, why do you think so? > > http://lists.ffmpeg.org/pipermail/ffmpeg-devel-irc/2016-December/003980.html http://www.openwall.com/lists/oss-security/2016/12/08/1 Iiuc (which isn't sure at all) this is about 26000 streams. Carl Eugen
Hi, On Sat, Dec 10, 2016 at 5:24 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2016-12-10 14:07 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > > Hi, > > > > On Sat, Dec 10, 2016 at 7:11 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> > > wrote: > > > >> 2016-12-09 12:56 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > >> > >> > On IRC, we discussed at what values OOM start occurring, which > >> > seems to be around 30k-60k > >> > >> This is not true, why do you think so? > > > > http://lists.ffmpeg.org/pipermail/ffmpeg-devel-irc/ > 2016-December/003980.html > > http://www.openwall.com/lists/oss-security/2016/12/08/1 > Iiuc (which isn't sure at all) this is about 26000 streams. And how does that fundamentally change the discussion? The question was about orders of magnitude (think powers of 10), so you just changes the answer from log10(30k)=4.48 to log10(26k)=4.41. In both cases, a sensible limit is something like exp10(3) or exp10(4), but exp10(2) is not necessary IMHO, Ronald
2016-12-10 23:36 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > Hi, > > On Sat, Dec 10, 2016 at 5:24 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> > wrote: > >> 2016-12-10 14:07 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: >> > Hi, >> > >> > On Sat, Dec 10, 2016 at 7:11 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> >> > wrote: >> > >> >> 2016-12-09 12:56 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: >> >> >> >> > On IRC, we discussed at what values OOM start occurring, which >> >> > seems to be around 30k-60k >> >> >> >> This is not true, why do you think so? >> > >> > http://lists.ffmpeg.org/pipermail/ffmpeg-devel-irc/ >> 2016-December/003980.html >> >> http://www.openwall.com/lists/oss-security/2016/12/08/1 >> Iiuc (which isn't sure at all) this is about 26000 streams. > > And how does that fundamentally change the discussion? The question was > about orders of magnitude (think powers of 10), so you just changes the > answer from log10(30k)=4.48 to log10(26k)=4.41. > In both cases, a sensible > limit is something like exp10(3) or exp10(4), but exp10(2) is not necessary > IMHO, Isn't this exactly what I requested in this very thread? Carl Eugen
On Sat, Dec 10, 2016 at 11:36 PM, Ronald S. Bultje <rsbultje@gmail.com> wrote: > Hi, > > On Sat, Dec 10, 2016 at 5:24 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> > wrote: > >> 2016-12-10 14:07 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: >> > Hi, >> > >> > On Sat, Dec 10, 2016 at 7:11 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> >> > wrote: >> > >> >> 2016-12-09 12:56 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: >> >> >> >> > On IRC, we discussed at what values OOM start occurring, which >> >> > seems to be around 30k-60k >> >> >> >> This is not true, why do you think so? >> > >> > http://lists.ffmpeg.org/pipermail/ffmpeg-devel-irc/ >> 2016-December/003980.html >> >> http://www.openwall.com/lists/oss-security/2016/12/08/1 >> Iiuc (which isn't sure at all) this is about 26000 streams. > > > And how does that fundamentally change the discussion? The question was > about orders of magnitude (think powers of 10), so you just changes the > answer from log10(30k)=4.48 to log10(26k)=4.41. In both cases, a sensible > limit is something like exp10(3) or exp10(4), but exp10(2) is not necessary > IMHO, > If we must have a pre-defined limit (which I still find a questionable choice to limit resource usage in general, OOM itself is not an inherently "evil" thing, perfectly valid files could cause OOM just due to sheer size and if OOM is a crash then something else needs fixing to check mallocs), then 1000 (ie. exp10(3)) (or 1024 because we like power of 2 numbers) is probably going to be enough for all sorts of valid files. Even odd mpegts files with loads of empty streams in empty programs would never reach that. - Hendrik
On Sun, 11 Dec 2016 15:29:32 +0100 Hendrik Leppkes <h.leppkes@gmail.com> wrote: > On Sat, Dec 10, 2016 at 11:36 PM, Ronald S. Bultje <rsbultje@gmail.com> wrote: > > Hi, > > > > On Sat, Dec 10, 2016 at 5:24 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> > > wrote: > > > >> 2016-12-10 14:07 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > >> > Hi, > >> > > >> > On Sat, Dec 10, 2016 at 7:11 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> > >> > wrote: > >> > > >> >> 2016-12-09 12:56 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > >> >> > >> >> > On IRC, we discussed at what values OOM start occurring, which > >> >> > seems to be around 30k-60k > >> >> > >> >> This is not true, why do you think so? > >> > > >> > http://lists.ffmpeg.org/pipermail/ffmpeg-devel-irc/ > >> 2016-December/003980.html > >> > >> http://www.openwall.com/lists/oss-security/2016/12/08/1 > >> Iiuc (which isn't sure at all) this is about 26000 streams. > > > > > > And how does that fundamentally change the discussion? The question was > > about orders of magnitude (think powers of 10), so you just changes the > > answer from log10(30k)=4.48 to log10(26k)=4.41. In both cases, a sensible > > limit is something like exp10(3) or exp10(4), but exp10(2) is not necessary > > IMHO, > > > > If we must have a pre-defined limit (which I still find a questionable > choice to limit resource usage in general, OOM itself is not an > inherently "evil" thing, perfectly valid files could cause OOM just > due to sheer size and if OOM is a crash then something else needs > fixing to check mallocs), +1 > then 1000 (ie. exp10(3)) (or 1024 because we > like power of 2 numbers) is probably going to be enough for all sorts > of valid files. Even odd mpegts files with loads of empty streams in > empty programs would never reach that. Subtitle streams, font attachments, attached images, and other things could also inflate the number of streams, so 1000 seems a bit low.
Le primidi 21 frimaire, an CCXXV, wm4 a écrit : > Subtitle streams, font attachments, attached images, and other things > could also inflate the number of streams, so 1000 seems a bit low. As a side note, attachments are presented in the streams' extradata, and as such loaded immediately. A player would have, for each font, one copy in the streams' codecpar, one copy in the streams codec context for compatibility, and one copy in the memory of the font rasterizer library. We need to change that. Regards,
On Sun, 11 Dec 2016 16:19:03 +0100 Nicolas George <george@nsup.org> wrote: > Le primidi 21 frimaire, an CCXXV, wm4 a écrit : > > Subtitle streams, font attachments, attached images, and other things > > could also inflate the number of streams, so 1000 seems a bit low. > > As a side note, attachments are presented in the streams' extradata, and > as such loaded immediately. > > A player would have, for each font, one copy in the streams' codecpar, > one copy in the streams codec context for compatibility, and one copy in > the memory of the font rasterizer library. > > We need to change that. > > Regards, > Attachment contents are AVPackets, and thus reference counted (or can be). It's up to a player what to do with that, not libavformat.
Le primidi 21 frimaire, an CCXXV, wm4 a écrit :
> Attachment contents are AVPackets
That is true for attached pictures but not other kinds of attachments.
Regards,
On Sun, 11 Dec 2016 16:50:00 +0100 Nicolas George <george@nsup.org> wrote: > Le primidi 21 frimaire, an CCXXV, wm4 a écrit : > > Attachment contents are AVPackets > > That is true for attached pictures but not other kinds of attachments. > > Regards, > Right. I looked and font data is in extradata. That doesn't seem very ideal. Rather than making extradata refcounted, this should probably be a new AVPacket* field.
Le primidi 21 frimaire, an CCXXV, wm4 a écrit : > Right. I looked and font data is in extradata. That doesn't seem very > ideal. Rather than making extradata refcounted, this should probably be > a new AVPacket* field. It feels like a round peg in a square hole: use the existing structures even when they are not really convenient. In particular, you still end up loading all the attachments into memory even if they are not needed at all. They can be freed later, but still are loaded, wasting time and resources. I think a new function av_get_attachment_data() would be much more convenient, and a new AVInputFormat method to let muxers load attachment data only when requested. Regards,
On Sun, Dec 11, 2016 at 03:29:32PM +0100, Hendrik Leppkes wrote: > On Sat, Dec 10, 2016 at 11:36 PM, Ronald S. Bultje <rsbultje@gmail.com> wrote: > > Hi, > > > > On Sat, Dec 10, 2016 at 5:24 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> > > wrote: > > > >> 2016-12-10 14:07 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > >> > Hi, > >> > > >> > On Sat, Dec 10, 2016 at 7:11 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> > >> > wrote: > >> > > >> >> 2016-12-09 12:56 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > >> >> > >> >> > On IRC, we discussed at what values OOM start occurring, which > >> >> > seems to be around 30k-60k > >> >> > >> >> This is not true, why do you think so? > >> > > >> > http://lists.ffmpeg.org/pipermail/ffmpeg-devel-irc/ > >> 2016-December/003980.html > >> > >> http://www.openwall.com/lists/oss-security/2016/12/08/1 > >> Iiuc (which isn't sure at all) this is about 26000 streams. > > > > > > And how does that fundamentally change the discussion? The question was > > about orders of magnitude (think powers of 10), so you just changes the > > answer from log10(30k)=4.48 to log10(26k)=4.41. In both cases, a sensible > > limit is something like exp10(3) or exp10(4), but exp10(2) is not necessary > > IMHO, > > > > If we must have a pre-defined limit (which I still find a questionable > choice to limit resource usage in general, OOM itself is not an > inherently "evil" thing, perfectly valid files could cause OOM just > due to sheer size and if OOM is a crash then something else needs > fixing to check mallocs), then 1000 (ie. exp10(3)) (or 1024 because we i think with "crash" it was meant that ffmpeg is killed by the OOM killer [...]
diff --git a/libavformat/options_table.h b/libavformat/options_table.h index d5448e503f..19cb87ae4e 100644 --- a/libavformat/options_table.h +++ b/libavformat/options_table.h @@ -105,7 +105,7 @@ static const AVOption avformat_options[] = { {"format_whitelist", "List of demuxers that are allowed to be used", OFFSET(format_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, CHAR_MAX, D }, {"protocol_whitelist", "List of protocols that are allowed to be used", OFFSET(protocol_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, CHAR_MAX, D }, {"protocol_blacklist", "List of protocols that are not allowed to be used", OFFSET(protocol_blacklist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, CHAR_MAX, D }, -{"max_streams", "maximum number of streams", OFFSET(max_streams), AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 0, INT_MAX, D }, +{"max_streams", "maximum number of streams", OFFSET(max_streams), AV_OPT_TYPE_INT, { .i64 = 100 }, 0, INT_MAX, D }, {NULL}, };
Suggested-by: Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/options_table.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)