Last Comment Bug 723465 - Investigate if ListBase<LC>::finalize should use deferred release
: Investigate if ListBase<LC>::finalize should use deferred release
Status: VERIFIED FIXED
[sg:critical][advisory-tracking+]
: sec-critical
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: 12 Branch
: x86_64 Linux
: -- normal (vote)
: mozilla15
Assigned To: Peter Van der Beken [:peterv]
:
Mentors:
: 736825 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-02 04:23 PST by Olli Pettay [:smaug] (high review load, please consider other reviewers)
Modified: 2012-07-12 08:51 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified
verified
13+
verified


Attachments
v1 (3.52 KB, patch)
2012-03-19 07:38 PDT, Peter Van der Beken [:peterv]
jst: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr10+
Details | Diff | Review

Description Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-02-02 04:23:53 PST
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?
Comment 1 Bill McCloskey (:billm) 2012-02-14 11:17:03 PST
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.
Comment 2 Peter Van der Beken [:peterv] 2012-03-18 06:50:20 PDT
*** Bug 736825 has been marked as a duplicate of this bug. ***
Comment 3 Peter Van der Beken [:peterv] 2012-03-18 06:51:01 PDT
Bug 736825 has a testcase.
Comment 4 Peter Van der Beken [:peterv] 2012-03-19 07:38:14 PDT
Created attachment 607161 [details] [diff] [review]
v1
Comment 5 Al Billings [:abillings] 2012-04-03 16:35:50 PDT
Will we need this for branches?

What are the security implications here?
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2012-04-03 16:52:26 PDT
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...
Comment 7 Bill McCloskey (:billm) 2012-04-26 14:03:46 PDT
Peter, is there anything stopping this from landing?
Comment 8 Peter Van der Beken [:peterv] 2012-04-27 06:07:45 PDT
(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
Comment 9 Peter Van der Beken [:peterv] 2012-04-27 06:13:29 PDT
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
Comment 10 Ed Morley [:emorley] 2012-04-29 14:00:13 PDT
http://hg.mozilla.org/mozilla-central/rev/917f69eb26ef
Comment 11 Alex Keybl [:akeybl] 2012-04-30 10:01:52 PDT
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.
Comment 12 Al Billings [:abillings] 2012-04-30 13:10:33 PDT
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.
Comment 14 Al Billings [:abillings] 2012-05-18 17:05:30 PDT
Is this going in for ESR?
Comment 15 Peter Van der Beken [:peterv] 2012-05-30 12:12:47 PDT
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
Comment 16 Peter Van der Beken [:peterv] 2012-05-30 13:11:42 PDT
https://hg.mozilla.org/releases/mozilla-esr10/rev/ac1b28490529
Comment 17 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-07-11 12:11:34 PDT
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

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