On Sun, 17 Aug 2014 13:29:15 +0200
Nicolas George <geo...@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 <micahgali...@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,
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
