Closed Bug 1002702 Opened 6 years ago Closed 6 years ago

"Assertion failure: !mWorkerPrivate" with port.close(), GC

Categories

(Core :: DOM: Workers, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla33
Tracking Status
firefox30 --- wontfix
firefox31 --- verified
firefox32 --- verified
firefox33 --- verified
firefox-esr24 --- unaffected
b2g-v1.3 --- unaffected
b2g-v1.3T --- unaffected
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: jruderman, Assigned: baku)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, sec-high, testcase, Whiteboard: [adv-main31+])

Attachments

(3 files, 3 obsolete files)

Attached file testcase
1. Load the testcase
2. Use about:memory to force a GC

OR 

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

Assertion failure: !mWorkerPrivate, at dom/workers/SharedWorker.cpp:43

Security-sensitive because (1) it's similar to bug 998474 and (2) it involves GC.
Attached file stack
It looks like this assertion means we've failed to run some kind of cleanup code, which sounds bad, so I'm going to set this to sec-high.  Please adjust as appropriate.
Keywords: sec-high
Can you look into this Andrea?  Thanks.
Flags: needinfo?(amarchesini)
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attached patch sw.patch (obsolete) — Splinter Review
The reason of this crash is because we call "Close()" only when we Unlink is running becaue of GC. This is not enough for this test-case. This patch forces Close() in the dtor of SharedWorker.
Attachment #8424930 - Flags: review?(nsm.nikhil)
Comment on attachment 8424930 [details] [diff] [review]
sw.patch

It's probably better to have khuey sanity-check this.
Attachment #8424930 - Flags: review?(nsm.nikhil) → review?(khuey)
Attached patch sw.patch (obsolete) — Splinter Review
Mochitest added
Attachment #8424930 - Attachment is obsolete: true
Attachment #8424930 - Flags: review?(khuey)
Attachment #8424981 - Flags: review?(khuey)
Group: dom-core-security
Comment on attachment 8424981 [details] [diff] [review]
sw.patch

Review of attachment 8424981 [details] [diff] [review]:
-----------------------------------------------------------------

You haven't actually run the test ...
Attachment #8424981 - Flags: review?(khuey) → review-
Attached patch sw.patch (obsolete) — Splinter Review
0:05.59 TEST-PASS | unknown test url | No crash \o/

I don't know why I lost an 's' in SpecialPowers saving the test file.
Can you review it again, please?
Attachment #8424981 - Attachment is obsolete: true
Attachment #8427920 - Flags: review?(khuey)
Attached patch sw.patchSplinter Review
Still the 's' was missing :)
Attachment #8427920 - Attachment is obsolete: true
Attachment #8427920 - Flags: review?(khuey)
Attachment #8427965 - Flags: review?(khuey)
Kyle, can you review this patch, or should somebody else?  It has been sitting here for over a month.
Flags: needinfo?(khuey)
This is a bit weird, it means that error events stop being delivered to the SharedWorker once close() is called on the port object... Since there's no other way to disconnect the SharedWorker, and this is just error events, I guess this is ok for now.
This appears to affect more than trunk, so it should have gotten sec-approval, though it is too late now.  What versions does this affect?
Flags: needinfo?(amarchesini)
(In reply to Andrew McCreight [:mccr8] from comment #13)
> This appears to affect more than trunk, so it should have gotten
> sec-approval, though it is too late now.  What versions does this affect?

Sorry, I completely forgot the set-approval. I would say: ff 29 till now.
Flags: needinfo?(amarchesini)
https://hg.mozilla.org/mozilla-central/rev/9dafb9b56b5c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Not only did this land without sec-approval, it landed with a test too.
Group: dom-core-security
Please try to get beta and aurora approval for this.  We're almost at the final beta for this cycle.
Flags: needinfo?(amarchesini)
Comment on attachment 8427965 [details] [diff] [review]
sw.patch

Andrea is on PTO.

Approval Request Comment
[Feature/regressing bug #]: bug 924089
[User impact if declined]: It's probably a use-after-free opportunity, so crash or exploit is possible
[Describe test coverage new/current, TBPL]: There's a test in the tree already...
[Risks and why]: Should be low risk, this just removes a dangling pointer from a hashtable.
[String/UUID change made/needed]: None
Attachment #8427965 - Flags: approval-mozilla-beta?
Attachment #8427965 - Flags: approval-mozilla-aurora?
Flags: needinfo?(amarchesini)
Attachment #8427965 - Flags: approval-mozilla-beta?
Attachment #8427965 - Flags: approval-mozilla-beta+
Attachment #8427965 - Flags: approval-mozilla-aurora?
Attachment #8427965 - Flags: approval-mozilla-aurora+
Reproduced the original issue on OSX 10.9.4, Ubuntu 13.10 and Win 8.1 using the STR from comment #0. Used build:
* http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-28-03-02-03-mozilla-central/

Went through verification using the following builds:

* fx-33.0a1 (Nightly)
** OSX 10.9.4 x64: [Passed]
** Ubuntu 13.10 x64: [Passed]
** Win 8.1 x64: [Passed]
** Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-07-08-03-02-03-mozilla-central/

fx-32.0a2 (Aurora)
** OSX 10.9.4 x64: [Passed]
** Ubuntu 13.10 x64: [Passed]
** Win 8.1 x64: [Passed]
** Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-07-08-00-40-01-mozilla-aurora/

* fx-31.0b8 (BETA)
** OSX 10.9.4 x64: [Passed]
** Ubuntu 13.10 x64: [Passed]
** Win 8.1 x64: [Passed]
** Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/31.0b8/

- opened the poc and waited a few minutes and ensured that the browser wasn't crashing
- opened the poc using multiple tabs and windows and ensured that the browser wasn't crashing
- opened several tabs and windows and invoked the GC through about:memory, ensured that the browser wasn't crashing
Whiteboard: [adv-main31+]
Blocks: 1183926
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.