Last Comment Bug 387955 - Crash [@ JS_CallTracer]
: Crash [@ JS_CallTracer]
Status: VERIFIED FIXED
[sg:critical?]
: crash, testcase, verified1.8.1.8
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 All
: -- critical (vote)
: ---
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
: 383493 (view as bug list)
Depends on:
Blocks: jsfunfuzz
  Show dependency treegraph
 
Reported: 2007-07-12 16:35 PDT by Jesse Ruderman
Modified: 2011-06-13 10:01 PDT (History)
8 users (show)
dveditz: blocking1.8.1.8+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
valgrind output (10.42 KB, text/plain)
2007-07-12 17:59 PDT, Blake Kaplan (:mrbkap)
no flags Details
valgrind output for more explicit test (9.37 KB, text/plain)
2007-07-13 01:59 PDT, Igor Bukanov
no flags Details
valgrind output for more explicit test (9.37 KB, text/plain)
2007-07-13 02:07 PDT, Igor Bukanov
no flags Details
fix v1 (1.69 KB, patch)
2007-07-13 08:19 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
fix v2 (5.84 KB, patch)
2007-07-13 13:12 PDT, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review
js1_7/extensions/regress-387955-01.js (2.49 KB, text/plain)
2007-08-05 19:42 PDT, Bob Clary [:bc:]
no flags Details
js1_7/extensions/regress-387955-02.js (2.59 KB, text/plain)
2007-08-05 19:43 PDT, Bob Clary [:bc:]
no flags Details
1.8.1 version of v2 (5.18 KB, patch)
2007-09-24 02:17 PDT, Igor Bukanov
brendan: review+
dveditz: approval1.8.1.8+
Details | Diff | Splinter Review

Description Jesse Ruderman 2007-07-12 16:35:37 PDT
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 2007-07-12 16:54:35 PDT
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 2007-07-12 17:50:34 PDT
mrbkap was able to reproduce using Valgrind and a debug build.
Comment 3 Blake Kaplan (:mrbkap) 2007-07-12 17:59:50 PDT
Created attachment 272098 [details]
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 2007-07-13 01:59:08 PDT
Created attachment 272144 [details]
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 2007-07-13 02:07:17 PDT
Created attachment 272145 [details]
valgrind output for more explicit test

The last output had wrong ordering of lines
Comment 6 Igor Bukanov 2007-07-13 04:33:17 PDT
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 2007-07-13 07:29:00 PDT
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 2007-07-13 08:19:02 PDT
Created attachment 272181 [details] [diff] [review]
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] 2007-07-13 10:22:42 PDT
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] 2007-07-13 10:46:59 PDT
(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 2007-07-13 13:06:55 PDT
(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 2007-07-13 13:08:18 PDT
[I accidentally clicked on resolved fixed the last time.]
Comment 13 Igor Bukanov 2007-07-13 13:12:16 PDT
Created attachment 272224 [details] [diff] [review]
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] 2007-07-13 14:03:22 PDT
(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] 2007-07-13 14:05:32 PDT
Comment on attachment 272224 [details] [diff] [review]
fix v2

Or to fix js_PutBlockObject as you did -- thanks!

/be
Comment 16 Igor Bukanov 2007-07-13 14:24:40 PDT
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 2007-07-13 14:29:19 PDT
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:] 2007-08-05 19:42:03 PDT
Created attachment 275377 [details]
js1_7/extensions/regress-387955-01.js
Comment 19 Bob Clary [:bc:] 2007-08-05 19:43:03 PDT
Created attachment 275378 [details]
js1_7/extensions/regress-387955-02.js
Comment 20 Bob Clary [:bc:] 2007-09-18 15:51:00 PDT
verified fixed 1.9.0 linux/mac*/windows.
Comment 21 Igor Bukanov 2007-09-24 02:17:21 PDT
Created attachment 282085 [details] [diff] [review]
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] 2007-09-24 12:19:55 PDT
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 2007-09-27 11:26:34 PDT
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:] 2007-10-05 04:12:39 PDT
verified fixed 1.8.1.8 win/mac*/linux
Comment 25 Alexander Sack 2007-10-09 10:11:10 PDT
Igor, I assume this doesn't exist on 1.8.0 branch, right?
Comment 26 Igor Bukanov 2007-10-09 13:07:44 PDT
(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:] 2007-10-18 19:34:31 PDT
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
Comment 28 timeless 2007-11-07 08:08:52 PST
*** Bug 383493 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.