Closed Bug 455598 Opened 12 years ago Closed 12 years ago

nsICookieManager2.cookieExists incorrectly assumes a nsCookie param

Categories

(Core :: Networking: Cookies, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla1.9.1b1

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

(Keywords: crash, verified1.9.0.4)

Attachments

(3 files)

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: 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
(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
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
Attachment #338957 - Flags: superreview?(cbiesinger)
Attachment #338957 - Flags: review?(bzbarsky)
Attachment #338957 - Flags: review+
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+
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
Closed: 12 years ago
Flags: blocking1.9.1?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
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.
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+
Yes... s/near/exact/ :-)
Comment on attachment 339062 [details] [diff] [review]
1.9.0 branch patch

extra points!
Attachment #339062 - Flags: review?(dwitte) → review+
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
(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.
Flags: in-testsuite?
(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
Can you write an xpcshell test?
(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?
Yes
(In reply to comment #20)
> Yes

That's good.  Test forthcoming.
No longer depends on: 448676
Test for both trunk and 1.9.0 branch.
Attachment #339198 - Flags: superreview?(bzbarsky)
Attachment #339198 - Flags: review?(dwitte)
Attachment #339198 - Flags: review?(dwitte) → review+
Attachment #339198 - Flags: superreview?(bzbarsky) → superreview+
Checkin needed for attachment 339198 [details] [diff] [review].
Keywords: checkin-needed
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+
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
thanks ehsan!
I assume that the unit test is passing for 1.9?
(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.
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.