Closed Bug 758471 Opened 12 years ago Closed 12 years ago

Don't sweep native interfaces during compartment GC

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15
Tracking Status
firefox14 --- fixed
firefox15 --- fixed
firefox-esr10 --- fixed

People

(Reporter: billm, Assigned: billm)

Details

(Keywords: sec-critical, Whiteboard: [advisory-tracking+][qa?])

Attachments

(1 file)

Attached patch patchSplinter Review
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+
https://hg.mozilla.org/mozilla-central/rev/a172444cca11
Status: NEW → RESOLVED
Closed: 12 years ago
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?
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
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?
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.