429615 - Using untrapped copy of JSScript.code to avoid JSOP_TRAP checks.

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

+++ This is a spin-off of bug #422137 +++

The recent fuzzer improvment to include the trap function from the js shell has reveled quite a few bugs where the code have not checked for JSOP_TRAP when accessing JSScript.code.

Instead of adding JSOP_TRAP checks to all such places an idea would be to add JSScript.origCode or similar field to JSScript. This field  should hold the original bytecode without JSOP_TRAP mutations. The field can be initially initialized to point to JSScript.code. The first JS_SetTrap for the JSScript would then clone JSScript.code before mutating bytecodes into JSOP_TRAP.

Comment 1

[Igor Bukanov]

Recording patch dependency

Comment 2

[Igor Bukanov]

Attachment: v1

This is a version of the patch from bug 422137 commenet 64 with JSScript.prologLength changes factored out into the patch from bug 429616 cooment 1.

The patch adds JSScript.origCode which is cloned from JSScript.code when JS_SetTrap sets the first trap for the script and updates the rest of code to access JSScript.origCode, not JSScript.code, when reading the bytecode.

During the development I renamed temporary JSScript.code into JSScript._code to reveal all the places in the code that required an update.

Comment 3

[Brendan Eich [:brendan]]

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

> JS_PUBLIC_API(JSOp)
> JS_GetTrapOpcode(JSContext *cx, JSScript *script, jsbytecode *pc)
> {
>-    JSRuntime *rt;
>-    JSTrap *trap;
>-    JSOp op;
>+    if ((size_t) (pc - script->origCode) > script->length)

Bug: >=, not > here.

>+        pc = script->origCode + (pc - script->code);

An assertion that pc is in [script->code, script->code + script->length) would be good.

>@@ -304,21 +287,20 @@ JS_HandleTrap(JSContext *cx, JSScript *s
> #endif
>     }
>     DBG_UNLOCK(cx->runtime);
> 
>     /*
>      * It's important that we not use 'trap->' after calling the callback --
>      * the callback might remove the trap!
>      */
>-    op = (jsint)trap->op;
>     status = trap->handler(cx, script, pc, rval, trap->closure);
>     if (status == JSTRAP_CONTINUE) {
>         /* By convention, return the true op to the interpreter in rval. */
>-        *rval = INT_TO_JSVAL(op);
>+        *rval = INT_TO_JSVAL(script->origCode[pc - script->code]);

Note well for next comment/question.

>@@ -5363,17 +5363,17 @@ interrupt:
> #endif /* JS_HAS_EXPORT_IMPORT */
> 
>           BEGIN_CASE(JSOP_TRAP)
>             switch (JS_HandleTrap(cx, script, regs.pc, &rval)) {
>               case JSTRAP_ERROR:
>                 goto error;
>               case JSTRAP_CONTINUE:
>                 JS_ASSERT(JSVAL_IS_INT(rval));
>-                op = (JSOp) JSVAL_TO_INT(rval);
>+                op = (JSOp) script->origCode[regs.pc - script->code];

JSVAL_TO_INT(rval) is unused here -- why?

> const char *
>-js_ComputeFilename(JSContext *cx, JSStackFrame *caller,
>+js_ComputeFilename(JSContext *cx, JSStackFrame *fp,

Was caller renamed fp just to shorten later expressions, or is caller an inaccurate name?

>+    if (fp->regs &&
>+        fp->script->origCode[fp->regs->pc - fp->script->code] == JSOP_EVAL) {
>+        JS_ASSERT(fp->script->origCode[fp->regs->pc - fp->script->code +
>+                                       JSOP_EVAL_LENGTH] == JSOP_LINENO);

This sort of (fp->script->origCode[fp->regs->pc - fp->script->code...]) expression happens elsewhere -- macroize?

>+    if ((size_t) (pc - script->origCode) > script->length)
>+        pc = script->origCode + (pc - script->code);

See first comments above -- look for recurrences.

/be

Comment 4

[Igor Bukanov]

Sorry for not obsoleting the apatch here earlier: it does not work with a multithreaded debugger, see bug 422137 comment 67.   

Comment 5

[Igor Bukanov]

Introducing JSScript.origCode is wrong idea as it introduces inherent races. See bug 430293 for a better one.