[FFmpeg-devel] [PATCH] Handling special characters in a URL.

[Michael Niedermayer]

On Tue, Feb 26, 2013 at 08:07:46AM +0530, Senthilnathan Maadasamy wrote:
> On Sat, Feb 23, 2013 at 12:14 AM, Stefano Sabatini <stefasab at gmail.com>wrote:
> 
> > > + * @param src portion of an URL (e.g. protocol, hostname, path) to
> > > + *            percent-encoded
> >
> > percent-encode
> >
> Fixed.
> 
> >
> > > +static size_t ff_url_percent_encode(char *dst, const char *src,
> > > +                                    const char *allowed, size_t
> > dst_size)
> >
> > note: since this is *static*, the ff_ is not even required, you can
> > name it "percent_encode_url()" for example
> >
> Fixed.
> 
> >
> > > +
> >
> > > +    if (enc_len < dst_size) dst[enc_len] = '\0';
> >
> > This should always terminate the buffer, even when the buffer is not
> > long enough (but see comment below).
> >
> Fixed.
> 
> >
> > > +
> > > +    enc_size = ff_url_percent_encode(path_enc, path, "/?", path_size);
> > > +    if (enc_size <= path_size) {
> > > +        av_strlcpy(path, path_enc, path_size);
> > > +    } else {
> > > +        av_log(NULL, AV_LOG_ERROR,
> > > +            "Skipping path percent-encoding since buffer is too
> > small\n");
> > > +    }
> >
> > I'm sorry to swing back and forth, since this is basically a custom
> > function (no use outside this file), it might be better to implement
> > your first approach, in order to avoid duplication and keep it simple
> > (that is you do in-place encoding).
> >
> > Also the warning may be a bit more descriptive, for example:
> >
> >        av_log(NULL, AV_LOG_ERROR,
> >               "Skipping percent-encoding for string '%s' since buffer is
> > too small\n", );
> >
> >
> I agree that this will make the code compact.  I have made the suggested
> changes.  Please let me know if I have missed something.
> 
> 
> > Then as I wrote av_url_split() suffers major problems (no way to
> > signal errors, e.g. in case of buffer truncation), but fixing it is
> > beyond the scope of this patch I guess.
> >
> > BTW please send a git-format patch, so I don't have to fight with
> > patching tools for applying the patch.
> >
> I am attaching the patch created with git-format patch.  Thanks for your
> time and valuable suggestions.
> 
> Thanks,
> Senthil

>  utils.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 1ab0efa12eeec3989536f34bb660d6ab845863a8  special-characters-in-URL.patch
> From 4cce32a39623f920b68ea071710a599d9e92324f Mon Sep 17 00:00:00 2001
> From: Senthilnathan M <senthilnathan.maadasamy at gmail.com>
> Date: Sun, 10 Feb 2013 23:08:52 +0530
> Subject: [PATCH] Support for special characters in URL
> 
> Signed-off-by: Senthilnathan M <senthilnathan.maadasamy at gmail.com>
> ---
>  libavformat/utils.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 9bd2d0c..a3f69a3 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -3767,6 +3767,49 @@ void av_pkt_dump_log2(void *avcl, int level, AVPacket *pkt, int dump_payload,
>      pkt_dump_internal(avcl, NULL, level, pkt, dump_payload, st->time_base);
>  }
>  
> +/**
> + * Percent encode part of an URL string according to RFC 3986.
> + *
> + * @param component portion of an URL (e.g. protocol, hostname, path) to
> + *            percent-encode.  This will be percent-encoded in place.
> + * @param allowed string containing the allowed characters which must not be
> + *                encoded. It may be NULL if there are no such characters.
> + * @param component_size size in bytes of the component buffer
> + * @return 0 on success, -1 on error
> + */
> +static int percent_encode_url(char *component,
> +                                    const char *allowed, size_t component_size)
> +{
> +    char enc[MAX_URL_SIZE], c;
> +    int enc_len = 0;
> +    char *src = component;
> +

> +    while (c = *src) {

null pointer dereference


> +        if (isalnum(c) || strchr("-._~%", c)
> +            || (allowed && strchr(allowed, c))) {
> +            if (enc_len+1 < MAX_URL_SIZE) enc[enc_len] = c;
> +            else break;
> +            enc_len++;
> +        } else {
> +            if (enc_len+3 < MAX_URL_SIZE) snprintf(&enc[enc_len], 4, "%%%02x", c);
> +            else break;
> +            enc_len += 3;
> +        }
> +        src++;
> +    }
> +

> +    enc[enc_len++] = '\0';

i suspect this can write outside of the array


> +    if (enc_len <= component_size) {

> +        av_strlcpy(component, enc, component_size);

this encodes things like
A.B.C.D?ttl=123
which thebn fail apparently

i didnt immedeatly see how to fix this thus i reverted the commit so
that this can be worked on without hurry and without all kinds of
things being broken in the meantime

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

What does censorship reveal? It reveals fear. -- Julian Assange
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130228/913af051/attachment.asc>