[FFmpeg-devel] [PATCH 0/3] Finish new iteration APIs

[wm4]

On Thu, 22 Feb 2018 16:28:56 +0100
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Wed, Feb 21, 2018 at 09:02:40PM +0100, wm4 wrote:
> > On Wed, 21 Feb 2018 19:14:59 +0100
> > Michael Niedermayer <michael at niedermayer.cc> wrote:
> >   
> > > On Wed, Feb 21, 2018 at 09:27:02AM +0100, wm4 wrote:  
> > > > On Tue, 20 Feb 2018 21:45:12 +0100
> > > > Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > >     
> > > > > On Tue, Feb 20, 2018 at 06:28:20PM +0100, wm4 wrote:    
> > > > > > On Tue, 20 Feb 2018 17:30:32 +0100
> > > > > > Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > > > >       
> > > > > > > On Tue, Feb 20, 2018 at 10:17:02AM -0300, James Almer wrote:      
> > > > > > > > On 2/20/2018 9:21 AM, wm4 wrote:        
> > > > > > > > > On Tue, 20 Feb 2018 08:47:51 -0300
> > > > > > > > > James Almer <jamrial at gmail.com> wrote:    
> > > [...]  
> > > > > > And for the 100th time: the new API is completely orthogonal to
> > > > > > allowing user-registered components. Since nobody could actually use
> > > > > > the API before, it's no problem to drop the old APIs now, and to add
> > > > > > actually working API once the other, much much much bigger problems are
> > > > > > solved.
> > > > > > 
> > > > > > Even if you argue that keeping the linked list is absolutely necessary,
> > > > > > the new API could support a linked list too.
> > > > > >       
> > > > > > > is it the ff_* symbols you are thinking of ?      
> > > > > > 
> > > > > > ff_ symbols are private.
> > > > > >       
> > > > > > > This is a problem for an existing API it is not a problem for a new API as
> > > > > > > we can change the symbols if they are intended to be used for individual
> > > > > > > component registration.
> > > > > > > The whole discussion is about designing a new API. So any limitation of
> > > > > > > an existing API can be changed.
> > > > > > > 
> > > > > > > There also should be no need to call register_all in the existing API,
> > > > > > > and there cannot be such a problem in a new design because it would be
> > > > > > > a new design you wouldnt design it to "not work".      
> > > > > > 
> > > > > > You're just being contradictory all across the board here. In other
> > > > > > places you're claiming this register mechanism is needed for
> > > > > > security or to make it possible to eliminate unused components when
> > > > > > static linking. But if all components are already registered without
> > > > > > doing anything, how is that supposed to work?      
> > > > > 
> > > > > If an application wants to register all, it calls the register_all()
> > > > > function
> > > > > If an application wants to register a subset it registers just that
> > > > > subset and does not call register_all    
> > > > 
> > > > But you just said on your mail "There also should be no need to call
> > > > register_all in the existing API,". How is that supposed to mesh with 
> > > > "If an application wants to register a subset it registers just that
> > > > subset and does not call register_all".    
> > > 
> > > I see no contradiction between what you quote, if that is what you meant:
> > > "There also should be no need to call register_all in the existing API,"
> > > and
> > > "If an application wants to register a subset it registers just that subset and does not call register_all"  
> > 
> > If there's no contradiction, it implies that either the first register
> > call will implicitly unregister all "automatically" registered
> > components, or the code has to explicitly unregister components. but in
> > both cases this would still require linking all components even if you
> > use static linking.
> > 
> > So you have the following possibilities:
> > - register_all needs to be called
> >   - contradicts your claim it doesn't need to be called  
> 
> > - register_all needs not to be called
> >   - all components are already registered implicitly  
> 
> why would not calling register_all(), register all components implicitly ?

So what did you mean by "There also should be no need to call
register_all in the existing API,"?

> 
> >     => static linking will pull in all components  
> >   - but the application can register a subset of components it needs
> >     (your claim)  
> >     => probably means components get unregistered as soon as the  
> >        application registers a subset (or some really tricky linker
> >        magic that removes the auto registration code if manual
> >        registration functions are used?)  
> 
> This sure would be possible but i dont think this complexity is needed
> 
> 
> > 
> > Well, which is it? But you could also just explicitly say what you have
> > in mind.  
> 
> Really, the very simple thing that register() registers one component,
> register_all() registeres all, and then there is a 3rd function to
> stop all further registration that could be called register_lock() or
> register_disable() which returns the list of registered components after
> further registration is disabled.

OK, but I don't want to call register_all(), and I don't want to have
to think about globally visible state, and I don't want to have to
document such issues in my own libavcodec using library.

Really, you should just accept that such things are completely
unacceptable in a library, even if you keep coming up with arguments
why it's supposedly not this way.

We're removing some badness here, yet you're fighting it for no reason.
At best you could defend the security argument, but that is very fuzzy
too. (Probably because everyone tends to accept even bad security
mitigations.)

> A library cannot register more components after that and if it somehow
> managed to register a component before that is detected by the user
> application which then exits with an error message explaining the user
> that the security model is not compatible with a library loaded.
> And that the user either has to add the specific listed components to
> the components the application registers or to disable secure mode or
> to do something about the library that registered the component.
> 
> This incompatibility may seem a problem but this is exactly what the
> intend of this security layer is. To NEVER allow ANY code in a
> process to use a component except the ones the application listed as
> allowed.

I don't think that works. If there are "unknown" libavcodec uses in the
process that could take untrusted input data you've probably already
lost. On the other hand, you can set whitelists on any AVFormatContext
or whatever, because you control it.

> We have IIRC like 400 decoders, most are odd/rare/fringe and its a valid
> request to hard disable these at runtime process wide.

Maybe we should just disable them by default, instead of forcing the
user to play security russian roulette with them?

> a browser or other security sensitive application that doesnt need them
> would not want any lib it uses to open most of these fringe decoders. 
> 
> 
> applications which are not that strict about security would not use
> register_disable() and would rely on just whitelists.
> They would then have to accept that a 3rd party lib that uses libavcodec
> libavformat or libavfilter may do things they do not expect.
> Whitelists wont protect them from a alsa or gstreamer plugin as the
> application likely cannot that easily pass a whitelist throigh these
> frameworks to our libs and even if it can it first has to know that
> it has to, which it may not know.

gstreamer needs its own security mechanisms. ALSA doesn't even feed
untrusted input data to any decoders.

> 
> >   
> > >   
> > > > 
> > > > But indeed one goal was that applications don't have to call the
> > > > completely pointless register_all functions.    
> > >   
> > > >     
> > > > > And even the documentation says this currently:
> > > > > /**
> > > > >  * Register all the codecs, parsers and bitstream filters which were enabled at
> > > > >  * configuration time. If you do not call this function you can select exactly
> > > > >  * which formats you want to support, by using the individual registration
> > > > >  * functions.
> > > > >  *    
> > > > 
> > > > Explain how that could actually work with dynamic linking.    
> > > 
> > > If you see an issue with dynamic linking, please
> > > explain what you mean, and i will try to show that it does not
> > > affect a newly designed API.  
> > 
> > ff_ symbols are not exported across DSO/DLL boundaries. You seem to be
> > mixing the discussion about old and new APIs, yes. Part of your
> > argument was that it breaks things that _were_ possible, but as I have
> > been telling you, selective registration of components could not be
> > used with dynamic linking.
> >   
> > > Fundamentally, one can of course proof this simply by showing that
> > > other libs do have working registration style functions even when
> > > dynamically linked.  
> > 
> > Of course that's possible, with a new API. But we haven't seen any such
> > API. You haven't described any concrete ideas. Your idea might be
> > changing the component structs to be prefixed with av_ instead of ff_
> > to get them exported or so. I don't think you've described any of that.  
> 
> I havent really described details before because i am happy about any 
> implementation that achives all goals. And i do not want to bikeshed
> around on the details, my point is mainly just that 
> 
> having only a subset of components enabled is possible
> and that it makes sense to support it

Yes, it's called whitelists. You implemented them.

> 
> 
> Yes, changing the symbols to av_* is an option.
> 
> Thanks
> 
> [...]