Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Investigate if ListBase<LC>::finalize should use deferred release

VERIFIED FIXED in Firefox 13

Status

()

Core
XPConnect
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: smaug, Assigned: peterv)

Tracking

({sec-critical})

12 Branch
mozilla15
x86_64
Linux
sec-critical
Points:
---

Firefox Tracking Flags

(firefox13 verified, firefox14 verified, firefox15 verified, firefox-esr1013+ verified)

Details

(Whiteboard: [sg:critical][advisory-tracking+])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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)

Updated

5 years ago
Duplicate of this bug: 736825
(Assignee)

Comment 3

5 years ago
Bug 736825 has a testcase.
Assignee: nobody → peterv
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 4

5 years ago
Created attachment 607161 [details] [diff] [review]
v1
Attachment #607161 - Flags: review?(jst)

Updated

5 years ago
Attachment #607161 - Flags: review?(jst) → review+
Will we need this for branches?

What are the security implications here?
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

5 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

5 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?
http://hg.mozilla.org/mozilla-central/rev/917f69eb26ef
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox15: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
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+
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

5 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
status-firefox-esr10: --- → affected
tracking-firefox-esr10: --- → 13+
Keywords: sec-critical
Whiteboard: [sg:critical]
Is this going in for ESR?
Whiteboard: [sg:critical] → [sg:critical][advisory-tracking+]
(Assignee)

Comment 15

5 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

5 years ago
Attachment #607161 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
(Assignee)

Comment 16

5 years ago
https://hg.mozilla.org/releases/mozilla-esr10/rev/ac1b28490529
status-firefox-esr10: affected → fixed
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
status-firefox-esr10: fixed → verified
status-firefox13: fixed → wontfix
status-firefox14: fixed → verified
status-firefox15: fixed → verified
status-firefox13: wontfix → verified
Group: core-security
You need to log in before you can comment on or make changes to this bug.