Last Comment Bug 639343 - TM: Crash [@ js::DefaultValue] or "Assertion failure: obj,"
: TM: Crash [@ js::DefaultValue] or "Assertion failure: obj,"
Status: RESOLVED FIXED
: [sg:critical?][ccbr][fixed-in-tracemo...
: assertion, crash, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Jeff Walden (remove +bmo to email)
: general
:
:
: jsfunfuzz 630996 590672
  Show dependency treegraph
 
Reported: 2011-03-06 09:59 PST by Gary Kwong [:nth10sd]
Modified: 2011-05-24 23:34 PDT (History)
11 users (show)
See Also:
Crash Signature:
[@ js::DefaultValue]
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  Macaw+
  .1-fixed
  ---
  unaffected
  ---
  unaffected


Attachments
stacks (4.74 KB, text/plain)
2011-03-06 09:59 PST, Gary Kwong [:nth10sd]
no flags Details
Patch for tracing JIT, with test (1.52 KB, patch)
2011-03-06 16:17 PST, Jeff Walden (remove +bmo to email)
gal: review-
Details | Diff | Splinter Review
Full patch and test (2.54 KB, patch)
2011-03-18 17:47 PDT, Jeff Walden (remove +bmo to email)
dvander: review+
dveditz: approval2.0+
Details | Diff | Splinter Review

Summon comment box

Description Gary Kwong [:nth10sd] 2011-03-06 09:59:49 PST
Created attachment 517282 [details]
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 (remove +bmo to email) 2011-03-06 15:01:52 PST
    /* 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 (remove +bmo to email) 2011-03-06 15:17:15 PST
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 (remove +bmo to email) 2011-03-06 16:17:21 PST
Created attachment 517319 [details] [review]
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 [:nth10sd] 2011-03-06 20:30:25 PST
Bug 639412 may be related.
Comment 5 Gary Kwong [:nth10sd] 2011-03-06 20:41:57 PST
(In reply to comment #3)
> Created attachment 517319 [details] [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 [:nth10sd] 2011-03-06 20:44:57 PST
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 7 Jeff Walden (remove +bmo to email) 2011-03-06 22:12:40 PST
*** Bug 639412 has been marked as a duplicate of this bug. ***
Comment 8 Andreas Gal :gal 2011-03-07 12:12:25 PST
Please renom for .x with a reviewed patch.
Comment 9 Jeff Walden (remove +bmo to email) 2011-03-07 12:55:24 PST
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 2011-03-10 13:52:40 PST
Guessing sg:critical given the LIR error. Please clear if that's wrong.
Comment 11 Andreas Gal :gal 2011-03-10 16:46:56 PST
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 (remove +bmo to email) 2011-03-18 17:04:40 PDT
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 (remove +bmo to email) 2011-03-18 17:47:37 PDT
Created attachment 520368 [details] [review]
Full patch and test
Comment 14 David Anderson [:dvander] 2011-03-18 18:04:11 PDT
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 (remove +bmo to email) 2011-03-18 19:18:00 PDT
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 16 Jeff Walden (remove +bmo to email) 2011-03-21 11:34:48 PDT
*** Bug 643234 has been marked as a duplicate of this bug. ***
Comment 17 Jeff Walden (remove +bmo to email) 2011-03-25 17:29:20 PDT
http://hg.mozilla.org/tracemonkey/rev/5cbe7fd88614
Comment 18 Jeff Walden (remove +bmo to email) 2011-03-25 18:13:54 PDT
Comment on attachment 520368 [details] [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] 2011-03-31 14:27:22 PDT
http://hg.mozilla.org/mozilla-central/rev/5cbe7fd88614
Comment 20 Daniel Veditz 2011-04-01 11:38:03 PDT
Want in "macaw" but not approving yet until we get some verification of the trunk fix.
Comment 21 Jeff Walden (remove +bmo to email) 2011-04-06 12:43:14 PDT
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] 2011-04-08 16:11:58 PDT
(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 2011-04-11 10:57:25 PDT
Comment on attachment 520368 [details] [review]
Full patch and test

Approved for the mozilla2.0 repository, a=dveditz for release-drivers
Comment 24 Christian Legnitto [:LegNeato] 2011-04-11 15:35:00 PDT
Transplanted from mozilla-central onto releases/mozilla2.0:

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

Note You need to log in before you can comment on or make changes to this bug.