[FFmpeg-devel] [PATCH 2/2] avformat: add protocol_whitelist
Michael Niedermayer
michael at niedermayer.cc
Sun Jan 24 16:03:40 CET 2016
On Sun, Jan 24, 2016 at 03:37:24PM +0100, Andreas Cadhalpun wrote:
> 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.
the AVFormatContexts AVIOContext can be NULL
>
> > 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.
will change that
>
> 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.
can you elaborate
i dont see how that would work
also some users use their private IO, i dont think theres a foolproof
way to identify if its remote or local
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160124/f35917a5/attachment.sig>
More information about the ffmpeg-devel
mailing list