The default bug view has changed. See this FAQ.

UMR in nsCookieService::CountCookiesFromHostInternal, GetCookiesFromHost, and GetCookieInternal when given empty host string

RESOLVED FIXED

Status

()

Core
Networking: Cookies
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: mrbkap, Assigned: dwitte@gmail.com)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(status1.9.2 beta5-fixed, status1.9.1 .8-fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

14.09 KB, patch
dwitte@gmail.com
: review+
beltzner
: approval1.9.2+
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
Since I don't actually believe we purposely pass these functions an empty string, I think I ran out of memory allocating a string and we passed that empty string to GetCookiesFromHost.

While the request is nonsensical, we should deal more gracefully with this condition.
(Assignee)

Updated

8 years ago
Duplicate of this bug: 527437
(Assignee)

Comment 2

8 years ago
This applies to 1.9.1, 1.9.2, and trunk. It may be the cause of some of the crashreports in cookieservice, too. We should fix this ASAP and see what the stats say.
OS: Mac OS X → All
Hardware: x86 → All
Summary: nsCookieService::CountCookiesFromHostInternal and nsCookieService::GetCookiesFromHost read uninitialized memory in OOM-type conditions → UMR in nsCookieService::CountCookiesFromHostInternal, GetCookiesFromHost, and GetCookieInternal when given empty host string
Version: 1.9.1 Branch → Trunk
(Assignee)

Comment 3

8 years ago
Created attachment 411270 [details] [diff] [review]
patch v1

Check and assert the precondition that host is never empty or ".", and add xpcshell tests to that effect.
Assignee: nobody → dwitte
Status: NEW → ASSIGNED
Attachment #411270 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #411270 - Flags: review? → review?(cbiesinger)
Comment on attachment 411270 [details] [diff] [review]
patch v1

+++ b/netwerk/cookie/src/nsCookieService.cpp
+  NS_ENSURE_TRUE(!aDomain.IsEmpty() && !aDomain.EqualsLiteral("."), NS_ERROR_INVALID_ARG);

80 characters, please (in a few places)
Attachment #411270 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 5

8 years ago
Created attachment 411316 [details] [diff] [review]
patch v2

http://hg.mozilla.org/mozilla-central/rev/a696d331ebf7

Requesting approval for 1.9.2 and 1.9.1 branch - this will probably clear up a bunch of crashes. (Not really enough to measure from trunk usage, though, so we should get this on 1.9.2 if we want to get an accurate measurement.)
Attachment #411270 - Attachment is obsolete: true
Attachment #411316 - Flags: review+
Attachment #411316 - Flags: approval1.9.2?
Attachment #411316 - Flags: approval1.9.1.6?
Backed you out on the trunk, for failures like http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1257819241.1257823829.5261.gz&fulltext=1 on very nearly every everythingelse run after your push.
(In reply to comment #6)
> Backed you out on the trunk, for failures like
> http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1257819241.1257823829.5261.gz&fulltext=1
> on very nearly every everythingelse run after your push.

You also caused a Txul regression. ;)
Comment on attachment 411316 [details] [diff] [review]
patch v2

not going to make 1.9.1.6 -- you can try again for the next release after trunk/1.9.2 landing and testing.
Attachment #411316 - Flags: approval1.9.1.6? → approval1.9.1.6-
(Assignee)

Updated

8 years ago
Attachment #411316 - Flags: approval1.9.2?
(Assignee)

Comment 9

8 years ago
Created attachment 411834 [details] [diff] [review]
patch v3

Tests failed (in other parts of the tree) because of the behavior change here. Having empty hosts is common (e.g. file:// URI's). This patch is more conservative; it maintains behavior for CountCookiesFromHost and GetCookiesFromHost by having them handle the trivial case. For Add and Remove, the cookie data is most likely coming from either the cookieservice itself or some other cookie data source, so we probably want to throw rather than silently fail.
Attachment #411316 - Attachment is obsolete: true
Attachment #411834 - Flags: review+
(Assignee)

Comment 10

8 years ago
http://hg.mozilla.org/mozilla-central/rev/a47840c2d742
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Attachment #411834 - Flags: approval1.9.2?
(Assignee)

Comment 11

7 years ago
Note for branch approval - reward: clears up a bunch of UMRs, and potentially crashes, caused when passing empty host strings into cookieservice API methods; risk: we now explicitly throw in two of those methods, where previously we pretended things were fine. This could break addons if they're not catching those exceptions, but this is arguably the right thing to do.
Comment on attachment 411834 [details] [diff] [review]
patch v3

a192=beltzner, I think the reward outweighs the risk here
Attachment #411834 - Flags: approval1.9.2? → approval1.9.2+
(Assignee)

Comment 13

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3e885e496787
status1.9.2: --- → final-fixed
(Assignee)

Comment 14

7 years ago
Comment on attachment 411834 [details] [diff] [review]
patch v3

See previous comments for risk details.
Attachment #411834 - Flags: approval1.9.1.7?
Comment on attachment 411834 [details] [diff] [review]
patch v3

Approved for 1.9.1.8, a=dveditz for release-drivers
Attachment #411834 - Flags: approval1.9.1.8? → approval1.9.1.8+
(Assignee)

Comment 16

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5825a6b111a5
status1.9.1: --- → .8-fixed

Comment 17

7 years ago
All -

I just filed this regression for FF 3.6b5:

https://bugzilla.mozilla.org/show_bug.cgi?id=536650

I'm not sure if that is related to this, but out of all of the issues that were resolved for FF 3.6b5, this bug seemed the most likely.

Apologies in advance if that is not the case.

Cheers,

- Bill
(Assignee)

Updated

7 years ago
Blocks: 536650
You need to log in before you can comment on or make changes to this bug.