[FFmpeg-devel] [PATCH 1/2] avcodec: add avcodec_register_one() to register one codec/parser/bsf/hwaccel by its char* name
Michael Niedermayer
michaelni at gmx.at
Tue Sep 30 19:57:08 CEST 2014
On Tue, Sep 30, 2014 at 06:23:38PM +0200, wm4 wrote:
> On Tue, 30 Sep 2014 17:21:20 +0200
> Michael Niedermayer <michaelni at gmx.at> wrote:
>
> > Hi
> >
> > On Tue, Sep 30, 2014 at 03:51:13PM +0200, wm4 wrote:
> > > On Tue, 30 Sep 2014 02:02:49 +0200
> > > Michael Niedermayer <michaelni at gmx.at> wrote:
> > >
> > > > Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> > > > ---
> > > > libavcodec/allcodecs.c | 51 +++++++++++++++++++++++++++++++++---------------
> > > > libavcodec/avcodec.h | 5 +++++
> > > > 2 files changed, 40 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> > > > index 7650543..019d5ea 100644
> > > > --- a/libavcodec/allcodecs.c
> > > > +++ b/libavcodec/allcodecs.c
> > > > @@ -29,49 +29,55 @@
> > > > #include "version.h"
> > > >
> > > > #define REGISTER_HWACCEL(X, x) \
> > > > - { \
> > > > + if (!name || !strcmp(name, AV_STRINGIFY(x##_hwaccel))) { \
> > > > extern AVHWAccel ff_##x##_hwaccel; \
> > > > - if (CONFIG_##X##_HWACCEL) \
> > > > + if (CONFIG_##X##_HWACCEL) { \
> > > > av_register_hwaccel(&ff_##x##_hwaccel); \
> > > > + found++; \
> > > > + } \
> > > > }
> > > >
> > > > #define REGISTER_ENCODER(X, x) \
> > > > - { \
> > > > + if (!name || !strcmp(name, AV_STRINGIFY(x##_encoder))) { \
> > > > extern AVCodec ff_##x##_encoder; \
> > > > - if (CONFIG_##X##_ENCODER) \
> > > > + if (CONFIG_##X##_ENCODER) { \
> > > > avcodec_register(&ff_##x##_encoder); \
> > > > + found++; \
> > > > + } \
> > > > }
> > > >
> > > > #define REGISTER_DECODER(X, x) \
> > > > - { \
> > > > + if (!name || !strcmp(name, AV_STRINGIFY(x##_decoder))) { \
> > > > extern AVCodec ff_##x##_decoder; \
> > > > - if (CONFIG_##X##_DECODER) \
> > > > + if (CONFIG_##X##_DECODER) { \
> > > > avcodec_register(&ff_##x##_decoder); \
> > > > + found++; \
> > > > + } \
> > > > }
> > > >
> > > > #define REGISTER_ENCDEC(X, x) REGISTER_ENCODER(X, x); REGISTER_DECODER(X, x)
> > > >
> > > > #define REGISTER_PARSER(X, x) \
> > > > - { \
> > > > + if (!name || !strcmp(name, AV_STRINGIFY(x##_parser))) { \
> > > > extern AVCodecParser ff_##x##_parser; \
> > > > - if (CONFIG_##X##_PARSER) \
> > > > + if (CONFIG_##X##_PARSER) { \
> > > > av_register_codec_parser(&ff_##x##_parser); \
> > > > + found++; \
> > > > + } \
> > > > }
> > > >
> > > > #define REGISTER_BSF(X, x) \
> > > > - { \
> > > > + if (!name || !strcmp(name, AV_STRINGIFY(x##_bsf))) { \
> > > > extern AVBitStreamFilter ff_##x##_bsf; \
> > > > - if (CONFIG_##X##_BSF) \
> > > > + if (CONFIG_##X##_BSF) { \
> > > > av_register_bitstream_filter(&ff_##x##_bsf); \
> > > > + found++; \
> > > > + } \
> > > > }
> > > >
> > > > -void avcodec_register_all(void)
> > > > +int avcodec_register_one(const char *name)
> > > > {
> > > > - static int initialized;
> > > > -
> > > > - if (initialized)
> > > > - return;
> > > > - initialized = 1;
> > > > + int found = 0;
> > > >
> > > > /* hardware accelerators */
> > > > REGISTER_HWACCEL(H263_VAAPI, h263_vaapi);
> > > > @@ -588,4 +594,17 @@ void avcodec_register_all(void)
> > > > REGISTER_BSF(NOISE, noise);
> > > > REGISTER_BSF(REMOVE_EXTRADATA, remove_extradata);
> > > > REGISTER_BSF(TEXT2MOVSUB, text2movsub);
> > > > +
> > > > + return found;
> > > > }
> > > > +
> > > > +void avcodec_register_all(void)
> > > > +{
> > > > + static int initialized;
> > > > +
> > > > + if (initialized)
> > > > + return;
> > > > + initialized = 1;
> > > > +
> > > > + avcodec_register_one(NULL);
> > > > +}
> > > > \ No newline at end of file
> > > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > > > index 94e82f7..de936a5 100644
> > > > --- a/libavcodec/avcodec.h
> > > > +++ b/libavcodec/avcodec.h
> > > > @@ -3472,6 +3472,11 @@ void avcodec_register(AVCodec *codec);
> > > > void avcodec_register_all(void);
> > > >
> > > > /**
> > > > + * @returns the number of newly registered entities
> > > > + */
> > > > +int avcodec_register_one(const char *name);
> > > > +
> > > > +/**
> > > > * Allocate an AVCodecContext and set its fields to default values. The
> > > > * resulting struct should be freed with avcodec_free_context().
> > > > *
> > >
> > > What's the purpose? It requires global mutable state, and won't work is
> > > more than one library in a process is using ffmpeg. So this would just
> > > add a fundamentally misdesigned API.
> > >
> >
> > > If the purpose is to restrict the codecs that can be used (maybe for
> > > security reasons?),
> >
> > yes, its intended for security purposes
> >
> >
> > > then it should be done in a safe manner
> >
> > yes
> >
> >
> > > with no
> > > global mutable state.
> >
> > i disagree, applications that care about security must
> > restrict used entities globally, thus by definition want to change
> > global state. maybe not the way its done in this patch yes, ill
> > think about it and submit something better but
>
> No, it doesn't work.
> Code using libavcodec can and will call
> avcodec_register_all().
yes, but avcodec_register_all() would then just not register anything
that has been restricted before.
There never was a gurantee that it would register some specific
decoder as it could have been disabled at compile time
>
> > A security critical application does not want a mysterious unknown
> > library to load and use a unrestricted libavcodec behind its back
> > that would likely bypass the whole idea of restricting the decoders
> > and demuxers
>
> The only reasonable way to do that is to make sure the whole thing is
> secure at build time. That is, strip everything unneeded or unwanted
> from ffmpeg when configuring, and making sure everything in the program
> is using this ffmpeg binary.
thats disallowed per policy in some distributions like debian AFAIK
libs must be build as shared libs and shared, no local copies allowed
if avoidable or something unless i misremember
also there are reasons for this, doig security maintaince for all
these copies would be a nightmare for a distribution
>
> You can not decide this at runtime and with an unknown set of
> libraries. Unless the library you're using is designed to enable this
> use case explicitly and won't call avcodec_register_all() on its own.
> But then you might as well create a better API, like a per
> AV(Codec/Format)Context whitelist of allowed codecs.
>
> >
> > > I can't see any other purpose; it won't prevent
> > > "uneeded" codecs from being pulled in by the linker.
> > >
> >
> > > Even worse, it would make it impossible to fix the thread-safety and
> > > library-safety issues of avcodec_register_all().
> >
> > what issues are you talking about? please elaborate
>
> What happens if two libraries call avcodec_register_all() at the same
> time in separate threads?
The individual register functions are thread safe, the
register_all isnt, which is not good i agree
>
> Oh well, you could probably add a hack to mitigate this, but the point
> is that this is unnecessary global state and could be avoided entirely.
> There's literally no reason for it to exist.
yes
anyway ive posted another patchset before reading this but i think
it should resolve some of the points raised here
>
> (A mitigation hack wouldn't fix this completely, because compiling
> without threading APIs is still supposed to result in thread-safe
> library, at least if this awful libavcodec "lock manager" thing is
> used.)
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140930/cc0d2005/attachment.asc>
More information about the ffmpeg-devel
mailing list