[FFmpeg-devel] [PATCH] Replace old cookies with new cookies of the same name
Nicolas George
george at nsup.org
Sun Aug 17 13:29:15 CEST 2014
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?
> 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,
--
Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140817/0f302e06/attachment.asc>
More information about the ffmpeg-devel
mailing list