[FFmpeg-devel] [PATCH 2/2] avformat: add protocol_whitelist

[Andreas Cadhalpun]

On 24.01.2016 03:42, Michael Niedermayer wrote:
> From: Michael Niedermayer <michael at niedermayer.cc>
> 
> TODO: Docs
> TODO: version bump
> 
> Note to maintainers: update tools
> 
> Note, testing and checking for missing changes is needed
> 
> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> ---
>  ffmpeg_opt.c                      |    4 ++--
>  libavdevice/lavfi.c               |    2 +-
>  libavformat/async.c               |    2 +-
>  libavformat/avformat.h            |    7 ++++++
>  libavformat/avio.c                |   44 ++++++++++++++++++++++++++++++++++---
>  libavformat/avio.h                |    4 ++++
>  libavformat/aviobuf.c             |   16 ++++++++++----
>  libavformat/cache.c               |    3 ++-
>  libavformat/concat.c              |    5 +++--
>  libavformat/crypto.c              |    5 +++--
>  libavformat/dashenc.c             |    9 ++++----
>  libavformat/ftp.c                 |   10 +++++----
>  libavformat/gopher.c              |    4 ++--
>  libavformat/hdsenc.c              |   12 +++++-----
>  libavformat/hls.c                 |    8 ++++---
>  libavformat/hlsenc.c              |   28 +++++++++++------------
>  libavformat/hlsproto.c            |   10 +++++----
>  libavformat/http.c                |   16 +++++++++-----
>  libavformat/icecast.c             |    3 ++-
>  libavformat/md5proto.c            |    5 +++--
>  libavformat/mmst.c                |    5 +++--
>  libavformat/movenc.c              |    2 +-
>  libavformat/options_table.h       |    1 +
>  libavformat/rtmpcrypt.c           |    5 +++--
>  libavformat/rtmpproto.c           |   10 +++++----
>  libavformat/rtpproto.c            |   10 ++++++---
>  libavformat/rtsp.c                |   20 ++++++++---------
>  libavformat/rtspdec.c             |   10 +++++----
>  libavformat/sapdec.c              |    5 +++--
>  libavformat/sapenc.c              |    9 +++++---
>  libavformat/segment.c             |   16 +++++++-------
>  libavformat/smoothstreamingenc.c  |   18 ++++++++-------
>  libavformat/srtpproto.c           |    3 ++-
>  libavformat/subfile.c             |    3 ++-
>  libavformat/tee.c                 |    4 +++-
>  libavformat/tls.c                 |    5 +++--
>  libavformat/tls_securetransport.c |    5 +++--
>  libavformat/url.h                 |    5 +++++
>  libavformat/utils.c               |   13 +++++++----
>  libavformat/webm_chunk.c          |    7 +++---
>  40 files changed, 230 insertions(+), 123 deletions(-)
> 

To make reviewing easier it would be nice to split this into a patch
that adds the protocol_whitelist mechanism and a subsequent one,
which forwards the protocol_whitelist where necessary.

> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index 4964263..2fb9130 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -1815,6 +1815,13 @@ typedef struct AVFormatContext {
>       * Demuxing: Set by user.
>       */
>      int (*open_cb)(struct AVFormatContext *s, AVIOContext **p, const char *url, int flags, const AVIOInterruptCB *int_cb, AVDictionary **options);
> +
> +    /**
> +     * ',' separated list of allowed protocols.
> +     * - encoding: unused
> +     * - decoding: set by user through AVOptions (NO direct access)
> +     */
> +    char *protocol_whitelist;
>  } AVFormatContext;

I'm not sure if adding protocol_whitelist to the AVFormatContext is good.
Conceptually it would fit better in the AVIOContext. I think that would also
make it easier to set a default for it.

> diff --git a/libavformat/avio.c b/libavformat/avio.c
> index 05d0557..ad9712d 100644
> --- a/libavformat/avio.c
> +++ b/libavformat/avio.c
[...]
> @@ -296,18 +302,43 @@ int ffurl_alloc(URLContext **puc, const char *filename, int flags,
>      return AVERROR_PROTOCOL_NOT_FOUND;
>  }
>  
> -int ffurl_open(URLContext **puc, const char *filename, int flags,
> -               const AVIOInterruptCB *int_cb, AVDictionary **options)
> +int ffurl_open_whitelist(URLContext **puc, const char *filename, int flags,
> +                         const AVIOInterruptCB *int_cb, AVDictionary **options, const char *whitelist)
>  {
> +    AVDictionary *tmp_opts = NULL;
> +    AVDictionaryEntry *e;
>      int ret = ffurl_alloc(puc, filename, flags, int_cb);
>      if (ret < 0)
>          return ret;
>      if (options && (*puc)->prot->priv_data_class &&
>          (ret = av_opt_set_dict((*puc)->priv_data, options)) < 0)
>          goto fail;
> +
> +    if (!options)
> +        options = &tmp_opts;
> +
> +    av_assert0(!whitelist ||
> +               !(e=av_dict_get(*options, "protocol_whitelist", NULL, 0)) ||
> +               !strcmp(whitelist, e->value));
> +
> +    if ((ret = av_dict_set(options, "protocol_whitelist", whitelist, 0)) < 0)
> +        goto fail;
> +

Wouldn't it be much simpler to pass the protocol_whitelist directly in options
to ffurl_open, instead of adding a new function with a new parameter, just
to set the option here.
That way one could avoid the need for a new public avio_open_whitelist function.
Granted, it would be less explicit then, which might cause people to forget
to set it in the future.
I'm not sure which way would be better overall.

>      if ((ret = av_opt_set_dict(*puc, options)) < 0)
>          goto fail;
> +
> +    whitelist = (*puc)->protocol_whitelist;
> +    if (whitelist && av_match_list((*puc)->prot->name, whitelist, ',') <= 0) {
> +        av_log(*puc, AV_LOG_ERROR, "Protocol not on whitelist!\n");

It would be nice to mention the protocol and the whitelist in the error message.

> +        ret = AVERROR(EINVAL);
> +        goto fail;
> +    }

I think it would be better to check the whitelist in ffurl_connect, as that's
where it actually opens the new protocol and it is not only called by
ffurl_open.

Also, this patch doesn't include a mechanism setting a good default for the
protocol_whitelist, which would be needed to fix the problem of unintentionally
mixing local and remote data.
If the protocol_whitelist would be saved in the AVIOContext, such a mechanism
could be implemented in ffurl_open/ffurl_connect.

Best regards,
Andreas