486139 - "Assertion failure: op == JSOP_ENUMELEM || op == JSOP_ENUMCONSTELEM, at ../jsopcode.cpp"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

uneval((function(){([z] = []).y >>= x}));

Assertion failure: op == JSOP_ENUMELEM || op == JSOP_ENUMCONSTELEM, at ../jsopcode.cpp:1443

This looks related to bug 453492 since the latter was just recently fixed-on-TM.

Comment 1

[Igor Bukanov]

([z] = []).y >>= x; corresponds to the following bytecode:

00000:  newarray 0
00004:  dup
00005:  zero
00006:  getelem
00007:  bindname "z"
00010:  qnamepart "z"
00013:  enumelem
00014:  dup
00015:  getprop "y"
00018:  name "x"
00021:  rsh
00022:  setprop "y"
00025:  pop

The bug happens when DecompileDestructuring checks whether it finished its at the following code, see http://hg.mozilla.org/tracemonkey/file/e824252798be/js/src/jsopcode.cpp#l1623:

if (pc == endpc || *pc != JSOP_DUP)
    break;

/*
 * Check for SRC_DESTRUCT on this JSOP_DUP, which would mean another
 * destructuring initialiser abuts this one, and we should stop.  This
 * happens with source of the form '[a] = [b] = c'.
 */
sn = js_GetSrcNote(jp->script, pc);
if (sn && SN_TYPE(sn) == SRC_DESTRUCT)
    break;

Here the code misinterprets JSOP_DUP that dups the left-hand-side of >>= for the dup that indicates another element of the destructuring array. I guess the code must adds a source note for each dup in the destructors to distinguish this case.

Comment 2

[Igor Bukanov]

This was hidden bug prior changes from the bug 460501. Before that bag landed JSOP_DUP comming from op= implementation would be prefixed in the bytecode with JSOP_GROUP. But the bug removed JSOP_GROUP and exposed this decompiler deficiency.

I nominate this bug as 1.9.1 blocker as I cannot easily see if this bug cannot be used to subvert a decompiler. In the above example the decompiler managed to misinterpret few unrelated bytecodes as a part of the decompiler sequence before hitting the local assert so to prove that the bug is not critical one has to analyze quite a few branches in the decompiler. It is more efficient to spend this time to fix this bug.

Comment 3

[Igor Bukanov]

Attachment: v1

The patch annotates JSOP_DUP that continues the destructuring pattern with SRC_CONTINUE. There is also an extra assert to ensure that that dup is either SRC_DESTRUCT or SRC_CONTINUE or noteless. I have not added an alias for reused SRC_CONTINUE since its matches the usage rather well on its own.

The patch also removes stalled comments about no longer existing JSOP_GROUP from jsparse.cpp.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 370486 [details] [diff] [review]
v1

Perfect! Hope this is the last hidden dependency on JSOP_GROUP...

/be

Comment 5

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/42deb57d5805

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/42deb57d5805

Comment 7

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/190cc81d3cda

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with testcase given in comment 0 on trunk and 1.9.1 with the
following debug builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre)
Gecko/20090422 Minefield/3.6a1pre ID:20090422224452

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre)
Gecko/20090422 Shiretoko/3.5b4pre ID:20090422122043

Comment 9

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929