Cookies cannot be stored with single character hostnames

VERIFIED FIXED

Status

()

Core
Networking: Cookies
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Chris Coulson, Assigned: Ehsan)

Tracking

({regression, reproducible, verified1.9.2})

1.9.2 Branch
All
Linux
regression, reproducible, verified1.9.2
Points:
---

Firefox Tracking Flags

(firefox5 unaffected, blocking1.9.2 .20+, status1.9.2 .20-fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
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
(Reporter)

Updated

6 years ago

Updated

6 years ago
(Reporter)

Comment 1

6 years ago
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

Updated

6 years ago
Blocks: 650522
Keywords: regression, reproducible

Updated

6 years ago
blocking1.9.2: --- → ?
Hardware: x86_64 → All
(Reporter)

Comment 2

6 years ago
Created attachment 542125 [details] [diff] [review]
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.
(Reporter)

Comment 3

6 years ago
Created attachment 542128 [details] [diff] [review]
Test for mozilla-central
(Reporter)

Updated

6 years ago
Attachment #542125 - Flags: review?(dwitte)
(Reporter)

Updated

6 years ago
Attachment #542128 - Flags: review?(dwitte)
apparently not a problem on trunk, where we took a different patch for bug 616264
Assignee: nobody → ehsan
Blocks: 616264
No longer blocks: 650522
blocking1.9.2: ? → .19+
status1.9.2: --- → wanted
status-firefox5: --- → unaffected
Blocks: 650522
No longer blocks: 616264

Comment 5

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

Looks great to me. Nice fix! r=dwitte
Attachment #542125 - Flags: review?(dwitte) → review+

Comment 6

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

r=dwitte
Attachment #542128 - Flags: review?(dwitte) → review+
Attachment #542125 - Flags: approval1.9.2.19?
The ubuntu folks want this badly.  Can we get the approval on this one soon, please?
I landed the m-c test on inbound.  Please do not mark the bug as FIXED when you merge to central.
Whiteboard: DO NOT MARK AS FIXED WHEN MERGING TO CENTRAL

Comment 9

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

Approved for 1.9.2.19
Attachment #542125 - Flags: approval1.9.2.19? → approval1.9.2.19+
test merged http://hg.mozilla.org/mozilla-central/rev/beadf61d53a7
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2b1eb891d7fe
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status1.9.2: wanted → .19-fixed
Resolution: --- → FIXED
Whiteboard: DO NOT MARK AS FIXED WHEN MERGING TO CENTRAL
Verified fixed using 1.9.2 testcase for 1.9.2.20.
Keywords: verified1.9.2

Comment 13

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

Changing the resolution to verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.