Crash [@ js::jit::AssertValidObjectPtr] or Assertion failure: obj->compartment() == cx->compartment(), at jit/VMFunctions.cpp

VERIFIED FIXED in Firefox 33



JavaScript Engine: JIT
4 years ago
3 years ago


(Reporter: gkw, Assigned: lth)


(Blocks: 1 bug, 6 keywords)

Mac OS X
assertion, crash, csectype-wildptr, regression, sec-critical, testcase
Dependency tree / graph

Firefox Tracking Flags

(firefox32 disabled, firefox33 verified, firefox-esr24 wontfix, firefox-esr31 disabled)


(Whiteboard: [jsbugmon:update], crash signature)


(2 attachments, 1 obsolete attachment)



4 years ago
Created attachment 8449104 [details]
Crash and assert stacks

x = []
x[8] = ((function() {})());
for each(let a in [0, 0]) {
    x.reducePar(function() {
        return [0];

asserts js debug shell on m-c changeset b6408c32a170 with --ion-eager --ion-offthread-compile=off at Assertion failure: obj->compartment() == cx->compartment(), at jit/VMFunctions.cpp but also sometimes crashes at js::jit::AssertValidObjectPtr

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --with-ccache --enable-threadsafe <other NSPR options>

=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20140609215111" and the hash "a98e6f80344c".
The "bad" changeset has the timestamp "20140609220510" and the hash "573458d10426".

Likely regression window:

(initially setting s-s and sec-high as a compartment assert doesn't sound good, but please feel free to change this as necessary.)

Lars, is bug 933313 a likely regressor?
Flags: needinfo?(lhansen)

Comment 1

4 years ago
 - Reproduces on first try on Linux64 debug build (--enable-debug --disable-optimize).
 - Also reproduces (slightly different crash but clearly same cause) with --thread-count=1.

Immediate observations:
 - The problem is that obj references some other objects that contain fields that are filled with
   the JS_POISONED_FORKJOIN_CHUNK poison (, suggesting some uninitialized fields.
 - The shape, and the shape's base shape, all contain a number of poisoned fields.
 - obj->compartment() == obj->lastProperty()->base()->compartment(), and that value is poisoned.
 - Actually obj itself has poisioned fields - it looks uninitialized.

Conditions for crashing:
 - The for-each statement must create a binding: if it is "for each(let a in ...)" 
   or "for each(var a in ...)" or "for each(a in ...)" without an existing global
   binding for a then we crash.  If there is an existing binding for a then we do not crash.
 - The array we're iterating over must be sparse and relatively large; 9 elements seems to
   be the smallest
 - The type in the array must be some unknown type
 - The kernel must return an object value

Room for variation:
 - The object being iterated over can be a plain object, it need not be an array
 - The object returned from the kernel could be any object value, eg {}

=> With one thread, at the time of the crash we are out of the parallel section and we've
   evacuated no objects.

=> Yet clearly, since the kernel returns an object value, there should have been at least one
   object evacuated.

The bug is almost certainly that when running the evacuation from PJS, the return value from the parallel section is not considered a root.  And indeed, that is a bug in Array.js in ArrayReducePar: the array "subreductions" should be passed to ForkJoin as an assignment target, but it is not.

I'll keep this as a security issue for now, since in a release build there would likely be a wild pointer into the discarded PJS nursery from the subreductions array.  It's possible that some sort of heap spray attack could be used, though not likely, given that many object types cannot be allocated in the nursery.
Assignee: nobody → lhansen
Flags: needinfo?(lhansen)

Comment 2

4 years ago
Created attachment 8449226 [details] [diff] [review]
Patch, with test case

Pass the intermediate array as a root to ForkJoin().
Attachment #8449226 - Flags: review?(shu)

Comment 3

4 years ago
Gary, in response to your question, this bug was indeed introduced by bug 933313, which created the requirement that the result array has to be passed as an argument to the ForkJoin() primitive.


4 years ago
Duplicate of this bug: 1033080

Comment 5

4 years ago
Created attachment 8449261 [details] [diff] [review]
Patch, with two test cases

Added another test case from bug 1033080.
Attachment #8449226 - Attachment is obsolete: true
Attachment #8449226 - Flags: review?(shu)
Attachment #8449261 - Flags: review?(shu)

Comment 6

4 years ago
Comment on attachment 8449261 [details] [diff] [review]
Patch, with two test cases

Review of attachment 8449261 [details] [diff] [review]:

Good catch.
Attachment #8449261 - Flags: review?(shu) → review+
Last Resolved: 4 years ago
status-firefox33: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
status-firefox-esr24: --- → wontfix
status-firefox33: fixed → verified
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security
status-firefox32: --- → disabled
Keywords: csectype-wildptr, sec-critical
I assume that this is disabled in ESR31?
status-firefox-esr31: --- → disabled
Group: core-security
You need to log in before you can comment on or make changes to this bug.