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

Tue Feb 20 22:45:12 EET 2018

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.

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

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

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

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

Some distros do not allow multiple custom builds of libs to be packaged for
installation at the same time.
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.

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.

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

> 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

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

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

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

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. 

Thanks

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No great genius has ever existed without some touch of madness. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180220/890153f5/attachment.sig>