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

[wm4]

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
    => 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?)

Well, which is it? But you could also just explicitly say what you have
in mind.

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

Anyway, existing plugin APIs seem to export some sort of generic
registration entrypoint, rather than exporting a specific symbol, so
that would be different too.

> 
> >   
> > >   
> > > >     
> > > > >     
> > > > > > 
> > > > > > 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.  
> 
> This is quite off topic but what you say doesnt fit together
> 
> * you said 
>   "Most of the serious projects using FFmpeg apply custom patches anyway."
>   and then various justifications ...
> but distributions ship a single shared set a FFmpeg libraries in general
> they dont allow or want many copies of patched libs.
> 
> So that would mean major distributions do not ship what you called 
> "serious projects using FFmpeg", i think that would be absurd and cannot be
> true. 

There's more in this world than Linux distros. Even projects which run
on Linux and have distro packages often enough provide their own FFmpeg
patches, which they apply to builds for other OSes, or their own Linux
packages, which embed their own full FFmpeg libs. They do the latter
because distros often mess up their packages, or are too old, or
cripple them in some way.

Linux distro centric thinking is of little use and very narrow-minded.

> And more so, there are very good reasons why distributions do not want
> multiple patched variants of libs, its a nightmare to track this and do
> security updates. Ever saw debian update 50 packages when a security update
> for libavcodec was released ? NO because they dont really allow this

I've never argued for distros to packages multiple copies of FFmpeg
libs in this thread or any connected discussion. Not sure why you think
I do.

The whole thing about applying patches was for special use cases, like
building a single static libavcodec lib and linking it to multiple
different executables with codecs selectively disabled (like oss-fuzz
apparently requires). It's completely reasonable to apply patches in
these cases to get some very special build behavior.

> And this is also why i said 
> "And this should maybe give us a hint that we have a problem that we should solve."
> That is if there are applications that patch our libs, they will loose these
> patches in distributions like debian or they will loose security support from
> distributions. This is not good and it is something we should solve if there are such
> cases.

I really have no idea why you're going on about this completely
unrelated issue in these threads. This is such a waste of time.

> 
> 
> [...]
> 
> > > 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?  
> 
> Iam interrested in factual discussion. But as we can see here it does not
> work that well. And so i avoid a bit to reply to you.

That is disrespectful.

> This doesnt mean i
> dont read your suggestions or ignore them just that i dont want to get
> pulled into these kind of threads.

What a coincidence, it's the same for me.

I have no idea why you're always starting with the same discussion,
even though there's no issue and nobody actually disagrees with the
core of what you're saying. The how many-est discussion about plugins
is this now? I don't know what to make out of it, but I think it's some
sort of offensive turbo-bikeshedding. But mostly it's tiring.

> 
> > 
> > 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.  
> 
> And there are so many inaccurate things in here too.
> I claimed that the commited patchset significantly increase the size of
> the "fuzzer tools" and that the diskspace there is limited.

Sure, I agree. But I'm also saying that this can be fixed.

> 1. You turn this into a "you claimed something would stop working", i
>   did not say that though it very possibly could stop working or it
>   could restrict future extensions like testing encoders or parsers.

FUD

> 2. "I supplied a simple solution for it, and then you ignored that"
>     IMO, a solution is a change to the code, you did not supply that.
>     you supplied some idea(s), there already are multiple ideas on
>     how to workarouind or solve it.

So supplying a most likely working idea to solve a problem is not a
solution? That sounds weird to me.

> 3. "and continued to claim that this case was broken for you."
>   Again i said the files are many times bigger and they where at the
>   time i said that. Thats solved when its solved not when someone
>   publishes an idea on how it could be solved.

I don't maintain that oss-fuzz thing though. Of course it happens that
some change sometimes inconveniences some users (while it helps
others). We have that the entire time. It includes changes made by you
as well. What is your point?

> Now how is this sort of arguing relevant to a "factual discussion"
> most of your example here is not factual

So it's not factual because even though I provided a possible solution
or idea of a solution, but it was not a concrete patch that could be
applied (i.e. not a fact), or how should I understand this?

> More so the factual discussion in this thread should be about designing
> a better API. Large parts of this are not contributing to that

Well, you haven't even given any ideas how such an API should be
designed. Yet you're complaining about it.

>     
> > 
> > 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.  
> 
> security concious applications would likely run FFmpeg and its libs in
> a seperate process. So there is not going to be other things in it.

Probably, but then again you could just use those codec whitelists you
added. And they work in all cases, not just when you can somehow be
sure to control the entire process.

> And security concious applications which do not will very likely be
> very restrictiv on what they link to. Its unlikely they will
> pull in huge unrelated plugin systems as that runs against the idea of
> security.

Then why are you arguing that we should have a plugin system?

> But as you say "Sure, if you can guarantee that it's this way"
> In fact its easy to gurantee this so as to eliminate all of these
> corner cases.

The conclusion of that would be "if you want security, run FFmpeg in a
separate process" - preferably sandboxed?

> you only need a register_lock() which disables registering any further
> components.
> Then the application just registers what it needs and calls
> if (register_lock() != "number of codecs the app registered")
>     fatal();

Doesn't work if that other component registers things before this is
called. With library ctors being popular (and FFmpeg being a library
which needs or needed global init making t his attractive) this is a
possibility that has to be considered.

> Now if a lib attempts to register a component after this it would
> only succeed if that component was already registered and fail otherwise.
> So no lib can inject more components than what the application authorized
> 
> 
> >   
> > >   
> > > > 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.  
> 
> no, thats your interpretation. Maybe what you want to hear, i dont know but
> its not what i said.

So what did you want to say. Just saying "don't rely on that alone" is
at most a weak argument away from admitting that I'm right.

> and this kind of reinterpreting things i say, is not a basis for discussion.
> 
> Thanks
> 
> [...]