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

[wm4]

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.

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?

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

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

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

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.

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.

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

Otherwise you're welcome to try, but of course that shouldn't block
current (and almost finished) attempts to solve other problems.

> and browsers which may only 
> want to register a subset of codecs & formats are screwed.

That's pretty amusing. Which browsers are dependent on a system FFmpeg
shared library and register only individual codecs?
 
> Seriously, why should we do it this way ?

None of your arguments actually work, so that's why we did it this way.

What you suggest is actually quite hazardous. The list of components
would still be global, but freely editable by any API user in the
process. That's going to lead to chaos. That's why we should absolutely
not do it your way.

> An API that allows registering subsets is not very complex

Oh yeah? Then how is that going to look, with our policy of not
exporting data symbols as public symbols. Are you going to generate and
export a few hundreds of new API functions (one for each codec)?

Of course a runtime mechanism (codec whitelists) already exists and
would not require further changes. Why do you think is that not
sufficient?