387955 - Crash [@ JS_CallTracer]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

To reproduce the crash, load the following script in an *opt* jsshell.  (Debug jsshell does not crash!)

You'll need -v 170 because it uses the "yield" keyword.


var fal = false;

function gen() { 
  let x;
  function y(){} 
  this.__defineGetter__('', function(){}); 
  if (fal)
    yield;
}

for (var i in gen()) { }

gc();

Comment 1

[Brian Crowder]

Fun:

crowdermac:~/mozilla/js/src crowder$ make -f Makefile.ref BUILD_OPT=1 clean all -j3 -s
crowdermac:~/mozilla/js/src crowder$ ./Darwin_OPT.OBJ/js -v 170 -f tmp.js
before 27696, after 27696, break 01008000

crowdermac:~/mozilla/js/src crowder$ make -f Makefile.ref BUILD_OPT=1 OPTIMIZER="-Os" clean all -j3 -s
crowdermac:~/mozilla/js/src crowder$ ./Darwin_OPT.OBJ/js -v 170 -f tmp.js
Segmentation fault

Yay, optimizer.

Comment 2

[Jesse Ruderman]

mrbkap was able to reproduce using Valgrind and a debug build.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: valgrind output

This is the valgrind output. The line that the first warning points to is:
           v = STOBJ_GET_SLOT(obj, i);
==>        if (JSVAL_IS_TRACEABLE(v)) {

The object in question is a Block object (the one created for the let in the generator function). i is 5, nslots is 6 (JS_INITIAL_NSLOTS). I'm not really sure how that's possible.

Comment 4

[Igor Bukanov]

Attachment: valgrind output for more explicit test

Here is a modified test to run with debug shell:

var fal = false;
var f;

function gen() {
    let x;
    function y(){}
    f = function(){};
    if (fal)
        yield;
}

for (var i in gen()) { }

dumpHeap(null, f, null, 2, this);

It clearly shows that the problem is that Block object for the let block is initialized here when tracing.

Comment 5

[Igor Bukanov]

Attachment: valgrind output for more explicit test

The last output had wrong ordering of lines

Comment 6

[Igor Bukanov]

I have found the problem. For the above test cases the interpreter calls js_PutBlockObject twice for block containin local "x". The first call comes from:

          BEGIN_CASE(JSOP_RETRVAL)    /* fp->rval already set */
            ASSERT_NOT_THROWING(cx);
            if (inlineCallCount)
          inline_return:
            {
                JSInlineFrame *ifp = (JSInlineFrame *) fp;
                void *hookData = ifp->hookData;

                /*
                 * If fp has blocks on its scope chain, home their locals now,
                 * before calling any debugger hook, and before freeing stack.
                 * This matches the order of block putting and hook calling in
                 * the "out-of-line" return code at the bottom of js_Interpret
                 * and in js_Invoke.
                 */
                if (fp->flags & JSFRAME_POP_BLOCKS) {
                    SAVE_SP_AND_PC(fp);
                    ok &= PutBlockObjects(cx, fp);
                }
 
The second call is from:

          BEGIN_CASE(JSOP_LEAVEBLOCKEXPR)
          BEGIN_CASE(JSOP_LEAVEBLOCK)
          {
            JSObject **chainp;

            /* Grab the result of the expression. */
            if (op == JSOP_LEAVEBLOCKEXPR)
                rval = FETCH_OPND(-1);

            chainp = &fp->blockChain;
            obj = *chainp;
            if (!obj) {
                chainp = &fp->scopeChain;
                obj = *chainp;

                /*
                 * This block was cloned, so clear its private data and sync
                 * its locals to their property slots.
                 */
                SAVE_SP_AND_PC(fp);
                ok = js_PutBlockObject(cx, obj);
                if (!ok)
                    goto out;
            }

This second time the block obj does not have frame associated with it so when js_PutBlockObject calls sprop->getter == block_getProperty, the getter returns early without setting v:

JSBool
js_PutBlockObject(JSContext *cx, JSObject *obj)
{
    JSScopeProperty *sprop;
    jsval v;

    for (sprop = OBJ_SCOPE(obj)->lastProp; sprop; sprop = sprop->parent) {
...
        if (!sprop->getter(cx, obj, INT_TO_JSVAL(sprop->shortid), &v) ||
            !js_DefineNativeProperty(cx, obj, sprop->id,
                                     v, NULL, NULL,
                                     JSPROP_ENUMERATE | JSPROP_PERMANENT,
                                     SPROP_HAS_SHORTID, sprop->shortid,
                                     NULL)) {
...
}

static JSBool
block_getProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
{
    JSStackFrame *fp;
    jsint slot;

    JS_ASSERT(JS_InstanceOf(cx, obj, &js_BlockClass, NULL));
    if (!JSVAL_IS_INT(id))
        return JS_TRUE;

    fp = (JSStackFrame *) JS_GetPrivate(cx, obj);
    if (!fp)
        return JS_TRUE;
...
}

This results in a junk stored in obj->fslot[5].

Comment 7

[Igor Bukanov]

Here is the test case for the shell to show problem explicitly:

var f;

function gen(yield_at_least_once) {
    let x = 11;
    function y(){}
    f = function(){ return x; };
    if (yield_at_least_once)
        yield;
}

for (var i in gen()) { }

if (f() !== 11)
    throw "unexpected value of local x";

Currenly it throw the exception since the code incorrectly calls js_PutBlockObject when the generator returns right after initialization.

Comment 8

[Igor Bukanov]

Attachment: fix v1

The fix makes sure that [generator] is the first instruction to execute in generator's bytecode so any [deflocalfun] will be executed after it. Without the patch [deflocalfun] creates a scope chain that is closed when [generator] exits function.

The patch alters xdr version to reject older code with non-first [generator].

Comment 9

[Brendan Eich [:brendan]]

Would like block_getProperty to set *vp = JSVAL_VOID before any early true returns -- vp is an out parameter for getters so all successful return paths should set it. Thanks for taking this,

/be

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #9)
> Would like block_getProperty to set *vp = JSVAL_VOID before any early true
> returns -- vp is an out parameter for getters so all successful return paths
> should set it.

Wrong, I'm forgetting my own design. getter vp is in/out just like setter: for JSPROP_SHARED *vp is JSVAL_VOID on entry, for unshared slot-ful props it is the last got or last set value.

Bug is in js_PutBlockObject, which should set v = JSVAL_VOID before calling sprop->getter.

/be

Comment 11

[Igor Bukanov]

(In reply to comment #10)
> Bug is in js_PutBlockObject, which should set v = JSVAL_VOID before calling
> sprop->getter.

As the test case from comment 7 shows the reason for the bug is that js_PutBlockObject is called twice and setting v to void before calling the getter  does not help at all.

Comment 12

[Igor Bukanov]

[I accidentally clicked on resolved fixed the last time.]

Comment 13

[Igor Bukanov]

Attachment: fix v2

The new version fixes regression in source note generation revealed by running the test suite and inlines call to block_getProperty in js_PutBlockObject to get better assertion coverage.

And I should remember to run the test suite for regressions before asking for r+. It just takes few minutes with -L slow-n.tests.

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #11)
> (In reply to comment #10)
> > Bug is in js_PutBlockObject, which should set v = JSVAL_VOID before calling
> > sprop->getter.
> 
> As the test case from comment 7 shows the reason for the bug is that
> js_PutBlockObject is called twice and setting v to void before calling the
> getter  does not help at all.

Understood, but it still seems worth fixing js_PutBlockObject to (re-)initialize v before each getter call.

/be

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 272224 [details] [diff] [review]
fix v2

Or to fix js_PutBlockObject as you did -- thanks!

/be

Comment 16

[Igor Bukanov]

I checked in the patch from comment 13 to the trunk:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.267; previous revision: 3.266
done
Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.364; previous revision: 3.363
done
Checking in jsxdrapi.h;
/cvsroot/mozilla/js/src/jsxdrapi.h,v  <--  jsxdrapi.h
new revision: 1.33; previous revision: 1.32
done

Comment 17

[Igor Bukanov]

The bug exists on 1.8.1 branch as well. This is a critical bug as it allows to expose a value from uninitialized local C variable to a script,

Comment 18

[Bob Clary [:bc] (inactive)]

Attachment: js1_7/extensions/regress-387955-01.js

Comment 19

[Bob Clary [:bc] (inactive)]

Attachment: js1_7/extensions/regress-387955-02.js

Comment 20

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 linux/mac*/windows.

Comment 21

[Igor Bukanov]

Attachment: 1.8.1 version of v2

To back port the patch I had to merge manually the changes in js_EmitFunctionBytecode and alter the xdr version to a new unique value. As the diff below shows, the other merge conflicts comes from trailing blank differences:

~/m/1.8.1/mozilla/js/src/> diff /home/igor/s/gen_prolog.patch /home/igor/s/gen_prolog-1.8.patch
4,10c4,8
< retrieving revision 3.266
< diff -U7 -p -r3.266 jsemit.c
< --- jsemit.c	12 Jul 2007 07:48:50 -0000	3.266
< +++ jsemit.c	13 Jul 2007 20:08:58 -0000
< @@ -3183,16 +3183,20 @@ bad:
<      goto out;
<  }
---
> retrieving revision 3.128.2.72
> diff -U8 -p -r3.128.2.72 jsemit.c
> --- jsemit.c	20 Apr 2007 18:45:18 -0000	3.128.2.72
> +++ jsemit.c	24 Sep 2007 09:08:24 -0000
> @@ -3110,18 +3110,22 @@ bad:
14a13,15
>      if (!js_AllocTryNotes(cx, cg))
>          return JS_FALSE;
>  
29c30,32
< @@ -6778,15 +6782,15 @@ js_FinishTakingSrcNotes(JSContext *cx, J
---
>  js_EmitFunctionBody(JSContext *cx, JSCodeGenerator *cg, JSParseNode *body,
> @@ -6729,17 +6733,17 @@ js_FinishTakingSrcNotes(JSContext *cx, J
>           * Either no prolog srcnotes, or no line number change over prolog.
45a49
>              for (;;) {
49,53c53,58
< retrieving revision 3.363
< diff -U7 -p -r3.363 jsobj.c
< --- jsobj.c	12 Jul 2007 07:48:50 -0000	3.363
< +++ jsobj.c	13 Jul 2007 20:08:58 -0000
< @@ -859,15 +859,15 @@ js_obj_toSource(JSContext *cx, JSObject 
---
> retrieving revision 3.208.2.54
> diff -U8 -p -r3.208.2.54 jsobj.c
> --- jsobj.c	11 Jul 2007 01:28:28 -0000	3.208.2.54
> +++ jsobj.c	24 Sep 2007 09:08:25 -0000
> @@ -852,17 +852,17 @@ js_obj_toSource(JSContext *cx, JSObject 
>          idstr = js_ValueToString(cx, ID_TO_VALUE(id));
70,71c75,76
< @@ -1283,15 +1283,15 @@ obj_eval(JSContext *cx, JSObject *obj, u
<      JSBool ok;
---
>          if (prop) {
> @@ -1986,27 +1986,32 @@ js_CloneBlockObject(JSContext *cx, JSObj
73,87d77
<      fp = cx->fp;
<      caller = JS_GetScriptedCaller(cx, fp);
<      JS_ASSERT(!caller || caller->pc);
<      indirectCall = (caller && *caller->pc != JSOP_EVAL);
<  
< -    /* 
< +    /*
<       * Ban all indirect uses of eval (global.foo = eval; global.foo(...)) and
<       * calls that attempt to use a non-global object as the "with" object in
<       * the former indirect case.
<       */
<      scopeobj = OBJ_GET_PARENT(cx, obj);
<      if (indirectCall || scopeobj) {
<          uintN flags = scopeobj
< @@ -1966,25 +1966,30 @@ js_CloneBlockObject(JSContext *cx, JSObj
122,128d111
< @@ -2802,15 +2807,15 @@ js_AllocSlot(JSContext *cx, JSObject *ob
<      }
<  
<      if (map->freeslot >= STOBJ_NSLOTS(obj) &&
<          !ReallocSlots(cx, obj, map->freeslot + 1, JS_FALSE)) {
<          return JS_FALSE;
<      }
130,138d112
< -    /* ReallocSlots or js_FreeSlot should set the free slots to void. */   
< +    /* ReallocSlots or js_FreeSlot should set the free slots to void. */
<      JS_ASSERT(STOBJ_GET_SLOT(obj, map->freeslot) == JSVAL_VOID);
<      *slotp = map->freeslot++;
<      return JS_TRUE;
<  }
<  
<  void
<  js_FreeSlot(JSContext *cx, JSObject *obj, uint32 slot)
142,146c116,121
< retrieving revision 1.32
< diff -U7 -p -r1.32 jsxdrapi.h
< --- jsxdrapi.h	11 Jul 2007 09:25:45 -0000	1.32
< +++ jsxdrapi.h	13 Jul 2007 20:08:58 -0000
< @@ -197,15 +197,15 @@ JS_XDRFindClassById(JSXDRState *xdr, uin
---
> retrieving revision 1.14.58.11
> diff -U8 -p -r1.14.58.11 jsxdrapi.h
> --- jsxdrapi.h	13 Apr 2007 23:37:13 -0000	1.14.58.11
> +++ jsxdrapi.h	24 Sep 2007 09:08:25 -0000
> @@ -195,17 +195,17 @@ JS_XDRFindClassById(JSXDRState *xdr, uin
>   * Bytecode version number.  Decrement the second term whenever JS bytecode
154,155c129,130
< -#define JSXDR_BYTECODE_VERSION      (0xb973c0de - 14)
< +#define JSXDR_BYTECODE_VERSION      (0xb973c0de - 15)
---
> -#define JSXDR_BYTECODE_VERSION      (0xb973c0de - 10)
> +#define JSXDR_BYTECODE_VERSION      (0xb973c0de - 16)
162a138
>  extern JSBool
----- exited abnormally with code 1

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 282085 [details] [diff] [review]
1.8.1 version of v2

approved for 1.8.1.8, a=dveditz for release-drivers

The patches look nearly identical, might be fine w/out re-review by brendan.

Comment 23

[Igor Bukanov]

I checked in the patch from comment 21 to MOZILLA_1_8_BRANCH:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.128.2.73; previous revision: 3.128.2.72
done
Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.208.2.55; previous revision: 3.208.2.54
done
Checking in jsxdrapi.h;
/cvsroot/mozilla/js/src/jsxdrapi.h,v  <--  jsxdrapi.h
new revision: 1.14.58.12; previous revision: 1.14.58.11
done

Comment 24

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.1.8 win/mac*/linux

Comment 25

[Alexander Sack]

Igor, I assume this doesn't exist on 1.8.0 branch, right?

Comment 26

[Igor Bukanov]

(In reply to comment #25)
> I assume this doesn't exist on 1.8.0 branch, right?

Right, this is a bug in the generator support code that exists only on 1.8.1 and later. 

Comment 27

[Bob Clary [:bc] (inactive)]

Checking in regress-387955-01.js;
/cvsroot/mozilla/js/tests/js1_7/extensions/regress-387955-01.js,v  <--  regress-387955-01.js
initial revision: 1.1

Checking in regress-387955-02.js;
/cvsroot/mozilla/js/tests/js1_7/extensions/regress-387955-02.js,v  <--  regress-387955-02.js
initial revision: 1.1