[FFmpeg-devel] [PATCH] Ignore expired cookies

[Alexander Strasser]

Hi!

On 2017-03-29 19:42 -0400, Micah Galizia wrote:
> I'm going to have to submit a v2 for this patch (hopefully soon) --
> this version only accomplishes half the job: not sending expired
> cookies. The change should also prevent storing them in the first
> place.
> 
> Regardless, thanks for your help on this one.

I noticed one or two things while reading the patch. See below.

> On Sat, Mar 25, 2017 at 7:27 PM, Micah Galizia <micahgalizia at gmail.com> wrote:
> > Signed-off-by: Micah Galizia <micahgalizia at gmail.com>
> > ---
> >  libavformat/http.c | 43 +++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 39 insertions(+), 4 deletions(-)
> >
> > diff --git a/libavformat/http.c b/libavformat/http.c
> > index 293a8a7..53fae2a 100644
> > --- a/libavformat/http.c
> > +++ b/libavformat/http.c
> > @@ -29,6 +29,7 @@
> >  #include "libavutil/avstring.h"
> >  #include "libavutil/opt.h"
> >  #include "libavutil/time.h"
> > +#include "libavutil/parseutils.h"
> >
> >  #include "avformat.h"
> >  #include "http.h"
> > @@ -48,6 +49,8 @@
> >  #define MAX_REDIRECTS 8
> >  #define HTTP_SINGLE   1
> >  #define HTTP_MUTLI    2
> > +#define MAX_EXPIRY    19
> > +#define WHITESPACES " \n\t\r"
> >  typedef enum {
> >      LOWER_PROTO,
> >      READ_HEADERS,
> > @@ -877,15 +880,20 @@ static int get_cookies(HTTPContext *s, char **cookies, const char *path,
> >
> >      *cookies = NULL;
> >      while ((cookie = av_strtok(set_cookies, "\n", &next))) {
> > -        int domain_offset = 0;
> > +        int domain_offset = 0, expired = 0;
> >          char *param, *next_param, *cdomain = NULL, *cpath = NULL, *cvalue = NULL;
> > +        char exp_buf[MAX_EXPIRY];
> >          set_cookies = NULL;
> >
> >          // store the cookie in a dict in case it is updated in the response
> >          if (parse_cookie(s, cookie, &s->cookie_dict))
> >              av_log(s, AV_LOG_WARNING, "Unable to parse '%s'\n", cookie);
> >
> > -        while ((param = av_strtok(cookie, "; ", &next_param))) {
> > +        while ((param = av_strtok(cookie, ";", &next_param))) {
> > +
> > +            // move past any leading whitespace
> > +            param += strspn(param, WHITESPACES);
> > +
> >              if (cookie) {
> >                  // first key-value pair is the actual cookie value
> >                  cvalue = av_strdup(param);
> > @@ -899,6 +907,33 @@ static int get_cookies(HTTPContext *s, char **cookies, const char *path,
> >                  int leading_dot = (param[7] == '.');
> >                  av_free(cdomain);
> >                  cdomain = av_strdup(&param[7+leading_dot]);
> > +            } else if (!av_strncasecmp("expires=", param, 8)) {
> > +                int i, j, exp_len, exp_buf_len = MAX_EXPIRY-1;
> > +                struct tm tm_buf = {0};
> > +                char *expiry = &param[8];
> > +
> > +                // strip off any punctuation or whitespace
> > +                exp_len = strlen(expiry);
> > +                for (i = 0, j = 0; i < exp_len && j < exp_buf_len; i++) {
> > +                    if ((expiry[i] >= '0' && expiry[i] <= '9') ||
> > +                        (expiry[i] >= 'A' && expiry[i] <= 'Z') ||
> > +                        (expiry[i] >= 'a' && expiry[i] <= 'z')) {
> > +                        exp_buf[j] = expiry[i];
> > +                        j++;
> > +                    }
> > +                }

If expiry is zero terminated and you are going through it one byte at a time,
you could omit the strlen call and just check if expiry[i] is non zero.

It's maybe the more common idiom too.

> > +                exp_buf[j] = '\0';
> > +                expiry = exp_buf;
> > +
> > +                // move the string beyond the day of week
> > +                while ((*expiry < '0' || *expiry > '9') && *expiry != '\0')
> > +                    expiry++;
> > +
> > +                if (av_small_strptime(expiry, "%d%b%Y%H%M%S", &tm_buf)) {
> > +                    time_t now = av_gettime() / 1000000;
> > +                    if (av_timegm(&tm_buf) < now)
> > +                        expired = 1;
> > +                }

The patch seems a bit long for what it achieves. I don't really have any
better idea for now. One thing that came to mind: you might be able to
merge the skipping-day-of-week loop into the first loop that writes the
stripped down date string into the buffer.


  Alexander

> >              } else {
> >                  // ignore unknown attributes
> >              }
> > @@ -907,9 +942,9 @@ static int get_cookies(HTTPContext *s, char **cookies, const char *path,
> >              cdomain = av_strdup(domain);
> >
> >          // ensure all of the necessary values are valid
> > -        if (!cdomain || !cpath || !cvalue) {
> > +        if (expired || !cdomain || !cpath || !cvalue ) {
> >              av_log(s, AV_LOG_WARNING,
> > -                   "Invalid cookie found, no value, path or domain specified\n");
> > +                   "Invalid cookie found, expired or no value, path or domain specified\n");
> >              goto done_cookie;
> >          }
> >
> > --
> > 2.9.3