Status

()

defect
--
critical
VERIFIED FIXED
12 years ago
8 years ago

People

(Reporter: jruderman, Assigned: igor)

Tracking

(Blocks 1 bug, {crash, testcase, verified1.8.1.8})

Trunk
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.8 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?], crash signature)

Attachments

(6 attachments, 2 obsolete attachments)

Reporter

Description

12 years ago
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();
Reporter

Updated

12 years ago
Whiteboard: [sg:critical?]

Comment 1

12 years ago
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.
Reporter

Comment 2

12 years ago
mrbkap was able to reproduce using Valgrind and a debug build.
OS: Mac OS X → All
Posted file 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.
Assignee

Updated

12 years ago
Assignee: general → igor
Assignee

Comment 4

12 years ago
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.
Assignee

Comment 5

12 years ago
The last output had wrong ordering of lines
Attachment #272144 - Attachment is obsolete: true
Assignee

Comment 6

12 years ago
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].
Assignee

Comment 7

12 years ago
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.
Assignee

Comment 8

12 years ago
Posted patch fix v1 (obsolete) — Splinter Review
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].
Attachment #272181 - Flags: review?(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
(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
Assignee

Comment 11

12 years ago
(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.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee

Comment 12

12 years ago
[I accidentally clicked on resolved fixed the last time.]
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 13

12 years ago
Posted patch fix v2Splinter Review
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.
Attachment #272181 - Attachment is obsolete: true
Attachment #272224 - Flags: review?(brendan)
Attachment #272181 - Flags: review?(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 on attachment 272224 [details] [diff] [review]
fix v2

Or to fix js_PutBlockObject as you did -- thanks!

/be
Attachment #272224 - Flags: review?(brendan) → review+
Assignee

Comment 16

12 years ago
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
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Assignee

Comment 17

12 years ago
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,
Flags: blocking1.8.1.6?

Updated

12 years ago
Flags: in-testsuite+
Flags: blocking1.8.1.7? → blocking1.8.1.7+

Comment 20

12 years ago
verified fixed 1.9.0 linux/mac*/windows.
Status: RESOLVED → VERIFIED
Assignee

Comment 21

12 years ago
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
Attachment #282085 - Flags: review?(brendan)
Attachment #282085 - Flags: approval1.8.1.8?
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.
Attachment #282085 - Flags: approval1.8.1.8? → approval1.8.1.8+
Attachment #282085 - Flags: review?(brendan) → review+
Assignee

Comment 23

12 years ago
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
Keywords: fixed1.8.1.8

Comment 24

12 years ago
verified fixed 1.8.1.8 win/mac*/linux

Comment 25

12 years ago
Igor, I assume this doesn't exist on 1.8.0 branch, right?
Assignee

Comment 26

12 years ago
(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. 

Group: security

Comment 27

12 years ago
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

Updated

12 years ago
Duplicate of this bug: 383493
Crash Signature: [@ JS_CallTracer]
You need to log in before you can comment on or make changes to this bug.