[FFmpeg-devel] Ignore expired cookies

Signed-off-by: Micah Galizia <micahgalizia@gmail.com>
---
 libavformat/http.c | 43 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 4 deletions(-)

Hi,

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.

On Sat, Mar 25, 2017 at 7:27 PM, Micah Galizia <micahgalizia@gmail.com> wrote:
> Signed-off-by: Micah Galizia <micahgalizia@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++;
> +                    }
> +                }
> +                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;
> +                }
>              } 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
>

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@gmail.com> wrote:
> > Signed-off-by: Micah Galizia <micahgalizia@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

Le decadi 10 germinal, an CCXXV, Alexander Strasser a écrit :
> 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.

On the other hand, code that does not need a 0-terminated string is more
generic.

I am thinking of the ideas I posted a few months ago about reworking the
options system to avoid escaping hell. Parsers for various types must be
able to work in the middle of strings with foreign delimiters. Code that
is already capable of working in the middle of a string would be easier
to integrate.

Of course, all this is in the far future. I will not insist on all new
code to be able to work like that, but I would like it nonetheless. And
if it is already written that way, let us keep it.

All in all, 0-terminated strings were a terrible terrible idea.

Regards,

Thanks for those comments -- I'll get rid of the strlen in the
upcoming patch but, Nicolas, you lost me there.

Anyway, this patch only does half the job -- I have a new one that is
unfortunately larger, but has taken prior advice to break cookies into
dicts.

On Thu, Mar 30, 2017 at 4:12 PM, Nicolas George <george@nsup.org> wrote:
> Le decadi 10 germinal, an CCXXV, Alexander Strasser a écrit :
>> 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.
>
> On the other hand, code that does not need a 0-terminated string is more
> generic.
>
> I am thinking of the ideas I posted a few months ago about reworking the
> options system to avoid escaping hell. Parsers for various types must be
> able to work in the middle of strings with foreign delimiters. Code that
> is already capable of working in the middle of a string would be easier
> to integrate.
>
> Of course, all this is in the far future. I will not insist on all new
> code to be able to work like that, but I would like it nonetheless. And
> if it is already written that way, let us keep it.
>
> All in all, 0-terminated strings were a terrible terrible idea.
>
> Regards,
>
> --
>   Nicolas George
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Hi Nicolas!

On 2017-03-30 22:12 +0200, Nicolas George wrote:
> Le decadi 10 germinal, an CCXXV, Alexander Strasser a écrit :
> > 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.
> 
> On the other hand, code that does not need a 0-terminated string is more
> generic.

  This code depends already on a 0-terminated string. Otherwise the call of
strlen before would be wrong. Also the later loop is also written against
a 0-terminated string. IMHO for this patch it's not really important
to avoid relying on 0-terminated string.


> I am thinking of the ideas I posted a few months ago about reworking the
> options system to avoid escaping hell. Parsers for various types must be
> able to work in the middle of strings with foreign delimiters. Code that
> is already capable of working in the middle of a string would be easier
> to integrate.
> 
> Of course, all this is in the far future. I will not insist on all new
> code to be able to work like that, but I would like it nonetheless. And
> if it is already written that way, let us keep it.

  I am not in general against getting rid of 0-terminated string in
some places. I often thought of this too.


> All in all, 0-terminated strings were a terrible terrible idea.

Yes, they probably still are a bad idea.


  Alexander