[FFmpeg-devel] [PATCH 1/2] avcodec: add avcodec_register_one() to register one codec/parser/bsf/hwaccel by its char* name

wm4 nfxjfg at googlemail.com
Tue Sep 30 18:23:38 CEST 2014


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

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

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?

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.

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



More information about the ffmpeg-devel mailing list