105039 - set-cookie max-age field ignored

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[patrick]

The Max-Age field in the Set-Cookie http header is ignored

Comment 1

[Stephen P. Morse]

Attachment: process max-age field

Comment 2

[Stephen P. Morse]

cc'ing alecf and sgehani for reviews

Comment 3

[Samir Gehani]

Comment on attachment 53775 [details] [diff] [review]
process max-age field

r=sgehani

It would be nice to mention section 10.1.2 of RFC 2109 in the (first) comment about the expires attribute for other developer's reference.

Comment 4

[Alec Flett]

ack, I must have been bitten by the bugzilla-IP address password thing
What I meant to say was: isn't there a well-known way to convert strings to
integers? looking at the code, it looks like we use atoi() all over the place,
and I can't see any reason that it's inherently bad...

Comment 5

[Stephen P. Morse]

Alecf,

The reason not to use atoi is that we have to first get the sequence to be null 
terminated.  That would involve stepping through the string looking for a 
terminator (null or semicolon), saving the character at that termination 
position, replacing the character with a null, doing the atoi, and then 
restoring the original character.  Since we have to step through the string 
anyway, it seems simpler to just exract the value as we are stepping and be done 
with it.

The code for doing what I described is already being done for the "expire" 
attribute and it looks as follows.  Do you really want this?

    for(ptr=date; *ptr != '\0'; ptr++) {
      if(*ptr == ';') {
        origLast = ';';
        *ptr = '\0';
        break;
      }
    }
    expires = cookie_ParseDate(date);
    *ptr=origLast;

Comment 6

[Alec Flett]

Comment on attachment 53775 [details] [diff] [review]
process max-age field

yikes. I wasn't implying we should munge the string. in fact the munging you describe freaks me out a bit, I'm wondering if we should fix it up.
I didn't realize atoi required the string to be terminated before the first non-digit character.
sr=alecf

Comment 7

[Samir Gehani]

I wondered about this too but didn't think much of it since Steve's
implementation appears correct and efficient.  However, for reference, on
windows and linux atoi() does not require the input string to be null
terminated.  From the atoi() man page:
       The atoi() function converts the initial  portion  of  the
       string  pointed  to  by nptr to int.  The behaviour is the
       same as

              strtol(nptr, (char **)NULL, 10);

       except that atoi() does not detect errors.

Comment 8

[Stephen P. Morse]

I wasn't aware that atoi didn't need null termination.  OK, I'll revise the 
patch.

Comment 9

[Stephen P. Morse]

Attachment: patch using atoi

Comment 10

[Samir Gehani]

Comment on attachment 54088 [details] [diff] [review]
patch using atoi

>+    ptr += PL_strlen(MAXAGE);
>+    gmtCookieExpires = get_current_time() + atoi(*ptr);

I don't think we want to deref |ptr| when it is passed 
into atoi().

Comment 11

[Stephen P. Morse]

Attachment: fix compilation error in previous patch

Comment 12

[Stephen P. Morse]

Samir, you were too quick.  I had posted the patch before compiling it (because 
my tree was building) and was waiting for the build to finish before asking you 
to re-review it.

alecf, sgehani, you can re-review it now -- it has successfully compiled.

Comment 13

[Alec Flett]

Comment on attachment 54122 [details] [diff] [review]
fix compilation error in previous patch

ok, finally we have something nice :) sr=alecf

Comment 14

[Samir Gehani]

Comment on attachment 54122 [details] [diff] [review]
fix compilation error in previous patch

r=sgehani again :o)

Comment 15

[Stephen P. Morse]

fix checked in