If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Assertion failure: rt->gc.atomMarking.atomIsMarked(compartment->zone(), cell), at js/src/jscntxtinlines.h:92 with saveStack

RESOLVED FIXED in Firefox 54

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: decoder, Assigned: bhackett)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla54
x86_64
Linux
assertion, jsbugmon, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 unaffected, firefox53 unaffected, firefox54 fixed)

Details

(Whiteboard: [jsbugmon:update])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

8 months ago
The following testcase crashes on mozilla-central revision 9c06e744b1be (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off min.js):

low = high = newGlobal({})
high.low = low
high.eval("function a() { return saveStack(1, low) }")
set = eval("high.a()")
serialize(set)


Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x0000000000535be8 in js::CompartmentChecker::checkAtom (cell=<optimized out>, this=<optimized out>) at js/src/jscntxtinlines.h:92
#0  0x0000000000535be8 in js::CompartmentChecker::checkAtom (cell=<optimized out>, this=<optimized out>) at js/src/jscntxtinlines.h:92
#1  js::CompartmentChecker::check (symbol=<optimized out>, this=<optimized out>) at js/src/jscntxtinlines.h:107
#2  js::CompartmentChecker::check (this=0x7fffffffcdd0, v=...) at js/src/jscntxtinlines.h:116
#3  0x0000000000936abc in js::CompartmentChecker::check<JS::Value> (handle=..., this=0x7fffffffcdd0) at js/src/jscntxtinlines.h:83
#4  js::assertSameCompartment<JS::Handle<JS::Value> > (cx=0x7ffff6946000, t1=...) at js/src/jscntxtinlines.h:189
#5  0x0000000000bcfd5c in JSStructuredCloneWriter::startWrite (this=this@entry=0x7fffffffd1a0, v=...) at js/src/vm/StructuredClone.cpp:1378
#6  0x0000000000bd0e3d in JSStructuredCloneWriter::traverseSavedFrame (this=this@entry=0x7fffffffd1a0, obj=..., obj@entry=...) at js/src/vm/StructuredClone.cpp:1351
#7  0x0000000000bd035f in JSStructuredCloneWriter::startWrite (this=this@entry=0x7fffffffd1a0, v=...) at js/src/vm/StructuredClone.cpp:1448
#8  0x0000000000bd263d in JSStructuredCloneWriter::write (v=..., this=0x7fffffffd1a0) at js/src/vm/StructuredClone.cpp:1616
#9  WriteStructuredClone (cx=cx@entry=0x7ffff6946000, v=..., bufp=bufp@entry=0x7fffffffd660, scope=scope@entry=JS::StructuredCloneScope::SameProcessSameThread, cloneDataPolicy=..., cloneDataPolicy@entry=..., cb=cb@entry=0x0, cbClosure=0x0, transferable=...) at js/src/vm/StructuredClone.cpp:541
#10 0x0000000000bd275c in JS_WriteStructuredClone (cx=cx@entry=0x7ffff6946000, value=..., bufp=bufp@entry=0x7fffffffd660, scope=JS::StructuredCloneScope::SameProcessSameThread, cloneDataPolicy=..., cloneDataPolicy@entry=..., optionalCallbacks=optionalCallbacks@entry=0x0, closure=0x0, transferable=...) at js/src/vm/StructuredClone.cpp:2497
#11 0x0000000000bd27db in JSAutoStructuredCloneBuffer::write (this=this@entry=0x7fffffffd658, cx=cx@entry=0x7ffff6946000, value=..., transferable=..., cloneDataPolicy=..., cloneDataPolicy@entry=..., optionalCallbacks=optionalCallbacks@entry=0x0, closure=0x0) at js/src/vm/StructuredClone.cpp:2662
#12 0x000000000086a961 in Serialize (cx=cx@entry=0x7ffff6946000, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/TestingFunctions.cpp:2449
#13 0x0000000000535d09 in js::CallJSNative (cx=cx@entry=0x7ffff6946000, native=0x86a5f0 <Serialize(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:262
[...]
#26 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:7975
rax	0x0	0
rbx	0x7ffff06aa128	140737226907944
rcx	0x7ffff6c28a2d	140737333332525
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffcdb0	140737488342448
rsp	0x7fffffffcda0	140737488342432
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0x7fffffffcdd0	140737488342480
r13	0x7fffffffcf30	140737488342832
r14	0x7fffffffcf10	140737488342800
r15	0x7fffffffcf70	140737488342896
rip	0x535be8 <js::CompartmentChecker::check(JS::Value const&)+376>
=> 0x535be8 <js::CompartmentChecker::check(JS::Value const&)+376>:	movl   $0x0,0x0
   0x535bf3 <js::CompartmentChecker::check(JS::Value const&)+387>:	ud2    


Another GC assert, but I'm not sure if this one is shell-only or not (due to saveStack being involved). Marking s-s anyway until triaged.

Updated

8 months ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]

Comment 1

8 months ago
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/7311c06a7271
user:        Brian Hackett
date:        Mon Jan 30 06:31:47 2017 -0700
summary:     Bug 1324002 - Mark atoms separately in each zone, r=jonco,mccr8,peterv.

This iteration took 0.437 seconds to run.
Brian, is bug 1324002 a likely regressor?
Blocks: 1324002
Flags: needinfo?(bhackett1024)
Keywords: sec-high
Keywords: csectype-uaf
[Tracking Requested - why for this release]:
status-firefox53: --- → unaffected
tracking-firefox54: --- → ?
(Assignee)

Comment 4

8 months ago
Created attachment 8833041 [details] [diff] [review]
patch

This is a false positive, JSStructuredCloneWriter::startWrite always asserts the value is in the same compartment as the context, but for non-objects it just writes out the value without storing it anywhere.  This patch only checks when objects are being written out, though an alternative I guess would be to mark the atoms in the context when the caller is inspecting an unwrapped saved frame.
Assignee: nobody → bhackett1024
Flags: needinfo?(bhackett1024)
Attachment #8833041 - Flags: review?(jcoppeard)
Tracking 54+ for this sec high issue.
tracking-firefox54: ? → +

Comment 6

8 months ago
Comment on attachment 8833041 [details] [diff] [review]
patch

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

I think I'd like to keep the assertion as it is since it seems like an invariant that we should enforce.  Can you try your alternative approach?

If you really want to do this you could ask Steve what he thinks as he knows this code better then me.
Attachment #8833041 - Flags: review?(jcoppeard)
(Assignee)

Comment 7

8 months ago
Created attachment 8833276 [details] [diff] [review]
patch

Sure, here's the alternative patch.
Attachment #8833041 - Attachment is obsolete: true
Attachment #8833276 - Flags: review?(jcoppeard)

Comment 8

8 months ago
Comment on attachment 8833276 [details] [diff] [review]
patch

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

Thanks.
Attachment #8833276 - Flags: review?(jcoppeard) → review+
Unhiding per comment 4.
Group: javascript-core-security
Keywords: csectype-uaf, sec-high
tracking-firefox54: + → ---

Comment 10

8 months ago
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5475dc2e1f4
Fix bogus assert by marking atoms when serializing cross-zone saved frames, r=jonco.

Comment 11

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e5475dc2e1f4
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
status-firefox52: --- → unaffected
You need to log in before you can comment on or make changes to this bug.