The default bug view has changed. See this FAQ.

Don't sweep native interfaces during compartment GC

RESOLVED FIXED in Firefox 14

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

({sec-critical})

unspecified
mozilla15
sec-critical
Points:
---

Firefox Tracking Flags

(firefox14 fixed, firefox15 fixed, firefox-esr10 fixed)

Details

(Whiteboard: [advisory-tracking+][qa?])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 627060 [details] [diff] [review]
patch

This is the one aspect of bug 715761 that was correctness-related. Right now, we mark native interfaces when we mark XPCWrappedNativeProto and XPCWrappedNative. We won't mark an XPCWrappedNativeProto or XPCWrappedNative if it's not in a compartment that's being collected. However, we'll still sweep their native interfaces. That's bad.

This patch skips the sweeping if we aren't collecting all compartments. This is kind of an imperfect solution, because most GCs now are compartment GCs. However, it's hard for me to think of a situation where we'll accumulate tons of native interfaces in a short period of time. So I think it's okay that they have to wait until a full GC to be swept.

Eventually it would be nice if the maps for native interfaces were per-compartment rather than per-runtime. Then we could sweep only the ones that were marked. But that's a bigger change than I have time for right now.

The motivation for this is that I saw a stack pop on tryserver today with native interface sweeping at the top. So it seems like we might be hitting this. Recent patches have made it a lot more likely.
Attachment #627060 - Flags: review?(bobbyholley+bmo)
Comment on attachment 627060 [details] [diff] [review]
patch

>+    if (arg == UNMARK_ONLY)
>+        return JS_DHASH_NEXT;
>+

Please add a big comment here explaining why this is necessary (especially the part how we can't just skip marking them because they get marked when marking WNs and protos, which we _do_ want to do for compartmental GC).
Attachment #627060 - Flags: review?(bobbyholley+bmo) → review+
(Assignee)

Comment 2

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a172444cca11
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/a172444cca11
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox15: --- → fixed
Resolution: --- → FIXED
(In reply to Bill McCloskey (:billm) from comment #0)
> This is the one aspect of bug 715761 that was correctness-related.

"Correctness" in GC meaning a potential security problem? Does this affect ESR or Beta (fx14) and need to be fixed on those branches?
(Assignee)

Comment 5

5 years ago
This goes back to when compartment GC was introduced in Firefox 4. When I noticed the problem in bug 715761, it seemed like a theoretical problem. Lately we've been doing more compartment GCs and I started noticing crashes on tryserver that seemed like they might be related. The patch in this bug seemed to make them go away.
Keywords: sec-critical
(Assignee)

Comment 6

5 years ago
Comment on attachment 627060 [details] [diff] [review]
patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Low likelihood of crashes in xpconnect, possible exploitation.
Fix Landed on Version: 15
Risk to taking this patch (and alternatives if risky): Patch will require some rebasing that may introduce bugs. Otherwise low risk, although there's a theoretical possibility that we'll take longer to free some memory.
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 605662
User impact if declined: Low likelihood of crashes in xpconnect, possible exploitation.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Low risk, although there's a theoretical possibility that we'll take longer to free some memory.
String or UUID changes made by this patch: None
Attachment #627060 - Flags: approval-mozilla-esr10?
Attachment #627060 - Flags: approval-mozilla-beta?
Attachment #627060 - Flags: approval-mozilla-aurora?
Comment on attachment 627060 [details] [diff] [review]
patch

approving for Beta and ESR but if this fix landed on 15 then we don't need it for Aurora.
Attachment #627060 - Flags: approval-mozilla-esr10?
Attachment #627060 - Flags: approval-mozilla-esr10+
Attachment #627060 - Flags: approval-mozilla-beta?
Attachment #627060 - Flags: approval-mozilla-beta+
Attachment #627060 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/a272c966d0ca
https://hg.mozilla.org/releases/mozilla-esr10/rev/814e0e9a8618
status-firefox-esr10: --- → fixed
status-firefox14: --- → fixed
Whiteboard: [advisory-tracking+]
Is there something QA can do to verify this fix?
Whiteboard: [advisory-tracking+] → [advisory-tracking+][qa?]
Group: core-security
You need to log in before you can comment on or make changes to this bug.