Crash [@ js::gc::TenuredCell::arena]

VERIFIED FIXED in Firefox -esr45

Status

()

defect
--
critical
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: gkw, Assigned: jonco)

Tracking

(Blocks 2 bugs, 5 keywords)

Trunk
mozilla52
x86_64
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox49 wontfix, firefox-esr4550+ fixed, firefox50+ verified, firefox51+ verified, firefox52+ verified)

Details

(Whiteboard: [jsbugmon:update][adv-main50+][adv-esr45.5+], crash signature)

Attachments

(4 attachments)

Reporter

Description

3 years ago
The following testcase crashes on mozilla-central revision 938ce16be25f (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --ion-offthread-compile=off --no-baseline --no-ion):

// jsfunfuzz-generated
startgc(1, 'shrinking');
offThreadCompileScript("");
// Adapted from randomly chosen test: js/src/jit-test/tests/parser/bug-1263355-13.js
gczeal(9);
newGlobal();


Backtrace:

0   js-dbg-64-dm-clang-darwin-938ce16be25f	0x0000000107906c84 js::gc::TenuredCell::arena() const + 20 (HeapAPI.h:338)
1   js-dbg-64-dm-clang-darwin-938ce16be25f	0x00000001079012b3 js::GlobalHelperThreadState::trace(JSTracer*) + 611 (Heap.h:1270)
2   js-dbg-64-dm-clang-darwin-938ce16be25f	0x0000000107c0eeb8 js::gc::GCRuntime::traceRuntimeCommon(JSTracer*, js::gc::GCRuntime::TraceOrMarkRuntime, js::AutoLockForExclusiveAccess&) + 888 (RootMarking.cpp:386)
3   js-dbg-64-dm-clang-darwin-938ce16be25f	0x0000000107c0eadc js::gc::GCRuntime::traceRuntimeForMajorGC(JSTracer*, js::AutoLockForExclusiveAccess&) + 220 (Statistics.h:457)
4   js-dbg-64-dm-clang-darwin-938ce16be25f	0x0000000107700c60 js::gc::GCRuntime::updatePointersToRelocatedCells(JS::Zone*, js::AutoLockForExclusiveAccess&) + 320 (Statistics.h:435)
/snip

For detailed crash information, see attachment.
Reporter

Comment 2

3 years ago
GC is on the stack, this should be s-s as a start.
Group: javascript-core-security
Reporter

Comment 3

3 years ago
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/488c4ea38e16
parent:      312970:b294f305f8d1
user:        Jon Coppeard
date:        Wed Sep 07 11:30:32 2016 +0100
summary:     Bug 1292590 - Trace script pointers in off thread compilation tasks r=terrence a=abillings

Jon, is bug 1292590 a likely regressor?
Blocks: 1292590
Flags: needinfo?(jcoppeard)
Assignee

Comment 4

3 years ago
The helper thread trace methods currently get the runtime via a GC thing pointer, but the thing could be in the process of being moved.

We could use MaybeForwarded, but in fact it's easier to get the runtime from somewhere else instead.
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
Attachment #8789732 - Flags: review?(terrence)
Comment on attachment 8789732 [details] [diff] [review]
bug1301496-helper-thread-tracing

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

::: js/src/testcase.js
@@ +1,5 @@
> +startgc(1, 'shrinking');
> +offThreadCompileScript("");
> +// Adapted from randomly chosen test: js/src/jit-test/tests/parser/bug-1263355-13.js
> +gczeal(9);
> +newGlobal();

You probably don't want to check in this copy too.
Attachment #8789732 - Flags: review?(terrence) → review+
Assignee

Updated

3 years ago
Duplicate of this bug: 1302463
Assignee

Comment 7

3 years ago
Comment on attachment 8789732 [details] [diff] [review]
bug1301496-helper-thread-tracing

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Very difficult.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No.

Which older supported branches are affected by this flaw? 51 is affected.

If not all supported branches, which bug introduced the flaw? Bug 1292590.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Should be trivial.

How likely is this patch to cause regressions; how much testing does it need? Unlikely.
Attachment #8789732 - Flags: sec-approval?
Comment on attachment 8789732 [details] [diff] [review]
bug1301496-helper-thread-tracing

We need this to land quickly due to its generic signature possibly hiding other GC related failures in our automation. Also, the crash volume I'm seeing in FuzzManager is huge (> 8000 crashes associated to this bug right now).

Given that Al is on PTO, Dan on a conference and the risk involved here being really low, I'm giving sec-approval to land on mozilla-central. Please also prepare a patch to uplift to Aurora as soon as possible.
Comment on attachment 8789732 [details] [diff] [review]
bug1301496-helper-thread-tracing

[Triage Comment]

sec-approval=dveditz
a=dveditz for aurora
Attachment #8789732 - Flags: sec-approval?
Attachment #8789732 - Flags: sec-approval+
Attachment #8789732 - Flags: approval-mozilla-aurora+
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/41b22909f10a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee

Comment 13

3 years ago
Approval Request Comment
[Feature/regressing bug #]: Bug 1292590.
[User impact if declined]: Possible crash / security vulnerability.
[Describe test coverage new/current, TreeHerder]: On m-c since September 23rd.
[Risks and why]: Low.
[String/UUID change made/needed]: None.
Attachment #8795250 - Flags: review+
Attachment #8795250 - Flags: approval-mozilla-beta?
Assignee

Comment 14

3 years ago
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high bug.
User impact if declined: Possible crash / security vulnerability.
Fix Landed on Version: 52
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8795251 - Flags: review+
Attachment #8795251 - Flags: approval-mozilla-esr45?
Comment on attachment 8795250 [details] [diff] [review]
bug1301496-beta

Sec-high, Beta50+
Attachment #8795250 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8795251 [details] [diff] [review]
bug1301496-esr45

Sec-high, ESR45.5+
Attachment #8795251 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Group: javascript-core-security → core-security-release

Comment 18

3 years ago
JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx50
JSBugMon: This bug has been automatically verified fixed on Fx51
Depends on: 1308747
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main50+][adv-esr45.5+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.