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

[Andreas Cadhalpun]

On 27.01.2016 14:22, Michael Niedermayer wrote:
> From: Michael Niedermayer <michael at niedermayer.cc>
> 
> TODO: Docs
> TODO: version bump
> 
> Note to maintainers: update tools
> 
> Note to maintainers: set a default whitelist for your protocol
> If that makes no sense then consider to set "none" and thus require the user to specify a white-list
> for sub-protocols to be opened
> 
> Note, testing and checking for missing changes is needed
> 
> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> ---
>  libavformat/avformat.h      |    7 +++++
>  libavformat/avio.c          |   66 ++++++++++++++++++++++++++++++++++++++++---
>  libavformat/avio_internal.h |    4 +++
>  libavformat/aviobuf.c       |   16 ++++++++---
>  libavformat/options_table.h |    1 +
>  libavformat/url.h           |    6 ++++
>  libavformat/utils.c         |   13 ++++++---
>  7 files changed, 101 insertions(+), 12 deletions(-)

I generally like the approach, but the main problem now is that setting a
default protocol_whitelist doesn't quite work as intended.
This is because it is not propagated back to the AVFormatContext
protocol_whitelist, which is used (in the following patch) to forward
the whitelist when opening other protocols.
Thus I think there also needs to be a protocol_whitelist in the AVIOContext,
which can be used to propagate the whitelist set in ffurl_connect back
to the AVFormatContext.
This forwarding could be done in ffio_open_whitelist (URLContext->AVIOContext)
and ffio_open2_wrapper (AVIOContext->AVFormatContext), and the latter could
then be used instead of ffio_open_whitelist.

> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index 285bb16..273a6ae 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -1827,6 +1827,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

Really?
The following patch also uses the whitelist in various *enc.c files.

> +     * - decoding: set by user through AVOptions (NO direct access)
> +     */
> +    char *protocol_whitelist;
>  } AVFormatContext;
>  
>  int av_format_get_probe_score(const AVFormatContext *s);
> diff --git a/libavformat/avio.c b/libavformat/avio.c
> index 05d0557..eb80fc9 100644
> --- a/libavformat/avio.c
> +++ b/libavformat/avio.c
[...]
> @@ -201,12 +207,42 @@ fail:
>  
>  int ffurl_connect(URLContext *uc, AVDictionary **options)
>  {
> -    int err =
> +    int err;
> +    AVDictionary *tmp_opts = NULL;
> +    AVDictionaryEntry *e;
> +
> +    if (!options)
> +        options = &tmp_opts;
> +
> +    // Check that URLContext was initialized correctly and lists are matching if set
> +    av_assert0(!(e=av_dict_get(*options, "protocol_whitelist", NULL, 0)) ||
> +               !strcmp(uc->protocol_whitelist, e->value));
> +
> +    if (uc->protocol_whitelist && av_match_list(uc->prot->name, uc->protocol_whitelist, ',') <= 0) {
> +        av_log(uc, AV_LOG_ERROR, "Protocol not on whitelist \'%s\'!\n", uc->protocol_whitelist);
> +        return AVERROR(EINVAL);
> +    }
> +
> +    if (!uc->protocol_whitelist && uc->prot->default_whitelist) {
> +        uc->protocol_whitelist = av_strdup(uc->prot->default_whitelist);

This leaks memory. The protocol_whitelist should be freed in ffurl_closep.
Also it would be nice to have a debug message here saying which default
protocol whitelist gets set.

> diff --git a/libavformat/url.h b/libavformat/url.h
> index 391e3bc..5c3f3fb 100644
> --- a/libavformat/url.h
> +++ b/libavformat/url.h
> @@ -47,6 +47,7 @@ typedef struct URLContext {
>      int is_connected;
>      AVIOInterruptCB interrupt_callback;
>      int64_t rw_timeout;         /**< maximum time to wait for (network) read/write operation completion, in mcs */
> +    char *protocol_whitelist;
>  } URLContext;
>  
>  typedef struct URLProtocol {
> @@ -94,6 +95,7 @@ typedef struct URLProtocol {
>      int (*url_close_dir)(URLContext *h);
>      int (*url_delete)(URLContext *h);
>      int (*url_move)(URLContext *h_src, URLContext *h_dst);
> +    char *default_whitelist;

This should be a 'const char*' to avoid compilation warnings.

Best regards,
Andreas