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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: 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

Comment 1

[Jeff Walden [:Waldo]]

    /* 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!

Comment 2

[Jeff Walden [:Waldo]]

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?)

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Patch for tracing JIT, with test

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.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Bug 639412 may be related.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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...

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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..

Comment 8

[Andreas Gal :gal]

Please renom for .x with a reviewed patch.

Comment 9

[Jeff Walden [:Waldo]]

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.)

Comment 10

[Daniel Veditz [:dveditz]]

Guessing sg:critical given the LIR error. Please clear if that's wrong.

Comment 11

[Andreas Gal :gal]

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.

Comment 12

[Jeff Walden [:Waldo]]

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.

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Full patch and test

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 15

[Jeff Walden [:Waldo]]

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.

Comment 17

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/5cbe7fd88614

Comment 18

[Jeff Walden [:Waldo]]

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.

Comment 19

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/5cbe7fd88614

Comment 20

[Daniel Veditz [:dveditz]]

Want in "macaw" but not approving yet until we get some verification of the trunk fix.

Comment 21

[Jeff Walden [:Waldo]]

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?

Comment 22

[David Anderson [:dvander] - inactive, e-mail if emergency]

(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 23

[Daniel Veditz [:dveditz]]

Comment on attachment 520368 [details] [diff] [review]
Full patch and test

Approved for the mozilla2.0 repository, a=dveditz for release-drivers

Comment 24

[LegNeato]

Transplanted from mozilla-central onto releases/mozilla2.0:

    http://hg.mozilla.org/releases/mozilla-2.0/rev/7c42835e2341

Comment 25

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

A test was landed.

-> VERIFIED.