[FFmpeg-devel] [PATCH v2] libavformat/http: Refactor and fix additional leaks in get_cookies.

Richard Shaffer rshaffer at tunein.com
Thu Apr 19 23:07:09 EEST 2018


On Thu, Apr 19, 2018 at 1:04 PM, wm4 <nfxjfg at googlemail.com> wrote:

> On Thu, 19 Apr 2018 12:55:00 -0700
> rshaffer at tunein.com wrote:
>
> > From: Richard Shaffer <rshaffer at tunein.com>
> >
> > This refactors get_cookies to simplify some code paths, specifically for
> > skipping logic in the while loop or exiting it. It also simplifies the
> logic
> > for appending additional values to *cookies by replacing
> strlen/malloc/snprintf
> > with one call av_asnprintf.
> >
> > This refactor fixes a bug where the cookie_params AVDictionary would get
> leaked
> > if we failed to allocate a new buffer for writing to *cookies.
> > ---
> > Updated so that next = set_cookies = av_strdup(s->cookies) assignment is
> done
> > on a separate line instead of inside the if conditional.
> >
> >  libavformat/http.c | 65 +++++++++++++++++++++++-------
> ------------------------
> >  1 file changed, 28 insertions(+), 37 deletions(-)
> >
> > diff --git a/libavformat/http.c b/libavformat/http.c
> > index b4a1919f24..d59ffbbbe8 100644
> > --- a/libavformat/http.c
> > +++ b/libavformat/http.c
> > @@ -1015,7 +1015,8 @@ static int process_line(URLContext *h, char *line,
> int line_count,
> >  /**
> >   * Create a string containing cookie values for use as a HTTP cookie
> header
> >   * field value for a particular path and domain from the cookie values
> stored in
> > - * the HTTP protocol context. The cookie string is stored in *cookies.
> > + * the HTTP protocol context. The cookie string is stored in *cookies,
> and may
> > + * be NULL if there are no valid cookies.
> >   *
> >   * @return a negative value if an error condition occurred, 0 otherwise
> >   */
> > @@ -1025,15 +1026,20 @@ static int get_cookies(HTTPContext *s, char
> **cookies, const char *path,
> >      // cookie strings will look like Set-Cookie header field values.
> Multiple
> >      // Set-Cookie fields will result in multiple values delimited by a
> newline
> >      int ret = 0;
> > -    char *cookie, *set_cookies = av_strdup(s->cookies), *next =
> set_cookies;
> > -
> > -    if (!set_cookies) return AVERROR(EINVAL);
> > +    char *cookie, *set_cookies, *next;
> >
> >      // destroy any cookies in the dictionary.
> >      av_dict_free(&s->cookie_dict);
> >
> > +    if (!s->cookies)
> > +        return 0;
> > +
> > +    next = set_cookies = av_strdup(s->cookies);
> > +    if (!next)
> > +        return AVERROR(ENOMEM);
> > +
> >      *cookies = NULL;
> > -    while ((cookie = av_strtok(next, "\n", &next))) {
> > +    while ((cookie = av_strtok(next, "\n", &next)) && !ret) {
> >          AVDictionary *cookie_params = NULL;
> >          AVDictionaryEntry *cookie_entry, *e;
> >
> > @@ -1043,23 +1049,19 @@ static int get_cookies(HTTPContext *s, char
> **cookies, const char *path,
> >
> >          // continue on to the next cookie if this one cannot be parsed
> >          if (parse_set_cookie(cookie, &cookie_params))
> > -            continue;
> > +            goto skip_cookie;
> >
> >          // if the cookie has no value, skip it
> >          cookie_entry = av_dict_get(cookie_params, "", NULL,
> AV_DICT_IGNORE_SUFFIX);
> > -        if (!cookie_entry || !cookie_entry->value) {
> > -            av_dict_free(&cookie_params);
> > -            continue;
> > -        }
> > +        if (!cookie_entry || !cookie_entry->value)
> > +            goto skip_cookie;
> >
> >          // if the cookie has expired, don't add it
> >          if ((e = av_dict_get(cookie_params, "expires", NULL, 0)) &&
> e->value) {
> >              struct tm tm_buf = {0};
> >              if (!parse_set_cookie_expiry_time(e->value, &tm_buf)) {
> > -                if (av_timegm(&tm_buf) < av_gettime() / 1000000) {
> > -                    av_dict_free(&cookie_params);
> > -                    continue;
> > -                }
> > +                if (av_timegm(&tm_buf) < av_gettime() / 1000000)
> > +                    goto skip_cookie;
> >              }
> >          }
> >
> > @@ -1067,42 +1069,31 @@ static int get_cookies(HTTPContext *s, char
> **cookies, const char *path,
> >          if ((e = av_dict_get(cookie_params, "domain", NULL, 0)) &&
> e->value) {
> >              // find the offset comparison is on the min domain (b.com,
> not a.b.com)
> >              int domain_offset = strlen(domain) - strlen(e->value);
> > -            if (domain_offset < 0) {
> > -                av_dict_free(&cookie_params);
> > -                continue;
> > -            }
> > +            if (domain_offset < 0)
> > +                goto skip_cookie;
> >
> >              // match the cookie domain
> > -            if (av_strcasecmp(&domain[domain_offset], e->value)) {
> > -                av_dict_free(&cookie_params);
> > -                continue;
> > -            }
> > +            if (av_strcasecmp(&domain[domain_offset], e->value))
> > +                goto skip_cookie;
> >          }
> >
> >          // ensure this cookie matches the path
> >          e = av_dict_get(cookie_params, "path", NULL, 0);
> > -        if (!e || av_strncasecmp(path, e->value, strlen(e->value))) {
> > -            av_dict_free(&cookie_params);
> > -            continue;
> > -        }
> > +        if (!e || av_strncasecmp(path, e->value, strlen(e->value)))
> > +            goto skip_cookie;
> >
> >          // cookie parameters match, so copy the value
> >          if (!*cookies) {
> > -            if (!(*cookies = av_asprintf("%s=%s", cookie_entry->key,
> cookie_entry->value))) {
> > -                ret = AVERROR(ENOMEM);
> > -                break;
> > -            }
> > +            *cookies = av_asprintf("%s=%s", cookie_entry->key,
> cookie_entry->value);
> >          } else {
> >              char *tmp = *cookies;
> > -            size_t str_size = strlen(cookie_entry->key) +
> strlen(cookie_entry->value) + strlen(*cookies) + 4;
> > -            if (!(*cookies = av_malloc(str_size))) {
> > -                ret = AVERROR(ENOMEM);
> > -                av_free(tmp);
> > -                break;
> > -            }
> > -            snprintf(*cookies, str_size, "%s; %s=%s", tmp,
> cookie_entry->key, cookie_entry->value);
> > +            *cookies = av_asprintf("%s; %s=%s", tmp, cookie_entry->key,
> cookie_entry->value);
> >              av_free(tmp);
> >          }
> > +        if (!*cookies)
> > +            ret = AVERROR(ENOMEM);
> > +
> > +    skip_cookie:
> >          av_dict_free(&cookie_params);
> >      }
> >
>
> Pushed. (My mail client accidentally replied to the patch author only,
> so he'll see this a second time.)
>

Thanks.

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>


More information about the ffmpeg-devel mailing list