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

[wm4]

On Thu, 8 Dec 2016 20:57:05 +0100
Michael Niedermayer <michael at 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 at 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 at niedermayer.cc> wrote:
> > > >     
> > > > > Suggested-by: Andreas Cadhalpun <andreas.cadhalpun at googlemail.com>
> > > > > Signed-off-by: Michael Niedermayer <michael at 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.