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

[wm4]

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:
> > > > >     
> > > > >>> It has fully achieved its objectives. There's no more visible global
> > > > >>> mutable state, and the actual global mutable state has been reduced to
> > > > >>> less than 5 codec entries.      
> > > > >>
> > > > >> It's true that after the deprecation period they will effectively be the
> > > > >> only ones still mutable, but shouldn't the objective be to cover them all?    
> > > > > 
> > > > > That would be nice. But this has been discussed before. No kind of
> > > > > different registration API could fix this issue either, unless we start
> > > > > mallocing AVCodec structs and require the user to free them, or
> > > > > something equally absurd. The proper solution for this specific issue
> > > > > would be making a new API that somehow replaces AVCodec.pix_fmts.
> > > > >     
> > > > >>>
> > > > >>> Why are we discussing this _again_?      
> > > > >>
> > > > >> Because a drawback has been found, namely that link time optimization
> > > > >> using static libraries can't remove "non registered" modules anymore,
> > > > >> and now depends fully on what's enabled at configure time.
> > > > >> Ideally, a better registration based API that follows what i described
> > > > >> above should be designed with care.    
> > > > > 
> > > > > Oh yeah, bikeshed attack by Michael. As it was said a few thousand times
> > > > > already, public API users could NOT use the registration stuff to
> > > > > register only specific components at runtime. So they have to use the
> > > > > register_all variants, which pull in _all_ components even with static
> > > > > linking.    
> > > > 
> > > > Oh, i assumed it was possible to use avcodec_register() on a manual
> > > > basis in a proper API usage scenario, but i see now that's not the case.    
> > > 
> > > of course its possible to use avcodec_register() on a manual basis in a 
> > > proper API usage scenario, why would it not be ?
> > > 
> > > why do you think this function exists or was written ?  
> > 
> > I don't know, but it didn't make any sense. And wouldn't have made for
> > years to come.  
> 
> thats backward
> 
> if you look at git history the very first checkin 
> commit 9aeeeb63f7e1ab7b0b7bb839a5f258667a2d2d78 (HEAD)
> Author: Fabrice Bellard <fabrice at bellard.org>
> Date:   Wed Dec 20 00:02:47 2000 +0000
> 
>     Initial revision
> 
> has code like this:
> 
> ffmpeg.c:    register_avencoder(&ac3_encoder);
> ffmpeg.c:    register_avencoder(&mp2_encoder);
> ffmpeg.c:    register_avencoder(&mpeg1video_encoder);
> ffmpeg.c:    register_avencoder(&h263_encoder);
> 
> Here the application did use the register API to selectivly register
> codecs.

That's before FFmpeg has a stable API or even just ABI. It took a
decade or so to get there. I also think we didn't find a single
application that tried to do this these days. Maybe the fact that it
simply didn't work with dynamic linking also mattered.

> 
> 
> > 
> > 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".

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.

> 
> >   
> > >   
> > > > 
> > > > Nevermind then, my argument was focused on preventing losing some link
> > > > time optimization for static libraries assuming proper API usage.
> > > >     
> > > > > 
> > > > > And that can't be made with dynamic linking either. If you statically
> > > > > link anyway, you probably control everything, and you can configure this
> > > > > stuff at compile time.
> > > > > 
> > > > > Then I guess there's this very special case where a fuzzer statically
> > > > > links to libavcodec, and registers only a few components (technically
> > > > > violating the API and by guessing the component symbol name), and
> > > > > without calling the register_all functions. This would make the
> > > > > resulting executable much smaller, which is apparently important
> > > > > because Google (who runs the fuzzers for their oss-fuzz project) is too
> > > > > poor (?) to host all the statically linked fuzzers, _or_ the whitelist
> > > > > stuff is not enough to trick the fuzzer into not trying to fuzz the
> > > > > wrong code. In addition, it's apparently infeasible to just build
> > > > > every fuzzer with a separate libavcodec. (Not sure about the details, it
> > > > > was something like this.)
> > > > > 
> > > > > Those requirements are really quite nebulous. I don't know why we even
> > > > > should care - surely whoever does this will find a solution that works
> > > > > for them. For example they could apply a small patch that makes the
> > > > > codec_list[] symbol non-static non-const, which would allow them to
> > > > > overwrite it (i.e. deleting unneeded entries). It's a really simple
> > > > > solution that took me 5 minutes to come up with.    
> > > > 
> > > > Something like this is probably the best solution for the fuzzer issue, yes.    
> > > 
> > > This basically says one should fork ffmpeg if one has problems with the new API  
> > 
> > What? Jesus christ, stop being so obstinate and playing dumb just to
> > try to prove your non-existent point.
> > 
> > Applying a patch for a very special setup is not a problem.   
> 
> 
> > Most of the
> > serious projects using FFmpeg apply custom patches anyway.  
> 
> And this should maybe give us a hint that we have a problem that we
> should solve.

No, because they have special requirements and do things that wouldn't
make sense to do upstream, or want to do things that would not be
reasonable to do upstream.

> 
> > 
> > And you very well know that the solution I proposed (which is probably
> > just 1 of dozens of possible ideas) could be trivially changed and
> > applied upstream: prefix codec_list with ff_ and make it non-const and
> > non-static.

No reply... But I just solved your big problem?

> > > Thats a very chilling response to be honest in a discussion about that APIs
> > > design. More so as this is at a time we still can change the API.  
> > 
> > You've brought up the same tired arguments over and over, and every
> > time I've debunked them, and every time you ignored it. Chilling my
> > ass, your behavior is offensive.
> >   
> 
> > > and anyone who wants to only register a subset of components (due to security)
> > > has to either link to a seperate binary (which is  disallowed in some 
> > > major distributions which mandate shared libs without exception IIRC so that
> > > sounds great but simple isnt possible)
> > > or just not use FFmpeg or fork it or use a fork or just forget about only
> > > registering a subset.  
> > 
> > This is so over the place.
> > 
> > One time you mentioned static linking. This doesn't have anything to do
> > with distros. You can disable unwanted components at compile time. I've
> > mentioned that in my previous mail too, but no reply.  
> 
> Some distros do not allow multiple custom builds of libs to be packaged for
> installation at the same time.

But I'm talking about static linking here. That doesn't have to do
anything with static linking. _You_ brought up static linking
(something about saving space if not all codecs are used), but now
you're suddenly arguing about a setting where static linking is
forbidden. (???)

> And that has very good reason. Even if its allowed its very bad for a
> system to carry dozends of differfently custom patched libs or builds of libs.
> 
> So compile time disabling is not a solution that would work for the general
> case.

But for the static linking case, which you brought up yourself! Am I
talking to a wall?

> I tend to avoid arguing over your statments as it has in the past
> led to an escalation of attacks, insults and that helps noone.
> But it seems you are upset when i dont reply, and you are upset if i
> do reply.

Littering me with obstinate non-sequitur replies and inconsistent
arguments, and then insinuating that I got angry, and using that to
claim that I'm acting unreasonable is not a nice way to discuss at all.
Are you sure you're even interested in factual discussion?

For example take this case: you claimed something would stop working, I
supplied a simple solution for it, and then you ignored that I did and
continued to claim that this case was broken for you.

Sorry if I'm calling you out for it. I don't think that necessarily
constitutes and "attack" or an "insult", it just seems to serve you to
actually attack and insult me.

And I'm tired of that.

> > 
> > Regarding security assuming a dynamically linked distro FFmpeg build:
> > 
> > Registering wouldn't help you, because any other library in the same
> > process could just register all components.  
> 
> yes but there is no such library in almost all these cases.

Sure, if you can guarantee that it's this way - otherwise it won't
work. But it's actually pretty common in multimedia settings to load
other things as plugins. Consider MediaFoundation/DirectShow (which have
FFmpeg plugins, and many other plugins), gstreamer, or VLC.

So I would say your "almost all" is not correct.

> 
> > If you relied on
> > conditional registration, your security could be randomly broken and
> > you wouldn't even notice this. Consider interesting cases like ALSA
> > loading a plugin that uses libavcodec, or so.  
> 
> Its an additional layer, not something to rely on alone

OK, so you basically admit that it doesn't work.

> 
> > 
> > Also, this didn't work. I've mentioned the fact that the functions for
> > registering individual components couldn't actually work a lot (of
> > course you always ignored it). But in this specific case it wasn't even
> > possible in theory, because the individual component's pointers are not
> > exported across DSO/DLL boundaries.  
> 
> You are mistaken and i possibly ignored that because you "exploded" when i 
> pointed out that you where a while ago.
> the whole av_ and ff_ export rules came years after the register
> APIs, so there is no way they could have been the hurdle to it
> working.

Do you deny that the way I described is the current situation and has
been for almost a decade?

> 
> > 
> > So if you're trying to suggest that the new API currently makes it
> > harder/impossible for applications to use certain mitigation in this
> > setting, don't be fooled. It wasn't possible anyway.  
> 
> A modified API would allow this mitigation, the old API recently
> didnt support it no, you are completey correct here.

OK, so you basically admit that it didn't work. Not sure why you're
trying to argue with it then.

> 
> > 
> > Third, you wrote a mechanism to exclude components at runtime yourself.
> > Is this the script for a comedy? Did you just forget about it or what?
> > The whitelist stuff lets any application set a list of components that
> > are allowed to be used in certain situations. And it works even for
> > DLL, unlike your registration nonsense.
> >   
> 
> > > IMO, an API that allows registering subsets of components would be wiser
> > > for FFmpeg.
> > > 
> > > Of course we can go with a API that doesnt allow subset registeration but
> > > then later we need to add an API to seperatly register user componentgs (plugins),
> > > so 2 APIs and our fuzzer needs to patch that API,  
> > 
> > Assuming you want plugins to be registered process-global, the new API
> > could include those (it's pretty easy to "chain" iterate them with the
> > way the new API works). I'm not sure why you pretend there's a problem,
> > considering your current arguments.
> >  
> 
> > But for the hundredth time: user components have a much much much
> > harder problem, the problem that basically all API needed to implement
> > them is private and internal. So you need to change those APIs, and in
> > a way that everyone will accept that they will be public for decades to
> > come. (These APIs are private for a reason: they can be easily fixed
> > without changing the public API, and that's also the reason why the
> > internal libavfilter API used to be public, but was made private.)
> > 
> > You haven't ever responded to this very very important point, other
> > than platitudes about how this change supposedly makes it even harder
> > (which is not true, but you don't care).  
> 
> I do actually remember replying to this, so either i didnt send that
> mail or you didnt read it.
> 
> a fully public API for implementing components would be very usefull
> but not required for plugins. 
> Without this plugins would be tied to the minor/micro version of the lib
> update for example libavcodec and none of the previously build plugins
> would be loaded anymore, they would need to be rebuild and occasionally
> adapted at the source code level.

Everyone would laugh at FFmpeg if that was supposed to be a solution
for user plugins. But for the special-case of building FFmpeg internal
components as plugins (so they could be dynamically loaded), it would
work.

But you don't need the old register API for that, and implementation
wise it's still to realize even without the linked list. It just can use
avpriv_ functions and reserve space in the component arrays. I've
participated in writing such a thing before, so I believe I know what
I'm talking about.

So this objection of yours is not an argument either.

> 
> This is of course not ideal, but it is not blocking the implementation of
> plugins. It just adds a burden on maintaining plugins that is similar to
> the burden of maintaining internal components. 

No it doesn't add any burden whatsoever.