[FFmpeg-devel] avformat/options_table: Set the default maximum number of streams to 100

Submitted by Michael Niedermayer on Dec. 8, 2016, 5:37 p.m.

Details

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

Commit Message

Michael Niedermayer Dec. 8, 2016, 5:37 p.m.
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(-)

Comments

Ronald S. Bultje Dec. 8, 2016, 5:44 p.m.
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
James Almer Dec. 8, 2016, 6:25 p.m.
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.
wm4 Dec. 8, 2016, 6:25 p.m.
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?
Michael Niedermayer Dec. 8, 2016, 6:36 p.m.
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
wm4 Dec. 8, 2016, 6:48 p.m.
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.
Ronald S. Bultje Dec. 8, 2016, 7:05 p.m.
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
Michael Niedermayer Dec. 8, 2016, 7:57 p.m.
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

[...]
Michael Niedermayer Dec. 8, 2016, 7:57 p.m.
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 ?

[...]
Bodecs Bela Dec. 8, 2016, 8:09 p.m.
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
Nicolas George Dec. 8, 2016, 8:47 p.m.
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,
Carl Eugen Hoyos Dec. 8, 2016, 9:59 p.m.
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
Carl Eugen Hoyos Dec. 8, 2016, 10 p.m.
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
Andreas Cadhalpun Dec. 8, 2016, 11:52 p.m.
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
Andreas Cadhalpun Dec. 9, 2016, 12:03 a.m.
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
Michael Niedermayer Dec. 9, 2016, 1:44 a.m.
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 ...

[...]
wm4 Dec. 9, 2016, 9:04 a.m.
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.
wm4 Dec. 9, 2016, 9:10 a.m.
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.
Nicolas George Dec. 9, 2016, 10:35 a.m.
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,
Ronald S. Bultje Dec. 9, 2016, 11:56 a.m.
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
Michael Niedermayer Dec. 9, 2016, 2:04 p.m.
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.

[...]
Marton Balint Dec. 9, 2016, 7:33 p.m.
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
Michael Niedermayer Dec. 9, 2016, 8:32 p.m.
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

[...]
Carl Eugen Hoyos Dec. 10, 2016, 12:11 p.m.
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
Carl Eugen Hoyos Dec. 10, 2016, 12:12 p.m.
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
Marton Balint Dec. 10, 2016, 12:33 p.m.
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
Carl Eugen Hoyos Dec. 10, 2016, 12:36 p.m.
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
Ronald S. Bultje Dec. 10, 2016, 1:07 p.m.
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
Michael Niedermayer Dec. 10, 2016, 7:20 p.m.
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

[...]
Carl Eugen Hoyos Dec. 10, 2016, 10:24 p.m.
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
Ronald S. Bultje Dec. 10, 2016, 10:36 p.m.
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
Carl Eugen Hoyos Dec. 11, 2016, 2:14 p.m.
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
Hendrik Leppkes Dec. 11, 2016, 2:29 p.m.
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
wm4 Dec. 11, 2016, 3:13 p.m.
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.
Nicolas George Dec. 11, 2016, 3:19 p.m.
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,
wm4 Dec. 11, 2016, 3:47 p.m.
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.
Nicolas George Dec. 11, 2016, 3:50 p.m.
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,
wm4 Dec. 11, 2016, 4:04 p.m.
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.
Nicolas George Dec. 11, 2016, 4:09 p.m.
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,
Michael Niedermayer Dec. 11, 2016, 9:48 p.m.
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


[...]

Patch hide | download patch | download mbox

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},
 };