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

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 2

[[e-mail]]

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.

Comment 3

[[e-mail]]

Attachment: patch v1

Check and assert the precondition that host is never empty or ".", and add xpcshell tests to that effect.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

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)

Comment 5

[[e-mail]]

Attachment: 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.)

Comment 6

[Phil Ringnalda (:philor)]

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.

Comment 7

[Reed Loden [:reed]]

(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 8

[Daniel Veditz [:dveditz]]

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.

Comment 9

[[e-mail]]

Attachment: 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.

Comment 10

[[e-mail]]

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

Comment 11

[[e-mail]]

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 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 411834 [details] [diff] [review]
patch v3

a192=beltzner, I think the reward outweighs the risk here

Comment 13

[[e-mail]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3e885e496787

Comment 14

[[e-mail]]

Comment on attachment 411834 [details] [diff] [review]
patch v3

See previous comments for risk details.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 411834 [details] [diff] [review]
patch v3

Approved for 1.9.1.8, a=dveditz for release-drivers

Comment 16

[[e-mail]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5825a6b111a5

Comment 17

[William J. Edney]

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