Closed
Bug 723465
Opened 13 years ago
Closed 13 years ago
Investigate if ListBase<LC>::finalize should use deferred release
Categories
(Core :: XPConnect, defect)
Tracking
()
VERIFIED
FIXED
mozilla15
People
(Reporter: smaug, Assigned: peterv)
References
Details
(Keywords: sec-critical, Whiteboard: [sg:critical][advisory-tracking+])
Attachments
(1 file)
3.52 KB,
patch
|
jst
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
akeybl
:
approval-mozilla-esr10+
|
Details | Diff | Splinter Review |
I was trying to understand how new nodelist bindings work and noticed that release may happen in ::finalize. Is it possible that it may cause similar problems as what bug 712448 caused?
Would it be possible to assert in NS_RELEASE that we're not inside a GC? Ben says that he is using NS_RELEASE from a finalizer in a safe way, so maybe we could have some sort of whitelisted version that doesn't do the assert.
Assignee | ||
Comment 3•13 years ago
|
||
Bug 736825 has a testcase.
Assignee: nobody → peterv
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 4•13 years ago
|
||
Attachment #607161 -
Flags: review?(jst)
Updated•13 years ago
|
Attachment #607161 -
Flags: review?(jst) → review+
Comment 5•13 years ago
|
||
Will we need this for branches? What are the security implications here?
Comment 6•13 years ago
|
||
Yes, we'll need this for branches, the code exists all the way back to esr, but I don't know for sure if it's enabled there or not. AFAICT, the implications here is that we could potentially run arbitrary code while we're in GC, and that's sg:critical kinda bad. Peter, please correct me if I'm wrong here...
Peter, is there anything stopping this from landing?
Assignee | ||
Comment 8•13 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #7) > Peter, is there anything stopping this from landing? Nope, thanks for reminding me. https://hg.mozilla.org/integration/mozilla-inbound/rev/917f69eb26ef
Assignee | ||
Comment 9•13 years ago
|
||
Comment on attachment 607161 [details] [diff] [review] v1 [Approval Request Comment] Regression caused by (bug #): bug 648801 User impact if declined: security bug (could cause scripts running during the GC) Testing completed (on m-c, etc.): landed on inbound, has a crashtest Risk to taking this patch (and alternatives if risky): low risk, we're just delaying destruction of object till after the GC is done String changes made by this patch: None
Attachment #607161 -
Flags: approval-mozilla-beta?
Attachment #607161 -
Flags: approval-mozilla-aurora?
Comment 10•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/917f69eb26ef
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
status-firefox15:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Comment 11•13 years ago
|
||
Comment on attachment 607161 [details] [diff] [review] v1 [Triage Comment] This appears to be a low risk sg:high/crit fix early in the cycle. Approved for Beta 13 and Aurora 14.
Attachment #607161 -
Flags: approval-mozilla-beta?
Attachment #607161 -
Flags: approval-mozilla-beta+
Attachment #607161 -
Flags: approval-mozilla-aurora?
Attachment #607161 -
Flags: approval-mozilla-aurora+
Comment 12•13 years ago
|
||
Verified in debug trunk build for 4/30. When I run the testcase from Bug 736825 on the 4/28 debug build, I get the assert and then this crash: bp-38118b2a-0b0d-4932-938d-833442120430 On 4/30 debug trunk, I get no assert and no crash.
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 13•13 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/14d93414e972 https://hg.mozilla.org/releases/mozilla-aurora/rev/cfaabe5f04be
status-firefox13:
--- → fixed
status-firefox14:
--- → fixed
Updated•13 years ago
|
status-firefox-esr10:
--- → affected
tracking-firefox-esr10:
--- → 13+
Updated•13 years ago
|
Keywords: sec-critical
Whiteboard: [sg:critical]
Comment 14•12 years ago
|
||
Is this going in for ESR?
Updated•12 years ago
|
Whiteboard: [sg:critical] → [sg:critical][advisory-tracking+]
Assignee | ||
Comment 15•12 years ago
|
||
Comment on attachment 607161 [details] [diff] [review] v1 [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec:crit User impact if declined: crashes Fix Landed on Version: 13 Risk to taking this patch (and alternatives if risky): low risk, we're just delaying destruction of object till after the GC is done String or UUID changes made by this patch: none
Attachment #607161 -
Flags: approval-mozilla-esr10?
Updated•12 years ago
|
Attachment #607161 -
Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Assignee | ||
Comment 16•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-esr10/rev/ac1b28490529
Comment 17•12 years ago
|
||
Verified fixed using the testcase in bug 736825 with: * Firefox 10.0.6esrpre 2012-07-11 * Firefox 15.0a2 2012-07-11 * Firefox 14.0b10 * Firefox 13.0.1
Updated•12 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•