[FFmpeg-devel] [PATCH] Replace old cookies with new cookies of the same name

[wm4]

On Sun, 17 Aug 2014 13:29:15 +0200
Nicolas George <george at nsup.org> wrote:

> Le decadi 30 thermidor, an CCXXII, Micah Galizia a écrit :
> > When new cookie values (with the same name as an existing cookie) are
> > returned in an HLS stream, the current implementation will append the
> > new cookie to the list of cookies. This causes FFMPEG to send multiple
> > cookie values for the same cookie and in some cases exceed the http
> > header size in some cases.
> > 
> > The patch attached resolves the issue.
> 
> Thanks for the patch. Below are a few technical remarks. But before that, a
> general remark:
> 
> char *cookies;  ///< holds newline (\n) delimited Set-Cookie header field
>                      values (without the "Set-Cookie: " field name)
> 
> Well, this is just awful. This is obviously not your fault, this is existing
> code, but it is no way of building anything: it can do for a quick-and-dirty
> first implementation to get things working, but it makes a very bad ground
> to build upon.
> 
> It seems to me that most of your patch is there to deal with that awful data
> structure. A large part of the existing code too, for that matter.
> 
> IMHO, it would be much better to rework the current code to use a proper
> data structure. A dynarray of structures with name, value, path and domain
> already split seems like the best option.
> 
> I suspect it would even be simpler to do that than making sure your patch is
> correct in its current form.
> 
> Are you interested in that?

AFAIK this cookie string is exposed by AVOption API, and I use it for
setting cookies. And I don't feel like rewriting this code at all.

> 
> > From ad65b070a7b49698e623f08365ec7e751d0bae08 Mon Sep 17 00:00:00 2001
> > From: Micah Galizia <micahgalizia at gmail.com>
> > Date: Sun, 17 Aug 2014 20:50:02 +1000
> > Subject: [PATCH] Replace old cookies with new cookies of the same name
> > 
> > ---
> >  libavformat/http.c | 86 +++++++++++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 73 insertions(+), 13 deletions(-)
> > 
> > diff --git a/libavformat/http.c b/libavformat/http.c
> > index 7480834..f1b9c34 100644
> > --- a/libavformat/http.c
> > +++ b/libavformat/http.c
> > @@ -444,6 +444,77 @@ static int parse_icy(HTTPContext *s, const char *tag, const char *p)
> >      return 0;
> >  }
> >  
> > +static int update_cookies(URLContext *h, char **cookies, char *new_cookie) {
> > +
> > +    int prefix, offset, suffix, new_cookie_len = strlen(new_cookie);
> > +    char *name, *cookie, *next;
> > +
> > +    // if the existing cookies are empty then just set it and forget it
> > +    if (!*cookies) {
> > +        if (!(*cookies = av_strdup(new_cookie)))
> > +            return AVERROR(ENOMEM);
> > +        return 0;
> > +    }
> > +
> > +    // get the new cookies name
> > +    if (!(cookie = av_strdup(new_cookie)))
> > +        return AVERROR(ENOMEM);
> > +
> 
> > +    // if the new cookie format isnt valid just return (without error)
> > +    if (!(name = av_strtok(cookie, "=", &next))) {
> > +        av_log(h, AV_LOG_INFO, "Invalid new cookie format.");
> > +        return 0;
> > +    }
> 
> This makes the av_strdup()ed cookie leak.
> 
> > +
> > +    // copy the name and then free the copy of the cookies
> > +    name = av_asprintf("%s=", name);
> > +    av_free(cookie);
> > +
> > +    // find the offset and free the name (we don't need it anymore)
> > +    next = av_stristr(*cookies, name);
> > +    av_free(name);
> 
> I believe this is wrong: av_stristr() will find "n=" in "session=", so
> "Set-Cookie: n=42" will overwrite "session=1729".
> 
> > +
> > +    // if no substring is found, just append
> > +    if (!next) {
> > +        cookie = av_asprintf("%s\n%s", *cookies, new_cookie);
> > +        av_free(*cookies);
> > +        *cookies = cookie;
> > +        return 0;
> > +    }
> > +
> > +    prefix = next - *cookies;
> > +
> > +    // no subsequent newline means this is the last cookie
> 
> > +    if (!(next = av_stristr(next, "\n"))) {
> 
> av_stristr() for searching a single non-alphabetic character seems overkill.
> 
> > +
> > +        // old and new cookie plus null terminate
> 
> > +        if (!(cookie = av_malloc(prefix + new_cookie_len + 1)))
> 
> It needs a check for integer overflow.
> 
> > +            return AVERROR(ENOMEM);
> > +
> > +        strncpy(cookie, *cookies, prefix);
> 
> > +        sprintf(&cookie[prefix], "%s", new_cookie);
> 
> Unbounded string copy. I know the buffer has been allocated large enough,
> but this makes for fragile code.
> 
> > +        av_free(*cookies);
> > +        *cookies = cookie;
> > +        return 0;
> > +    }
> > +
> > +    offset = next - *cookies;
> > +    suffix = strlen(*cookies) - offset;
> > +
> 
> > +    if (!(cookie = av_malloc(prefix + suffix + new_cookie_len + 2)))
> 
> Check for integer overflow, same as above.
> 
> > +        return AVERROR(ENOMEM);
> > +
> > +    // copy in the prefix, new cookie, and suffix if they exist
> > +    if (prefix) strncpy(cookie, *cookies, prefix);
> 
> > +    sprintf(&cookie[prefix], "%s", new_cookie);
> 
> Unbounded string copy, same as above.
> 
> > +    if (suffix) strncpy(&cookie[prefix + new_cookie_len], &((*cookies)[offset]), suffix);
> > +
> > +    av_free(*cookies);
> > +    *cookies = cookie;
> > +
> > +    return 0;
> > +}
> > +
> >  static int process_line(URLContext *h, char *line, int line_count,
> >                          int *new_location)
> >  {
> > @@ -515,19 +586,8 @@ static int process_line(URLContext *h, char *line, int line_count,
> >              av_free(s->mime_type);
> >              s->mime_type = av_strdup(p);
> >          } else if (!av_strcasecmp(tag, "Set-Cookie")) {
> > -            if (!s->cookies) {
> > -                if (!(s->cookies = av_strdup(p)))
> > -                    return AVERROR(ENOMEM);
> > -            } else {
> > -                char *tmp = s->cookies;
> > -                size_t str_size = strlen(tmp) + strlen(p) + 2;
> > -                if (!(s->cookies = av_malloc(str_size))) {
> > -                    s->cookies = tmp;
> > -                    return AVERROR(ENOMEM);
> > -                }
> > -                snprintf(s->cookies, str_size, "%s\n%s", tmp, p);
> > -                av_free(tmp);
> > -            }
> > +            update_cookies(h, &s->cookies, p);
> > +            av_log(h, AV_LOG_VERBOSE, "Cookies set to '%s'\n", s->cookies);
> >          } else if (!av_strcasecmp(tag, "Icy-MetaInt")) {
> >              s->icy_metaint = strtoll(p, NULL, 10);
> >          } else if (!av_strncasecmp(tag, "Icy-", 4)) {
> 
> Regards,
>