Last Comment Bug 736589 - Crash [@ nsDOMStorage::GetNamedItem] with sessionStorage, GC
: Crash [@ nsDOMStorage::GetNamedItem] with sessionStorage, GC
Status: VERIFIED FIXED
[sg:critical][qa!]
: crash, testcase
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- critical (vote)
: mozilla14
Assigned To: Honza Bambas (:mayhemer)
:
:
Mentors:
Depends on:
Blocks: 326633 594645
  Show dependency treegraph
 
Reported: 2012-03-16 13:49 PDT by Jesse Ruderman
Modified: 2012-06-21 13:11 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified
+
verified
12+
verified


Attachments
testcase (requires extension for GC) (297 bytes, text/html)
2012-03-16 13:49 PDT, Jesse Ruderman
no flags Details
stack trace (5.96 KB, text/plain)
2012-03-16 13:50 PDT, Jesse Ruderman
no flags Details
v1 for m-c and m-a (6.33 KB, patch)
2012-03-26 17:39 PDT, Honza Bambas (:mayhemer)
jst: review+
akeybl: approval‑mozilla‑aurora+
honzab.moz: checkin+
Details | Diff | Splinter Review
v1 for m-b and m-r (7.02 KB, patch)
2012-03-26 18:20 PDT, Honza Bambas (:mayhemer)
jst: review+
Details | Diff | Splinter Review
v1.1 for m-b and m-r (7.17 KB, patch)
2012-03-27 15:58 PDT, Honza Bambas (:mayhemer)
honzab.moz: review+
akeybl: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
honzab.moz: checkin+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-03-16 13:49:35 PDT
Created attachment 606719 [details]
testcase (requires extension for GC)

1. Install https://www.squarefree.com/extensions/domFuzzLite3.xpi
2. Load the testcase
3. Click the button

Result: Firefox crashes in one of the following ways:
  [@ nsDOMStorage::GetNamedItem] calling a random address
  [@ nsDOMStorage::CacheStoragePermissions] accessing null
Comment 1 Jesse Ruderman 2012-03-16 13:50:10 PDT
Created attachment 606720 [details]
stack trace
Comment 2 Honza Bambas (:mayhemer) 2012-03-19 13:20:11 PDT
Thanks a lot for the test case!

I identified the cause and I more or less know how to fix this.

I can crash consistently on access to deleted mSecurityChecker that is a raw non-addrefing pointer.

I can see we create nsDOMStorage2 and set its mStorage to a new nsDOMStorage that gets set mSecurityChecker to the nsDOMStorage2 instance.  Later we fork nsDOMStorage2 to a new nsDOMStorage2 instance (the forking mechanism is part of storage event implementation).  Later the first nsDOMStorage2 instance get released (no longer needed).  But we still refer it in nsDOMStorage->mSecurityChecker.

A valid fix is to fix bug 732708.  But that is not acceptable on branches since depends on removal of globalStorage (bug 687579).

Solutions for branches:
- change the mSecurityChecker referencing (either addref it when not |this| or let forks ref the original instance) / easy+dangerous
- change the algorithm for security decision ; there is also mEventBroadcaster (also about to be removed as part of bug 732708) that could suffer the same way, so needs to be changed as well / more work+safer
Comment 3 Daniel Veditz [:dveditz] 2012-03-22 13:17:55 PDT
If bug 732708 is coming soon (say Firefox 13, the release globalStorage was removed) we could go with easy+dangerous for Firefox 12. If bug 732708 is going to take a while then we should go with "more work+safer".
Comment 4 Honza Bambas (:mayhemer) 2012-03-22 13:25:45 PDT
I have a wip patch that needs testing and actually a separate slightly modified version for each channel.  But it is doable.
Comment 5 Honza Bambas (:mayhemer) 2012-03-22 19:04:52 PDT
Try run for mozilla-central (just fresh):

https://tbpl.mozilla.org/?tree=Try&rev=42c9415848a2
Comment 6 Honza Bambas (:mayhemer) 2012-03-26 17:39:26 PDT
Created attachment 609555 [details] [diff] [review]
v1 for m-c and m-a

This applies cleanly to aurora (13) as well.  There had been two bugs interfering with this landed on version 13: bug 495337 and bug 687579.

So I need a bit different patch for version 12 and 11 (beta and release).
Comment 7 Honza Bambas (:mayhemer) 2012-03-26 18:20:59 PDT
Created attachment 609571 [details] [diff] [review]
v1 for m-b and m-r

This preserves security check for globalStorage and sessionStorage using just domain, as we currently do on Beta and Release channels.
Comment 8 Honza Bambas (:mayhemer) 2012-03-26 18:21:20 PDT
(I cannot push to try, some ssh server issues)
Comment 9 Honza Bambas (:mayhemer) 2012-03-26 18:40:07 PDT
Comment on attachment 609571 [details] [diff] [review]
v1 for m-b and m-r

https://tbpl.mozilla.org/?tree=Try&rev=1d2d565ecd0d
Comment 10 Honza Bambas (:mayhemer) 2012-03-26 18:43:54 PDT
Comment on attachment 609571 [details] [diff] [review]
v1 for m-b and m-r

https://tbpl.mozilla.org/?tree=Try&rev=d368596306d3 (m-r)
Comment 11 Honza Bambas (:mayhemer) 2012-03-27 15:58:17 PDT
Created attachment 609929 [details] [diff] [review]
v1.1 for m-b and m-r

Linux didn't like missing return for default: case.  Otherwise identical to v1.

Try for this particular patch: https://tbpl.mozilla.org/?tree=Try&rev=37902a4693f9 (on m-b branch)
Comment 12 Honza Bambas (:mayhemer) 2012-03-27 16:01:27 PDT
Comment on attachment 609555 [details] [diff] [review]
v1 for m-c and m-a

[Approval Request Comment]
Regression caused by (bug #): 495112
User impact if declined: potentially exploitable crash
Testing completed (on m-c, etc.): none so far landed, now having only try results
Risk to taking this patch (and alternatives if risky): low, the logic is identical, just not using unsafe dereference anymore
String changes made by this patch: none
Comment 13 Honza Bambas (:mayhemer) 2012-03-27 16:05:05 PDT
Comment on attachment 609929 [details] [diff] [review]
v1.1 for m-b and m-r

[Approval Request Comment]
Regression caused by (bug #): 495112
User impact if declined: potentially exploitable crash
Testing completed (on m-c, etc.): none so far, now having only try results (this patch is for beta and release only, so it cannot be tested on m-c or m-a)
Risk to taking this patch (and alternatives if risky): low, the logic is identical, just not using unsafe dereference anymore
String changes made by this patch: none
Comment 14 Honza Bambas (:mayhemer) 2012-03-27 16:06:57 PDT
Side note: there is also mEventBroadcaster member (raw ptr).  That is safe to use, since it is either null or set immediately before use to an object valid for the time of the operation with mEventBroadcaster.
Comment 15 Honza Bambas (:mayhemer) 2012-03-27 16:28:32 PDT
Comment on attachment 609555 [details] [diff] [review]
v1 for m-c and m-a

https://hg.mozilla.org/integration/mozilla-inbound/rev/3eea0725665e
Comment 16 Honza Bambas (:mayhemer) 2012-03-28 04:43:44 PDT
Comment on attachment 609929 [details] [diff] [review]
v1.1 for m-b and m-r

https://tbpl.mozilla.org/?tree=Try&rev=e011283d7a2e (m-b, android, xul)
Comment 17 Ed Morley [:emorley] 2012-03-28 14:04:23 PDT
(In reply to Honza Bambas (:mayhemer) from comment #15)
> Comment on attachment 609555 [details] [diff] [review]
> v1 for m-c and m-a
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3eea0725665e

https://hg.mozilla.org/mozilla-central/rev/3eea0725665e
Comment 18 Alex Keybl [:akeybl] 2012-03-29 16:25:33 PDT
Comment on attachment 609929 [details] [diff] [review]
v1.1 for m-b and m-r

[Triage Comment]
Approved for Beta 12 and Aurora 13 - we need to make sure to land this on the ESR branch as well, since 10.0.4 will be shipped simultaneously with FF12.
Comment 19 Honza Bambas (:mayhemer) 2012-04-02 07:56:03 PDT
Comment on attachment 609555 [details] [diff] [review]
v1 for m-c and m-a

https://hg.mozilla.org/releases/mozilla-aurora/rev/b5ac0b502079
Comment 20 Honza Bambas (:mayhemer) 2012-04-02 07:56:31 PDT
Comment on attachment 609929 [details] [diff] [review]
v1.1 for m-b and m-r

https://hg.mozilla.org/releases/mozilla-beta/rev/be73dfc96f93
Comment 21 Honza Bambas (:mayhemer) 2012-04-02 10:08:19 PDT
Comment on attachment 609929 [details] [diff] [review]
v1.1 for m-b and m-r

Try on esr10:
https://tbpl.mozilla.org/?tree=Try&rev=400d24cd8799
Comment 22 Honza Bambas (:mayhemer) 2012-04-02 14:32:50 PDT
Comment on attachment 609929 [details] [diff] [review]
v1.1 for m-b and m-r

https://hg.mozilla.org/releases/mozilla-esr10/rev/cea3a82f3f24

(In reply to Alex Keybl [:akeybl] from comment #18)
> Comment on attachment 609929 [details] [diff] [review]
> v1.1 for m-b and m-r
> 
> [Triage Comment]
> Approved for Beta 12 and Aurora 13 - we need to make sure to land this on
> the ESR branch as well, since 10.0.4 will be shipped simultaneously with
> FF12.

I took this as a+ for esr10, but now I spot the flag.  I presume it should be set (postfacto, sorry).
Comment 23 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-03 11:50:05 PDT
Comment on attachment 609929 [details] [diff] [review]
v1.1 for m-b and m-r

[Triage Comment]
post-facto a+ :)
Comment 24 Huzaifa Sidhpurwala 2012-04-19 23:23:54 PDT
any CVE for this one?
Comment 25 Vlad [QA] 2012-05-21 07:13:55 PDT
This bug has to be verified on a debug build?
I've followed the steps from the description on Firefox 11 beta and I didn't got no crash, so I'm guessing that I'm doing something wrong.

Tracy can you help with some clarifications?

Thanks
Comment 26 Tracy Walker [:tracy] 2012-05-21 07:21:05 PDT
If no crash occurred when clicking the "Start test" button of the test case, then that is verified fixed.
Comment 27 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-21 13:11:03 PDT
Verified fixed in Firefox 10.0.6esrpre 2012-06-21.

Note You need to log in before you can comment on or make changes to this bug.