GC: Recalculation of GC fullness in EndSweepPhase broken

RESOLVED FIXED in Firefox 20

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

({csectype-uaf, regression, sec-critical})

Trunk
mozilla21
csectype-uaf, regression, sec-critical
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox18 unaffected, firefox19 unaffected, firefox20+ fixed, firefox21+ fixed, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [adv-main20-])

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
Bug 790338 re-broke calculation of whether the GC is full vs. compartmental in EndSweepPhase (I can't find the original defect right now).
That seems like it could do bad things to how we schedule GC and CC.
Blocks: 790338
status-firefox20: --- → affected
tracking-firefox20: --- → ?
(Assignee)

Comment 2

5 years ago
Created attachment 697909 [details] [diff] [review]
Proposed fix

Fix the issue and add regression tests.
Group: core-security
Ah, yes, this problem. Bug 801957 is the one you were thinking of. Thanks for writing regression tests, as this is clearly a touchy area of the code. :)
Keywords: csec-uaf, regression, sec-critical
status-firefox18: --- → unaffected
status-firefox19: --- → unaffected
Assignee: general → jcoppeard

Updated

5 years ago
tracking-firefox20: ? → +
(Assignee)

Comment 4

5 years ago
Created attachment 698758 [details] [diff] [review]
Proposed fix
Attachment #697909 - Attachment is obsolete: true
Attachment #698758 - Flags: review?(wmccloskey)
Comment on attachment 698758 [details] [diff] [review]
Proposed fix

Review of attachment 698758 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/TestingFunctions.cpp
@@ +1072,5 @@
>  bool
>  js::DefineTestingFunctions(JSContext *cx, HandleObject obj)
>  {
> +#ifdef JS_GC_ZEAL
> +    JS_SetFinalizeCallback(cx->runtime, TestFinalizeCallback);

This code can execute from the browser, in which case it will conflict with the browser's finalize callback.

I think this might work better as a JSAPI test. Can you try that instead?

::: js/src/jsgc.cpp
@@ +4327,5 @@
>      AssertCanGC();
>      Collect(rt, true, SliceBudget::Unlimited, gckind, reason);
>  }
>  
> +static bool CompartmentsSelected(JSRuntime *rt)

Need a newline between bool and the function name.
Attachment #698758 - Flags: review?(wmccloskey)
(Assignee)

Updated

5 years ago
Blocks: 826673
(Assignee)

Comment 6

5 years ago
Created attachment 699312 [details] [diff] [review]
Fix with updated test code
Attachment #698758 - Attachment is obsolete: true
Attachment #699312 - Flags: review?(wmccloskey)
Comment on attachment 699312 [details] [diff] [review]
Fix with updated test code

Review of attachment 699312 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi-tests/testGCFinalizeCallback.cpp
@@ +16,5 @@
> +
> +static void
> +FinalizeCallback(JSFreeOp *fop, JSFinalizeStatus status, JSBool isCompartmentGC)
> +{
> +	if (FinalizeCalls < BufferSize) {

It looks like some tab characters snuck in here. Please replace with spaces.

@@ +97,5 @@
> +    CHECK(checkFinalizeIsCompartmentGC(true));
> +
> +#ifdef JS_GC_ZEAL
> +
> +	/* Full GC with reset due to new compartment, becoming compartment GC. */

Another tab here.
Attachment #699312 - Flags: review?(wmccloskey) → review+
status-firefox21: --- → affected
tracking-firefox21: --- → +
(Assignee)

Comment 8

5 years ago
Created attachment 701064 [details] [diff] [review]
Fix for test failures in opt builds
Attachment #699312 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
Created attachment 701106 [details] [diff] [review]
Also fix whitespace issues in patch
Attachment #701064 - Attachment is obsolete: true
Comment on attachment 701106 [details] [diff] [review]
Also fix whitespace issues in patch

Please take the printf out of the test.
Attachment #701106 - Flags: review+
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/31f4f50447e8
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/mozilla-central/rev/31f4f50447e8

Note wrong bug number in commit message!
You should nominate the patch for Aurora approval, if you think it is ready to land there.
status-firefox21: affected → fixed
Flags: in-testsuite+
Target Milestone: --- → mozilla21
(Assignee)

Comment 14

5 years ago
Comment on attachment 701106 [details] [diff] [review]
Also fix whitespace issues in patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 790338
User impact if declined: Potential to cause crashes, hence security issue
Testing completed (on m-c, etc.): This patch has been on m-c for about a week
Risk to taking this patch (and alternatives if risky): low risk, restores original behaviour before regression
String or UUID changes made by this patch: none
Attachment #701106 - Flags: approval-mozilla-aurora?
Comment on attachment 701106 [details] [diff] [review]
Also fix whitespace issues in patch

Approving on aurora as it has a low risk patch restoring original behavior, which led to this regression. Tested on m-c for a week.
Attachment #701106 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/07e1d48fd8f7

(With bug # fixed)
status-firefox20: affected → fixed
status-b2g18: --- → unaffected
status-firefox-esr17: --- → unaffected
Whiteboard: [adv-main20-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.