476073 - "Assertion failure: op2 == JSOP_INITELEM" involving object literal with setter, trap on initprop

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> f = function() { ({ y setter: print }); }
function () {
    ({y setter: print});
}
js> dis(f);
flags: LAMBDA INTERPRETED
main:
00000:  newinit 1
00002:  name "print"
00005:  setter
00006:  initprop "y"      <-- trap goes here
00009:  endinit
00010:  pop
00011:  stop

Source notes:

js> trap(f, 6, '');
js> f();
Assertion failure: op2 == JSOP_INITELEM, at ../jsinterp.cpp:5970

Comment 1

[Andrei Saprykin]

The problem occurs because the interpretor assumes that the second instruction of the compound instruction cannot be the trap instruction. As we discussed with Igor the patch should change the function trap(). If the offset parameter points to any instruction of a compound instruction the function always substitutes the first instruction in the the compound instruction.

Comment 2

[Jesse Ruderman]

Which two instructions form a "compound instruction"?  Will the proposed solution end up fixing bug 429239?

Comment 3

[Andrei Saprykin]

The "compound instruction" seems to be 05: setter 06: initprop "y". As for the bug 429239 I think that the js shell should check that the offset points to the beginning of the instruction and the instruction is not a part of a compound instruction. If any condition is broken then the shell has to report an error.

Comment 4

[Brendan Eich [:brendan]]

That's right: JSOP_GETTER and JSOP_SETTER are prefix opcodes that dispatch their successor opcodes. No source level debugger will ever plant a JSOP_TRAP on the successor. We should enforce this at a low level.

/be

Comment 5

[Brian Crowder]

How many opcodes are there like this?  Jesse, can you just specialize your fuzzer to avoid "successor" opcodes, to workaround this?

Comment 6

[Jesse Ruderman]

I told jsfunfuzz to not put traps on opcodes that follow "setter" or "getter" .  (Before, I had it not put traps on functions that contained "get" or "set"; the new exclusion is more precise.)

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #5)
> How many opcodes are there like this?

If my aging memory serves (which is a hint to check for yourself, not ask and trust :-P), JSOP_GETTER and JSOP_SETTER are hardcoded to dispatch their successors.

Note that JSOP_EQ/NE/LT/LE/GE/GT all try to dispatch their successor op if it's JSOP_IF{EQ,NE}. This is an optimization, so if the equality or relational op sees JSOP_TRAP it'll just let the main dispatch cycle handle the trap.

/be

Comment 8

[Andrei Saprykin]

Attachment: Proposed patch

Comment 9

[Andrei Saprykin]

Attachment: Updated patch

Comment 10

[Igor Bukanov]

Comment on attachment 360723 [details] [diff] [review]
Updated patch

>diff -r e7fffaf1f9fa js/src/jsapi.cpp
>--- a/js/src/jsapi.cpp	Sun Feb 01 23:16:39 2009 -0800
>+++ b/js/src/jsapi.cpp	Thu Feb 05 16:32:42 2009 +0100
>@@ -695,16 +695,63 @@ JS_PUBLIC_API(const char *)
> JS_PUBLIC_API(const char *)
> JS_GetTypeName(JSContext *cx, JSType type)
> {
>     if ((uintN)type >= (uintN)JSTYPE_LIMIT)
>         return NULL;
>     return JS_TYPE_STR(type);
> }
> 
>+JS_PUBLIC_API(uint32)
>+JS_FindProperTrapOffset(JSContext *cx, JSScript *script, uint32 offset)
>+{

Nit: call this JS_FindInstructionStart to reflect the exact nature of the code. 

>+    jsbytecode *pc, *end, *target, *prefix;
>+    uintN len;
>+    JSOp op;
>+    const JSCodeSpec *cs;
>+    JSBool hasPrefix = JS_FALSE;

Nit: instead of hasPrefix set prefix to NULL. It will also silence GCC warning about uninitialized variables.

>+
>+    JS_ASSERT(offset < script->length);
>+    if (!offset)
>+        return offset;
>+    pc = script->code;
>+    target = script->code + offset;
>+    end = script->code + script->length;
>+    while (pc < end) {

Given the "offset < script->length" condition pc < end must always be true before the loop returns. So use a for loop with an extra assert like:

for (pc = script->code; ; pc += len) {
    JS_ASSERT(pc < end);
   

>+        if (pc + len == target) {
>+            if (hasPrefix)
>+                return prefix - script->code;
>+            return offset;
>+        }
>+        if (target < pc + len) {
>+            if (hasPrefix)
>+                return prefix - script->code;
>+            return pc - script->code;
>+        }

Both ifs can be combined into just

>+        if (target <= pc + len) {
>+            if (hasPrefix)
>+                return prefix - script->code;
>+            return pc - script->code;
>+        }

But then even simple code would be to move this code right at the beginning of the loop and check:

>+        if (target <= pc) {
>+            if (hasPrefix)
>+                return prefix - script->code;
>+            return pc - script->code;
>+        }

> /*
>+ * Returns proper offset for the trap instruction. If the provided offset is
>+ * OK the same offset is returned. If the provided offset points somewhere in
>+ * the middle of the instruction then the offset of the beginning of the
>+ * instruction is returned. If the provided offset point to instruction that
>+ * has prefix instruction then the offset of the prefix instruction is returned.
>+ */
>+extern JS_PUBLIC_API(uint32)
>+JS_FindProperTrapOffset(JSContext *cx, JSScript *, uint32 offset);

Nit: rename and update comments about it.

>@@ -1374,16 +1374,20 @@ Trap(JSContext *cx, JSObject *obj, uintN
>     }
>     argc--;
>     str = JS_ValueToString(cx, argv[argc]);
>     if (!str)
>         return JS_FALSE;
>     argv[argc] = STRING_TO_JSVAL(str);
>     if (!GetTrapArgs(cx, argc, argv, &script, &i))
>         return JS_FALSE;
>+    if (JS_FindProperTrapOffset(cx, script, i) != i) {
>+        JS_ReportError(cx, "Wrong trap offset");
>+        return JS_FALSE;
>+    }

Here JS_FindProperTrapOffset require that offset < script->length. So the code must check for that. Alternatively JS_FindProperTrapOffset may explicitly allow for arbitrary offset and just return the same result as for script->length - 1. 

Another idea is to avoid error reporting and just sets the trap at the proper offset. It would allow fore better usability of the trap function. But I am not the right person to judge about it.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 360723 [details] [diff] [review]
Updated patch

>+++ b/js/src/jsapi.cpp	Thu Feb 05 16:32:42 2009 +0100

The new API belongs in jsdbgapi.{h,cpp}, not jsapi.{h,cpp}.

>+        }
>+        else {

Cuddle } with else in SpiderMonkey style (after K&R and original Unix).

/be

Comment 12

[Andrei Saprykin]

Attachment: Updated patch

Comment 13

[Igor Bukanov]

Comment on attachment 361549 [details] [diff] [review]
Updated patch

>+JS_FindInstructionStart(JSContext *cx, JSScript *script, uint32 offset)
...
>+            if (target == pc + len)
>+                return offset;
>+            else
>+                return pc - script->code;

Nit: no need for the else here. Alternatively, use return c ? b : c for dense code.

>@@ -1337,16 +1337,20 @@ Trap(JSContext *cx, JSObject *obj, uintN

>     if (!GetTrapArgs(cx, argc, argv, &script, &i))
>         return JS_FALSE;
>+    if (JS_FindInstructionStart(cx, script, i) != i) {

JS_FindInstructionStart asserts when the offset exceeds the length of the script. Thus the code should check for that before calling the function. 

>+        JS_ReportError(cx, "Wrong trap offset");
>+        return JS_FALSE;

I am not sure that this is right thing to do from the usability point of view.

To  Jesse Ruderman: do you have an opinion about behavior of trap() when its argument points inside instruction or outside of the bytecode? I.e. should it throw an error or just set the trap to the nearest valid point?

Comment 14

[Jesse Ruderman]

I would prefer for it to throw an error.  If I give it an incorrect offset, it's probably because the bytecode has changed, and if Spidermonkey tries to guess I might end up trapping on the wrong instruction.

Comment 15

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

Is this related to bug 429237 - "trap() JS shell builtin should check for setting a trap twice (instead of asserting)" in any way ? (It's mentioned "instead of asserting, the trap() builtin should raise an exception." there)

Comment 16

[Igor Bukanov]

Comment on attachment 361549 [details] [diff] [review]
Updated patch

Mass clearing stalled request reviews.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

js> f = function() { ({ y setter: print }); }
typein:1: SyntaxError: missing : after property id:
typein:1: f = function() { ({ y setter: print }); }
typein:1: ......................^

Is this still valid or does the test just need changing?

Comment 18

[Jeff Walden [:Waldo]]

That syntax has since been removed; try "set y(v) { print(v); }" maybe?  In any case, I don't think that change would fix this bug.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

js> f = function() { ({ set y(v) { print(v); } }); }
(function () {({set y(v) {print(v);}});})
js> dis(f);
flags: LAMBDA NULL_CLOSURE
loc     op
-----   --
main:
00000:  newinit 1
00005:  lambda (function (v) {print(v);})
00010:  setter
00011:  initprop "y"
00014:  endinit
00015:  pop
00016:  stop

Source notes:
 ofs  line    pc  delta desc     args
---- ---- ----- ------ -------- ------

js> trap(f, 6, '');
js> f();
js>

I note that flags went from LAMBDA_INTERPRETED to LAMBDA NULL_CLOSURE.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Passes when I trap 11 too.