nsICookieManager2.cookieExists incorrectly assumes a nsCookie param

VERIFIED FIXED in mozilla1.9.1b1

Status

()

Core
Networking: Cookies
--
critical
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

({crash, verified1.9.0.4})

Trunk
mozilla1.9.1b1
crash, verified1.9.0.4
Points:
---
Bug Flags:
blocking1.9.0.4 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

10 years ago
Created attachment 338957 [details] [diff] [review]
Patch (v1)
[Checkin: Comment 5]

The implementation of nsICookieManager2.cookieExists incorrectly uses static_cast to cast the nsICookie2 parameter to an nsCookie pointer, and this results in a crash when called from JavaScript (and it can possibly lead to other crashes, if some C++ code decides to implements its own nsICookie2 and pass it to cookieExists.

For an example of the crash, see bug 248970 comment 257.  This patch makes the implementation not dependent on nsCookie.
Attachment #338957 - Flags: review?(bzbarsky)
(Assignee)

Updated

10 years ago
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
I imagine the fast path exists for a reason here, so you might want to keep it with a private interface to QI to.
Assignee: ehsan.akhgari → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 2

10 years ago
(In reply to comment #1)
> I imagine the fast path exists for a reason here, so you might want to keep it
> with a private interface to QI to.

We don't seem to be using this method anywhere in the tree <http://mxr.mozilla.org/mozilla-central/ident?i=cookieExists&tree=mozilla-central&filter=&scriptidly=1>.  Do you still think that the fast path exists intentionally?
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
(Assignee)

Comment 3

10 years ago
This can affect extensions authors, both on trunk and 1.9.0 branch, so I'm requesting blocking on both.
Flags: blocking1.9.1?
Flags: blocking1.9.0.3?
Severity: normal → critical

Updated

10 years ago
Attachment #338957 - Flags: superreview?(cbiesinger)
Attachment #338957 - Flags: review?(bzbarsky)
Attachment #338957 - Flags: review+

Comment 4

10 years ago
Comment on attachment 338957 [details] [diff] [review]
Patch (v1)
[Checkin: Comment 5]

it is used, here: http://mxr.mozilla.org/mozilla/source/extensions/cookie/nsCookiePermission.cpp#368

the fast path isn't that important here, so r=me. hitting up biesi for sr to cut bz some slack...

thanks for the patch!
Attachment #338957 - Flags: superreview?(cbiesinger) → superreview+
Keywords: checkin-needed
Comment on attachment 338957 [details] [diff] [review]
Patch (v1)
[Checkin: Comment 5]

http://hg.mozilla.org/mozilla-central/rev/fe79a762d6fe
Attachment #338957 - Attachment description: Patch (v1) → Patch (v1) [Checkin: Comment 5]
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: blocking1.9.1?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
(Assignee)

Comment 6

10 years ago
Thanks for the check-in, Serge!

I was going to write a crash test for this as well, but unfortunately the reftest framework seems to be limited to non-privileged content (or I don't know how to work around it) and testing this requires privileged code.  Is there any other way to test this?

I'll post a patch for the branch soon.
(Assignee)

Comment 7

10 years ago
Created attachment 339062 [details] [diff] [review]
1.9.0 branch patch

I'm not quite sure whether the branch patch needs its own s/sr (it's nearly identical to the trunk patch), but I'm requesting them anyway to be on the safe side. :-)
Attachment #339062 - Flags: superreview?(bzbarsky)
Attachment #339062 - Flags: review?(dwitte)
Comment on attachment 339062 [details] [diff] [review]
1.9.0 branch patch

Isn't this exactly identical?
Attachment #339062 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 9

10 years ago
Yes... s/near/exact/ :-)

Comment 10

10 years ago
Comment on attachment 339062 [details] [diff] [review]
1.9.0 branch patch

extra points!
Attachment #339062 - Flags: review?(dwitte) → review+
(Assignee)

Comment 11

10 years ago
Comment on attachment 339062 [details] [diff] [review]
1.9.0 branch patch

This bug can cause crashes if an extension uses this public cookie manager API.  The fix is simple and well-contained, and low-risk.  Nominating for approval to land on 1.9.0.3.
Attachment #339062 - Flags: approval1.9.0.3?
(In reply to comment #6)
> I was going to write a crash test for this as well, but unfortunately the
> reftest framework seems to be limited to non-privileged content (or I don't
> know how to work around it) and testing this requires privileged code.  Is
> there any other way to test this?

Martijn, do you have an answer to Ehsan's question?
You could use the mochitest framework, instead: http://developer.mozilla.org/en/Mochitest
(Assignee)

Comment 14

10 years ago
(In reply to comment #13)
> You could use the mochitest framework, instead:
> http://developer.mozilla.org/en/Mochitest

Can it perform crash tests?  In other words, what happens when the test fails?  Will the whole mochitest session crash?  Is this detectable and does it provide useful output on the Tinderbox?
If it crashes, the test has failed. The whole mochitest session will stop at that point. Afaik, tinderbox reports this. If you want to know more, perhaps you should ask on irc in #developers. They know more about this stuff than I do.
Probably it could look similar to all the other crash tests, e.g. http://mxr.mozilla.org/mozilla-central/source/layout/tables/crashtests/404301-1.xhtml
Flags: in-testsuite?
(Assignee)

Comment 17

10 years ago
(In reply to comment #16)
> Probably it could look similar to all the other crash tests, e.g.
> http://mxr.mozilla.org/mozilla-central/source/layout/tables/crashtests/404301-1.xhtml

I would have liked it as well, but it's not possible.  Such crash tests are also loaded by the reftest framework, and it's not possible to get UniversalXPConnect privileges in reftests, which is needed in order to reproduce the crash in this bug.  Bug 448676 tracks enabling UniversalXPConnect in the reftests (which would happen other reftests as well), so I'm marking it as a blocker here.

Regarding Martijn's suggestion in comment 15, I'm going to need to ask in #developers to see if a Mochitest would be feasible, but I don't remember seeing a Mochitest for testing a crash...

Worst case, I can create an extension to use this API in js and produce a crash, and we can add a Litmus test case to install that extension and verifying that the browser doesn't crash, but that wouldn't be so elegant... :-/

CCing sayrer to see if he can help here.
Depends on: 448676

Comment 18

10 years ago
Can you write an xpcshell test?
(Assignee)

Comment 19

10 years ago
(In reply to comment #18)
> Can you write an xpcshell test?

Yes, sure.  Will crash for an xpcshell test be correctly reported by the Tinderbox as the failure of that test case?

Comment 20

10 years ago
Yes
(Assignee)

Comment 21

10 years ago
(In reply to comment #20)
> Yes

That's good.  Test forthcoming.
(Assignee)

Updated

10 years ago
No longer depends on: 448676
(Assignee)

Comment 22

10 years ago
Created attachment 339198 [details] [diff] [review]
Test
[Checkin: Comment 25]

Test for both trunk and 1.9.0 branch.
Attachment #339198 - Flags: superreview?(bzbarsky)
Attachment #339198 - Flags: review?(dwitte)

Updated

10 years ago
Attachment #339198 - Flags: review?(dwitte) → review+
Attachment #339198 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 23

10 years ago
Checkin needed for attachment 339198 [details] [diff] [review].
Keywords: checkin-needed
(Assignee)

Comment 24

10 years ago
Comment on attachment 339198 [details] [diff] [review]
Test
[Checkin: Comment 25]

This is the unit test for the main patch.  This should land on branch together with the main patch (attachment 339062 [details] [diff] [review]).
Attachment #339198 - Attachment description: Test → Test (for checkin)
Attachment #339198 - Flags: approval1.9.0.3?
Comment on attachment 339198 [details] [diff] [review]
Test
[Checkin: Comment 25]

http://hg.mozilla.org/mozilla-central/rev/8e60ce0be3e6
Attachment #339198 - Attachment description: Test (for checkin) → Test [Checkin: Comment 25]
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Flags: blocking1.9.0.4? → blocking1.9.0.4+
Comment on attachment 339062 [details] [diff] [review]
1.9.0 branch patch

Approved for 1.9.0.4, a=dveditz for release-drivers
Attachment #339062 - Flags: approval1.9.0.4? → approval1.9.0.4+
Attachment #339198 - Flags: approval1.9.0.4? → approval1.9.0.4+
(Assignee)

Comment 27

10 years ago
Landed on CVS trunk (1.9 branch):

Checking in netwerk/cookie/src/nsCookieService.cpp;
/cvsroot/mozilla/netwerk/cookie/src/nsCookieService.cpp,v  <--  nsCookieService.cpp
new revision: 1.95; previous revision: 1.94
done
RCS file: /cvsroot/mozilla/netwerk/test/unit/test_bug455598.js,v
done
Checking in netwerk/test/unit/test_bug455598.js;
/cvsroot/mozilla/netwerk/test/unit/test_bug455598.js,v  <--  test_bug455598.js
initial revision: 1.1
done
Keywords: fixed1.9.0.4

Comment 28

10 years ago
thanks ehsan!
I assume that the unit test is passing for 1.9?
(Assignee)

Comment 30

10 years ago
(In reply to comment #29)
> I assume that the unit test is passing for 1.9?

Yes, why?

See <http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.0/1224785686.1224788396.19521.gz> for example...
I'm tasked with verifying the fix but there are no manual steps, just the unit test. I wanted to be doubly sure things were good before marking this as verified for 1.9.0.4. I will do so now since I can see it passing in the log file.
Keywords: fixed1.9.0.4 → verified1.9.0.4
Test passes on Linux, Windows, and OS X. Marking as verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.