549470 - LL_MAXINT expiry times of session cookies are not representable in JS

Status: NEW

(Core :: Networking: Cookies, defect, P5)

Description

[Kain]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6

I am the sole developer for the Firefox add-on "HistoryBlock." I have been trying to fix an issue concerning cookies getting destroyed when nsIPrivateBrowsingService.removeDataFromDomain(hostname) gets called. Essentially, I create an array of the cookies before I call it, then I try to call nsICookieManager2.add(host,path,name,value,isSecure,isHttpOnly,isSession,expiry) against each cookie after having "removed all data from domain".

The issue is that I will have 13 cookies for my test domain (forums.worldofwarcraft.com) before the purge, have 0 cookies for my test domain after the purge, and try to add back those 13 cookies (confirmed that I call add 13 times without any visible errors), and only 11 cookies actually get added.



Reproducible: Always

Steps to Reproduce:
http://pastebin.mozilla.org/705611
Actual Results:  
http://pastebin.mozilla.org/705584

Expected Results:  
http://pastebin.mozilla.org/705584

The first collection of cookies (count = 13) SHOULD be the same as the after results (i.e. 13 results instead of 11).

http://pastebin.mozilla.org/705612

This is the actual code I'm running (rather than a snippet from above). It should be roughly comparable, but I cannot get cookieManager2.add(<stuff>) to work for all the cookies I have.

I tried nsICookieService.setCookieString(<stuff>) as well and got the same results.

Comment 1

[Boris Zbarsky [:bzbarsky]]

dwitte, want to take a look?

Comment 2

[[e-mail]]

The snippets look like they should work; I didn't read all of the full code.

Unrelated comment:

              if(hostname.split(".").length > 2 &&
                 (cookie.host.indexOf(hostname) != -1 ||
cookie.host.indexOf(hostname.substring(hostname.indexOf("."),hostname.length)) != -1))

Shouldn't need those guards; nsICookieManager2.getCookiesFromHost will only return cookies for the given base domain.

Easiest way to figure this out would be if you turn on logging and run your code. See https://developer.mozilla.org/en/Creating_a_Cookie_Log for instructions.

Comment 3

[Kain]

Those actually aren't guards, I try and get the cookies for the hostname and the domain-wide at the same time. As I recall, without that proviso, the logs only had the "forums.worldofwarcraft.com" cookies, but the call to removeDataFromDomain(hostname) also wiped out the ".worldofwarcraft.com" cookies as well. This snippet basically kills the subdomain portion and leaves .example.com (in a very rudimentary way; it was mainly the first step in a refactoring batch I started but didn't finishing because of my issues here with cookies).

I'll try and turn on logging on and pastebin the results in a moment.

Comment 4

[Kain]

Attachment: Cookie log of bug in action

This is the step-by-step process of recreating the bug with the HistoryBlock addon installed:

1) Log into forums.worldofwarcraft.com
2) Open a new tab from one of the forum links (both tabs suggest I'm still logged in)
3) Close the newly opened tab (thus creating an array of the cookies for this domain, calling nsIPrivateBrowsingService.removeDataFromDomain("forums.worldofwarcraft.com"), and trying to add back each cookie individually)
4) Refresh the main page and see that I am no longer logged in (though 11/13 of the cookies have been created).

Comment 5

[[e-mail]]

(In reply to comment #3)
> Those actually aren't guards

My mistake; on 3.6 they're required, but on 3.7 you won't need them. But 3.7 isn't out yet. ;)

> 0[a28140]: ===== COOKIE NOT ACCEPTED =====
> 0[a28140]: request URL: 
> 0[a28140]: cookie string: (null)
> 0[a28140]: current time: Tue Mar 02 16:11:50 2010 GMT
> 0[a28140]: rejected because cookie has already expired

> 0[a28140]: ===== COOKIE NOT ACCEPTED =====
> 0[a28140]: request URL: 
> 0[a28140]: cookie string: (null)
> 0[a28140]: current time: Tue Mar 02 16:11:50 2010 GMT
> 0[a28140]: rejected because cookie has already expired

That's the problem. The JSESSIONID cookie is being rejected; further debugging shows:

First nsCookieService::AddInternal() call, when the site sets the cookie:
  expiry = 9223372036854775807 (LL_MAXINT; this is the internal default the cookie service uses for session cookies)

In JS, when getting 'cookie.expiry':
  expiry = 9223372036854776000 (xpconnect casts it to jsdouble and rounds to the nearest representable value -- jsdouble can only hold 53 mantissa bits)

Back in nsCookieService::AddInternal(), when adding the cookie a second time:
  expiry = -9223372036854775808 (xpconnect converts the jsdouble to an integer, which overflows to LL_MININT)

Arguably, the cookie service isn't smart about handing out enormous integers, which are (expectedly) unrepresentable in JS. But xpconnect could also be smarter here. I'll run this by mrbkap and see what he thinks.

Comment 6

[Kain]

So, your suggestion would be to add them back in a smarter way? Essentially, if the expiry value is too large, set it to something like (max-int - 1) and my troubles will cease?

Comment 7

[[e-mail]]

There's a bug here in our code, for sure. But as a workaround, you could do:

let expiry = cookie.expiry;
if (expiry > Math.pow(2, 62))
  expiry = Math.pow(2, 62);

As a fix for 3.6, we could just have the cookie service use 2^62 instead of LL_MAXINT for session cookies.

Comment 8

[Kain]

Can't edit comments (I guess), but here's something that doesn't make any sense
to me:

You are suggesting that the cookies not being accepted are ones that have
expired, but based on the cookies before and after (which I have attached via
pastebin in the original bug report), those aren't the ones not making it
through. Specifically, the cookies which are not being added correctly from
before the purge to after it are:

-------------------------------------------------------------------
Host; Path; Name; Value; isSecure; isHttpOnly; isSession; expiry;
-------------------------------------------------------------------

.worldofwarcraft.com; /; __utmz;
155754724.1267483848.1.1.utmcsr=(direct)|utmccn=(direct)|utmcmd=(none); false;
false; false; 1283251847;

.worldofwarcraft.com; /; opt; 1; false; false; false; 1268693448;

I have run more debug code to ensure that the expiry is after the current time,
so I am simply unsure how these two are not getting added correctly (and I am
equally unclear on how you came to the conclusion that the JSESSIONID cookie
was the one being rejected and presumably __utmc as well).

Comment 9

[Boris Zbarsky [:bzbarsky]]

If the expiry is really the problem... is there a reason negative expiry values are even allowed?  that is, a reason we don't use unsigned ints there?

Comment 10

[Kain]

I don't understand it, but I implemented the work-around and it seems to have entirely resolved my issue. 

Thanks for the support!

Comment 11

[[e-mail]]

(In reply to comment #8)
> You are suggesting that the cookies not being accepted are ones that have
> expired

The log you attached showed that __utmz was getting added fine:

> 0[a28140]: ===== COOKIE ACCEPTED =====
> 0[a28140]: request URL: 
> 0[a28140]: cookie string: (null)
> 0[a28140]: replaces existing cookie: false
> 0[a28140]: current time: Tue Mar 02 16:11:50 2010 GMT
> 0[a28140]: ----------------
> 0[a28140]: name: __utmz
> 0[a28140]: value: 155754724.1267483848.1.1.utmcsr=(direct)|utmccn=(direct)|utmcmd=(none)
> 0[a28140]: domain: .worldofwarcraft.com
> 0[a28140]: path: /
> 0[a28140]: expires: Tue Aug 31 10:50:47 2010 GMT
> 0[a28140]: created: Tue Mar 02 16:11:50 2010 GMT (id 1267546310691013)
> 0[a28140]: is secure: false
> 0[a28140]: is httpOnly: false

whereas JSESSIONID was one of the ones missing. (Unfortunately, the log doesn't show the name of cookies that were rejected, but it was clearly set further up in the log so I'm inferring that it was one of the ones that failed.)

(In reply to comment #9)
> If the expiry is really the problem... is there a reason negative expiry values
> are even allowed?  that is, a reason we don't use unsigned ints there?

Time values (PRTime, time_t, etc) can be negative, relative to the epoch -- granted, this isn't really a use case nowadays. We could also fix the bug by changing interfaces to hand out unsigned ints, but we'd have to be careful about sanitizing negative values.

Comment 12

[[e-mail]]

If we want to fix this in xpconnect, the place to do it is here:

http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpcconvert.cpp#540

LL_D2L (which simply casts) would need to be replaced by something smarter.

Comment 13

[[e-mail]]

Reassigning to nobody. If anyone wants to work on this, feel free!

Comment 14

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258