Closed Bug 639343 Opened 9 years ago Closed 9 years ago

TM: Crash [@ js::DefaultValue] or "Assertion failure: obj,"

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
Tracking Status
blocking2.0 --- Macaw+
status2.0 --- .1-fixed
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: gkw, Assigned: Waldo)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [sg:critical?][ccbr][fixed-in-tracemonkey])

Crash Data

Attachments

(2 files, 1 obsolete file)

Attached file stacks
(Array.prototype.__proto__)[0] = /a/
function f() {
  eval("\
    (function(){\
      for(t in [,0,0,0,0,0,0,0,0,0]) {\
        a = new Int8Array;\
        print(a[0])\
      }\
    })\
  ")()
}
f()

crashes js opt shell on TM changeset ae418087b4da with -j at js::DefaultValue and asserts js debug shell with -j at Assertion failure: obj

Seems to be a null deref but locking s-s just-in-case.

This was found using a combination of jsfunfuzz and jandem's method fuzzer.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   51690:c497469955fa
user:        Vladimir Vukicevic
date:        Fri Aug 27 12:06:34 2010 -0400
summary:     b=590672; treat ArrayBuffer() and SomeArrayType() as (0); r=shaver
    /* for out-of-range, do the same thing that the interpreter does, which is return undefined */
    if ((jsuint) idx >= tarray->length) {
        CHECK_STATUS_A(guard(false,
                             w.ltui(idx_ins, w.ldiConstTypedArrayLength(priv_ins)),
                             BRANCH_EXIT,
                             /* abortIfAlwaysExits = */true));
        v_ins = w.immiUndefined();
        return ARECORD_CONTINUE;
    }

J'accuse!
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Looks like methodjit might be affected as well, if my guesses at the meaning of this from GetElementIC::attachTypedArray are correct (although very brief testing isn't demonstrating problems, so who knows):

    outOfBounds.linkTo(masm.label(), &masm);
    masm.loadValueAsComponents(UndefinedValue(), typeReg, objReg);

So.  Just to be sure, should it be the case that |new Int8Array()[0]| looks up the prototype chain at |Int8Array.prototype| and at |Object.prototype|?  I assume yes, but I could imagine some sort of index-specific algorithm just returning |undefined| here to short-circuit.  The typed array spec seems to just say there's an index getter, but I don't believe it says anything about what behavior is for out-of-range indexes.  (Or is that implicit in the index-getterness in some way I don't know?)
Attached patch Patch for tracing JIT, with test (obsolete) — Splinter Review
For some reason my build doesn't have typed array ICs enabled, so that's why I can't reproduce the typed array failure, if there is one.  I'll get to that soon, or perhaps someone more in-tune with ICs should, but this patch at least fixes the tracer in the meantime.  Probably everything should be fixed at once, tho, since I suspect just this would cause failures from the IC stuff not being updated, hence no r? yet.
Bug 639412 may be related.
(In reply to comment #3)
> Created attachment 517319 [details] [diff] [review]
> Patch for tracing JIT, with test

After a quick test, this patch fixes the testcase in this bug as well as the testcases in bug 639412. I'm not sure if other issues will pop up or not...
fwiw, the test in comment #0 causes an LIR type error in 64-bit debug shell with -j:

Assertion failure: LIR type error (start of writer pipeline): arg 1 of 'orq' is 'immi' which has type int (expected quad): 0

Sounds bad enough to warrant at least .x+ in blocking2.0..
blocking2.0: --- → ?
Duplicate of this bug: 639412
Attachment #517319 - Flags: review-
blocking2.0: ? → ---
Please renom for .x with a reviewed patch.
Um, Andreas, why'd you r- that if I hadn't even requested review on it?  (Not disagreeing with the 2.0-, to be sure.)
Guessing sg:critical given the LIR error. Please clear if that's wrong.
Whiteboard: [ccbr] → [sg:critical?][ccbr]
Hey waldo, just wanted to make sure that you know that I don't think its a good fix. An r- was quicker than typing that up, I was sitting in the triage meeting trying to keep up. Sorry. Happy to discuss a fix.
With the patch on the 32-bit buildmonkey-right system:

~/jwalden/tracemonkey/js/src$ dbg/js -j
js> var ta = new Uint8Array(0)
js> Object.prototype[0] = Math
Math
js> for (var i = 0; i < 25; i++) assertEq(ta[0], Math)
js> options()
"anonfunfix,tracejit"
js> options("methodjit")
"anonfunfix,tracejit"
js> for (var i = 0; i < 25; i++) assertEq(ta[0], Math)
typein:6: Error: Assertion failed: got (void 0), expected Math

So I was definitely right, the tracer and methodjit both need to be fixed.
Attachment #517319 - Attachment is obsolete: true
Attachment #520368 - Flags: review?(dvander)
Hrm, what exactly is supposed to happen if you read an out-of-bounds typed array index? I vaguely recall someone saying that it should return undefined and that typed arrays are pretty much magic objects that don't behave like the ES5 spec would want. And the only reason I recall that is because, that tracer code was added to make some typed array demo fast.

That was bug 550351, but I don't see anything about performance there, so I don't know. The patch looks good anyway.
I think ES5 gives you enough rope to implement any semantics you want here, actually.

But no, I don't particularly know what those semantics should be.  It's worth noting that at least Chrome implements what this patch does, and what interpreter currently does, of looking for out-of-bounds indexes up the prototype chain.
Attachment #520368 - Flags: review?(dvander) → review+
Duplicate of this bug: 643234
http://hg.mozilla.org/tracemonkey/rev/5cbe7fd88614
Whiteboard: [sg:critical?][ccbr] → [sg:critical?][ccbr][fixed-in-tracemonkey]
Comment on attachment 520368 [details] [diff] [review]
Full patch and test

It's LIR/tracer typing error, that's bad.  And since there's a methodjit component here, it is at least conceivable that methodjit might rely on the type of the value returned when getting this property.  (I don't know if there is any such dependence.)  So it's off into the weeds, in ways which might be security hazards.  Clear point-release material in my book.
Attachment #520368 - Flags: approval2.0?
blocking2.0: --- → ?
blocking-fx: --- → ?
http://hg.mozilla.org/mozilla-central/rev/5cbe7fd88614
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Want in "macaw" but not approving yet until we get some verification of the trunk fix.
blocking2.0: ? → Macaw
status2.0: --- → wanted
The patch has tests.  They failed before the patch, they pass after.  Do you need more verification than that?  And if so, what kind of verification?
(In reply to comment #20)
> Want in "macaw" but not approving yet until we get some verification of the
> trunk fix.

The patch looks safe. Does verification refer to bake time?
Comment on attachment 520368 [details] [diff] [review]
Full patch and test

Approved for the mozilla2.0 repository, a=dveditz for release-drivers
Attachment #520368 - Flags: approval2.0? → approval2.0+
Transplanted from mozilla-central onto releases/mozilla2.0:

    http://hg.mozilla.org/releases/mozilla-2.0/rev/7c42835e2341
blocking-fx: ? → ---
Group: core-security
Crash Signature: [@ js::DefaultValue]
A test was landed.

-> VERIFIED.
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.