667087 - Cookies cannot be stored with single character hostnames

Status: VERIFIED FIXED

(Core :: Networking: Cookies, defect)

Description

[Chris Coulson]

I'm reporting this from https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/801778

Since Firefox 3.6.18, cookies cannot be stored with single character hostnames. This is reproducible by installing the extension developer addon and doing this in the JS shell:

cm = Cc["@mozilla.org/cookiemanager;1"].getService(Ci.nsICookieManager2);
expiry = (Date.now() + 1000) * 1000;
cm.countCookiesFromHost("a"); /* Should be 0 */
cm.add("a", "/", "foo", "bar", false, false, true, expiry);
cm.countCookiesFromHost("a"); /* Is still 0, but should now be 1 */

This wasn't a problem in 3.6.17. It doesn't seem to be a problem on current nightlies or aurora either (I didn't test 5.0)

I see there's a couple of suspect commits between 3.6.17 and 3.6.18, but I'm still waiting for my mozilla-1.9.2 tree to build before I can actually verify that:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bb935ffe5ff1
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bb728fdcd717

Comment 1

[Chris Coulson]

It's definitely this one which breaks it: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bb935ffe5ff1

When running the first iteration of the loop in nsCookieService::CountCookiesFromHostInt, (nextDot <= end && *(nextDot + 1) == '\0') always evaluates to true, so it just bails out

Comment 2

[Chris Coulson]

Attachment: Fix bug 667087.patch

Here is a patch which fixes this, with test-case. I'm not sure if it's the best way to fix this without understanding what http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bb728fdcd717 is for.

Comment 3

[Chris Coulson]

Attachment: Test for mozilla-central

Comment 4

[Daniel Veditz [:dveditz]]

apparently not a problem on trunk, where we took a different patch for bug 616264

Comment 5

[[e-mail]]

Comment on attachment 542125 [details] [diff] [review]
Fix bug 667087.patch

Looks great to me. Nice fix! r=dwitte

Comment 6

[[e-mail]]

Comment on attachment 542128 [details] [diff] [review]
Test for mozilla-central

r=dwitte

Comment 7

[(no longer active)]

The ubuntu folks want this badly.  Can we get the approval on this one soon, please?

Comment 8

[(no longer active)]

I landed the m-c test on inbound.  Please do not mark the bug as FIXED when you merge to central.

Comment 9

[LegNeato]

Comment on attachment 542125 [details] [diff] [review]
Fix bug 667087.patch

Approved for 1.9.2.19

Comment 10

[Marco Bonardo [:mak] (Away 10-21 August)]

test merged http://hg.mozilla.org/mozilla-central/rev/beadf61d53a7

Comment 11

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2b1eb891d7fe

Comment 12

[Al Billings [:abillings - ex-MoCo]]

Verified fixed using 1.9.2 testcase for 1.9.2.20.

Comment 13

[AndreiD[QA]]

(In reply to comment #12)
> Verified fixed using 1.9.2 testcase for 1.9.2.20.

Changing the resolution to verified.