Closed Bug 541255 Opened 10 years ago Closed 10 years ago

Crash [@ js_GC] or "Assertion failure: obj->isDenseArray(), at ../jsarray.cpp"

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
Tracking Status
blocking2.0 --- alpha4+
blocking1.9.2 --- .20+
status1.9.2 --- .20-fixed
blocking1.9.1 --- needed
status1.9.1 --- wanted

People

(Reporter: gkw, Assigned: jorendorff)

References

(Blocks 1 open bug)

Details

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

Crash Data

Attachments

(1 file)

(function(e) {
  eval("\
    [(function() {\
      x.k = function(){}\
    })() \
    for (x in [0])]\
  ")
})()

asserts js debug shell without -j on TM tip at Assertion failure: OBJ_IS_DENSE_ARRAY(cx, obj), at ../jsarray.cpp:2290

autoBisect shows this is probably related to bug 452498:

The first bad revision is:
changeset:   26784:2cf0bbe3772a
user:        Brendan Eich
date:        Sun Apr 05 21:17:22 2009 -0700
summary:     upvar2, aka the big one take 2 (452598, r=mrbkap).
Tricksy.  Look for a patch after I finish a review and make regex literals trace again.

The bisect result looks red-herring-ish; I think extra tracing just happened to expose a latent bug.

Writing user-specified values into dslots seems very very dodgy.  I don't want to have to think about how or whether that could be exploited, erring on the side of paranoia.

I take it this is the assert at start of js_ArrayCompPush?  2290 lands on this right now according to hgweb:

  2289         ok = js_MergeSort(vec, (size_t) newlen, elemsize,
  2290                           sort_compare_strings, cx, mergesort_tmp);
Assignee: general → jwalden+bmo
Group: core-security
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [sg:critical?]
(In reply to comment #1)
> I take it this is the assert at start of js_ArrayCompPush?  2290 lands on this
> right now according to hgweb:

Yup, should be that assert:

js> (function(e) {
  eval("\
    [(function() {\
      x.k = function(){}\
    })() \
    for (x in [0])]\
  ")
})()
Assertion failure: OBJ_IS_DENSE_ARRAY(cx, obj), at ../jsarray.cpp:2407

Program received signal SIGABRT, Aborted.
0x00007fff85343fe6 in __kill ()
(gdb) bt
#0  0x00007fff85343fe6 in __kill ()
#1  0x0000000100138deb in JS_Assert (s=0x1001c8b9c "OBJ_IS_DENSE_ARRAY(cx, obj)", file=0x1001bf4e3 "../jsarray.cpp", ln=2407) at ../jsutil.cpp:70
#2  0x00000001000287a3 in js_ArrayCompPush (cx=0x100411580, obj=0x1003f6600, v=22) at ../jsarray.cpp:2407
#3  0x00000001000a03c5 in js_Interpret (cx=0x100411580) at jsops.cpp:4142
#4  0x00000001000a42ec in js_Execute (cx=0x100411580, chain=0x1003f6580, script=0x100415060, down=0x100889450, flags=16, result=0x7fff5fbfe988) at jsinterp.cpp:1620
#5  0x00000001000bf9e1 in obj_eval (cx=0x100411580, obj=0x1003f6000, argc=1, argv=0x100889520, rval=0x7fff5fbfe988) at ../jsobj.cpp:1543
#6  0x00000001000a525a in js_Invoke (cx=0x100411580, argc=1, vp=0x100889510, flags=2) at jsinterp.cpp:1378
#7  0x0000000100090cc2 in js_Interpret (cx=0x100411580) at jsops.cpp:2305
#8  0x00000001000a42ec in js_Execute (cx=0x100411580, chain=0x1003f6000, script=0x100414fe0, down=0x0, flags=0, result=0x7fff5fbff5e8) at jsinterp.cpp:1620
#9  0x00000001000113f6 in JS_ExecuteScript (cx=0x100411580, obj=0x1003f6000, script=0x100414fe0, rval=0x7fff5fbff5e8) at ../jsapi.cpp:4970
#10 0x0000000100009cac in Process (cx=0x100411580, obj=0x1003f6000, filename=0x0, forceTTY=0) at ../../shell/js.cpp:532
#11 0x000000010000a574 in ProcessArgs (cx=0x100411580, obj=0x1003f6000, argv=0x7fff5fbff7e8, argc=0) at ../../shell/js.cpp:848
#12 0x000000010000a88b in main (argc=0, argv=0x7fff5fbff7e8, envp=0x7fff5fbff7f0) at ../../shell/js.cpp:4898
(gdb) f 2
#2  0x00000001000287a3 in js_ArrayCompPush (cx=0x100411580, obj=0x1003f6600, v=22) at ../jsarray.cpp:2407
2407        JS_ASSERT(OBJ_IS_DENSE_ARRAY(cx, obj));
(gdb) l
2402    }
2403
2404    JSBool JS_FASTCALL
2405    js_ArrayCompPush(JSContext *cx, JSObject *obj, jsval v)
2406    {
2407        JS_ASSERT(OBJ_IS_DENSE_ARRAY(cx, obj));
2408        uint32_t length = (uint32_t) obj->fslots[JSSLOT_ARRAY_LENGTH];
2409        JS_ASSERT(length <= js_DenseArrayCapacity(obj));
2410
2411        if (length == js_DenseArrayCapacity(obj)) {
(gdb)
(In reply to comment #1)
> I take it this is the assert at start of js_ArrayCompPush?  2290 lands on this
> right now according to hgweb:
> 
>   2289         ok = js_MergeSort(vec, (size_t) newlen, elemsize,
>   2290                           sort_compare_strings, cx, mergesort_tmp);

Apologies, I think I copied the assert with the wrong line number. I had taken the assert from TM repository 2cf0bbe3772a, which was from 2009. Comment 2 should be the up-to-date one.
Now asserts at Assertion failure: obj->isDenseArray(), at ../jsarray.cpp:2380

A worse-looking testcase is coming up - this second one seems to be able to crash opt shells too.
Summary: "Assertion failure: OBJ_IS_DENSE_ARRAY(cx, obj), at ../jsarray.cpp" → "Assertion failure: obj->isDenseArray(), at ../jsarray.cpp"
function f(e) {
  eval("\
    [((function g(o, bbbbbb) {\
      if (aaaaaa = bbbbbb) {\
        return window.r = []\
      }\
      g(aaaaaa, bbbbbb + 1);\
      #3={}\
    })([], 0)) \
    for (window in this) \
    for each(x in [0, 0])\
    ]\
  ")
}
t = 1;
f()

crashes opt shell on TM tip without -j at js_GC near null and asserts js debug shell on TM tip without -j at Assertion failure: obj->isDenseArray(), at ../jsarray.cpp:2380

===

Mac js opt crash stack:

Exception Type:  EXC_BAD_ACCESS (SIGBUS)
Exception Codes: KERN_PROTECTION_FAILURE at 0x0000000000000016
Crashed Thread:  0  Dispatch queue: com.apple.main-thread

Thread 0 Crashed:  Dispatch queue: com.apple.main-thread
0   js-opt-32-tm-darwin           	0x0004f392 js_GC + 5762
1   js-opt-32-tm-darwin           	0x0002085b js_DestroyContext(JSContext*, JSDestroyContextMode) + 155
2   js-opt-32-tm-darwin           	0x0000eda9 JS_DestroyContext + 25
3   js-opt-32-tm-darwin           	0x00008cf5 main + 1189
4   js-opt-32-tm-darwin           	0x000029ed _start + 208
5   js-opt-32-tm-darwin           	0x0000291c start + 40
Keywords: crash
Summary: "Assertion failure: obj->isDenseArray(), at ../jsarray.cpp" → Crash [@ js_GC] or "Assertion failure: obj->isDenseArray(), at ../jsarray.cpp"
Whiteboard: [sg:critical?] → [ccbr][sg:critical?]
function f(e) {
    eval("[function () { w.r = 0 }() for (w in [0])]")
}
f(0);

The lambda compiles to:

00000:  trace
00001:  name "w"
00004:  zero
00005:  setprop "r"
00008:  pop
00009:  stop

The NAME instruction is loading the wrong slot. It should get the string "0". Instead it gets the array being constructed.

The crash goes away if I delete the parameter `e`.
*steal* Should be quickish.
Assignee: jwalden+bmo → jorendorff
Correction: If I pass the lambda to dis(), it compiles to the code in comment 6. In the actual crashing test, it compiles to:

main:
00000:   4  trace
00001:   4  getupvar 0      <--- insn to load w
00004:   4  zero
00005:   4  setprop "r"
00008:   4  pop
00009:   4  stop

The upvar cookie is MAKE_UPVAR_COOKIE(1, 1). Comprehension-transplanting trouble, maybe.
I will look at this and try to advise... I'll be the advisor, like Dr. Lazarus in "Galaxy Quest". Maybe cdleary (who has gumption) can lose his shirt actually fighting the rock monster.

"Look around you, can you form some kind of rudimentary lathe?" - Guy Fleegman

/be
It's not comprehension-transplanting trouble. This fails too:

  function f(y) {
      eval("let (z=2, w=y) { (function () { w.p = 7; })(); }");
  }
  var x = {};
  f(x);
  assertEq(x.p, 7);

Here's what I think the trouble is. In js_GetUpvar, there are four cases:

    if (!fp->fun) {
        vp = fp->slots + fp->script->nfixed;
    } else if (slot < fp->fun->nargs) {
        vp = fp->argv;
    } else if (slot == CALLEE_UPVAR_SLOT) {
        vp = &fp->argv[-2];
        slot = 0;
    } else {
        slot -= fp->fun->nargs;
        JS_ASSERT(slot < fp->script->nslots);
        vp = fp->slots;
    }

fp is the frame in which w is defined: the eval frame. I think we should go to the first branch. Instead we end up in the fourth, adjusting for fp->fun->nargs, which I'm pretty sure makes no sense in this frame.
Attached patch v1Splinter Review
This isn't going to win any awards, but it seems like the minimal change.

We don't attempt to JIT this case:

trace stopped: 13951: Null closure function object parent must be global object
Abort recording of tree tests/js1_8_5/regress/regress-541255-4.js:8@60 at tests/js1_8_5/regress/regress-541255-4.js:8@64: lambda.

Apparently the lambda created by the compiler (invoked from eval) is not parented to the Call object for the enclosing function, not the global. Is that correct?

I added another test anyway, just in case that ever gets traced.
Attachment #435306 - Flags: review?(brendan)
(In reply to comment #11)
> Apparently the lambda created by the compiler (invoked from eval) is not

Stray "not" at end of this line, right?

> parented to the Call object for the enclosing function, not the global. Is that
> correct?

Correct, striking the stray not.

> I added another test anyway, just in case that ever gets traced.

Thanks. This is the notorious MakeUpvarForEval special case. I thought we had it covered (there are other tests; dmandelin knows of them too). Thanks again, will r+ with a nit in a sec.

/be
Comment on attachment 435306 [details] [diff] [review]
v1

>+    if (!fp->fun || fp->flags & JSFRAME_EVAL) {

House style overparenthesizes bitwise against logical connectives, as well as against most other ops.

>+    if (!fp->fun || fp->flags & JSFRAME_EVAL) {

Ditto.

/be
Attachment #435306 - Flags: review?(brendan) → review+
http://hg.mozilla.org/tracemonkey/rev/00caf9fe2748
Whiteboard: [ccbr][sg:critical?] → [ccbr][sg:critical?][fixed-in-tracemonkey]
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
Want this on branches, but for now not blocking 1.9.1/1.9.2 pending landing on mozilla-central. Please request approval when you're ready to land.
blocking1.9.1: ? → needed
blocking1.9.2: ? → needed
blocking2.0: ? → alpha4+
http://hg.mozilla.org/mozilla-central/rev/00caf9fe2748
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Gary: Does this really affect the 1.9.1 branch as you nominated? Elsewhere it looks like it was pinned down to a regression from a fix that didn't land on that branch.
blocking1.9.2: needed → .8+
(In reply to comment #17)
> Gary: Does this really affect the 1.9.1 branch as you nominated? Elsewhere it
> looks like it was pinned down to a regression from a fix that didn't land on
> that branch.

Yes, it really does still affect. Upvar2 landed on 1.9.1, and the testcases in comment #0 and comment #6 still assert:

32-bit 1.9.1 changeset 2eb0724d74f8:

$ ./js-dbg-32-191-darwin 
js> function f(e) {
    eval("[function () { w.r = 0 }() for (w in [0])]")
}
f(0);

js> Assertion failure: OBJ_IS_DENSE_ARRAY(cx, obj), at ../jsarray.cpp:2361
Trace/BPT trap
blocking1.9.1: needed → .12+
Whiteboard: [ccbr][sg:critical?][fixed-in-tracemonkey] → [need branch patch][sg:critical?][fixed-in-tracemonkey][ccbr]
Does the same patch work for 1.9.1 and 1.9.2?
Moving forward to the next release as this didn't make it for 3.6.9 and 3.5.12
blocking1.9.1: .12+ → .13+
blocking1.9.2: .9+ → .10+
blocking1.9.1: .14+ → .15+
blocking1.9.2: .11+ → .12+
blocking1.9.1: .16+ → ?
blocking1.9.2: .13+ → ?
blocking1.9.1: ? → .18+
blocking1.9.2: ? → .15+
Whiteboard: [need branch patch][sg:critical?][fixed-in-tracemonkey][ccbr] → [needs answer to comment 19 from jorendorff][sg:critical?][fixed-in-tracemonkey][ccbr]
Deferring to a future point release as we have run out of time. If this absolutely needs to be fixed and cal land today or tomorrow, please send a note to release-drivers@mozilla.org
blocking1.9.1: .19+ → .20+
blocking1.9.2: .17+ → .18+
blocking1.9.1: .20+ → needed
blocking1.9.2: .18+ → .19+
Crash Signature: [@ js_GC]
dmandelin: please find someone on your team to get this into the 1.9.2 branch before code freeze (Aug 1).
Attachment #435306 - Flags: approval1.9.2.20?
(In reply to comment #22)
> dmandelin: please find someone on your team to get this into the 1.9.2
> branch before code freeze (Aug 1).

The existing patch applies directly, so requesting approval on that.

Btw, feel free to ask the original patch author directly to do branch landings.
Whiteboard: [needs answer to comment 19 from jorendorff][sg:critical?][fixed-in-tracemonkey][ccbr] → [sg:critical?][fixed-in-tracemonkey][ccbr]
Comment on attachment 435306 [details] [diff] [review]
v1

Approved for 1.9.2.20, a=dveditz for release-drivers
Attachment #435306 - Flags: approval1.9.2.20? → approval1.9.2.20+
Group: core-security
A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-541255-2.js.
Flags: in-testsuite+
Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.