Last Comment Bug 354151 - Bad assumptions about Array elements in jsxml.c
: Bad assumptions about Array elements in jsxml.c
Status: VERIFIED FIXED
[sg:critical?] gc hazard
: verified1.8.0.8, verified1.8.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 354145
  Show dependency treegraph
 
Reported: 2006-09-25 10:12 PDT by Igor Bukanov
Modified: 2006-11-10 11:37 PST (History)
4 users (show)
dveditz: blocking1.7.14-
dveditz: blocking‑aviary1.0.9-
mbeltzner: blocking1.8.1+
dveditz: blocking1.8.0.8+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix (10.57 KB, patch)
2006-09-26 05:31 PDT, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review
Fix v1b (10.57 KB, patch)
2006-09-26 12:04 PDT, Igor Bukanov
mbeltzner: approval1.8.1+
Details | Diff | Splinter Review
e4x/Regress/regress-354151-01.js (2.15 KB, text/plain)
2006-09-29 03:40 PDT, Bob Clary [:bc:]
no flags Details
e4x/Regress/regress-354151-02.js (2.24 KB, text/plain)
2006-09-29 03:41 PDT, Bob Clary [:bc:]
no flags Details
Fix v1b for 1.8.0 branch (16.25 KB, patch)
2006-10-09 15:12 PDT, Igor Bukanov
brendan: review+
dveditz: approval1.8.0.8+
mbeltzner: approval1.8.1-
Details | Diff | Splinter Review
e4x/Regress/regress-354151-01.js (2.18 KB, text/plain)
2006-10-31 17:10 PST, Bob Clary [:bc:]
no flags Details
e4x/Regress/regress-354151-02.js (2.27 KB, text/plain)
2006-10-31 17:11 PST, Bob Clary [:bc:]
no flags Details

Description Igor Bukanov 2006-09-25 10:12:35 PDT
Consider the following example:

function getter() { return 1; }
function setter(v) { return v; }

var xml = <a xmlns="foo:bar"/>;

Array.prototype.__defineGetter__(0, getter);
Array.prototype.__defineSetter__(0, setter);

xml.namespace();

Currently it crashes because xml_inScopeNamespaces assumes that Array instance may only contain array elements it explicitly put there. As this example demonstrates, this is not the case since script can define a getter on Array.prototype.
Comment 1 Igor Bukanov 2006-09-25 16:46:47 PDT
Here is a pure GC-hazard in xml_namespaceDeclarations where the setter for the first element unroots the second namespace stored in local variables and accessed later:

var xml = <tag xmlns:n="uri:1" xmlns:n2="uri:2" n:a="1" n2:a="1"/>;

function getter() { }

function setter(v)
{
  delete xml.@*::a;
  xml.removeNamespace("uri:2");
  gc();
}

Array.prototype.__defineGetter__(0, getter);
Array.prototype.__defineSetter__(0, setter);

xml.namespaceDeclarations();
~/m/trunk/mozilla/js/src> ./Linux_All_DBG.OBJ/js ~/m/files/y.js
before 9232, after 9232, break 08141000
Segmentation fault
Comment 2 Igor Bukanov 2006-09-26 05:31:44 PDT
Created attachment 240137 [details] [diff] [review]
Fix

The patch changes XML not to use JS Array instances for temporaries. Instead it uses a rooted wrapper around XMLArray to store namespaces temporary. Note that the patch roots more then strictly necessary, but I prefer this bit of paranoia hunting new bugs in future.
Comment 3 Igor Bukanov 2006-09-26 06:51:18 PDT
I am changing the title to reflect more generic nature of the bug as it exists not only in jsxml.c. Consider the following:



 ~/m/trunk/mozilla/js/src> cat ~/m/files/y.js 
var obj = {get a(){ return new Object(); }};

function setter(v)
{
  // Push out obj.a from all temp roots
  var tmp = { get toString() { return new Object(); }};
  try { String(tmp); } catch (e) {  }
  gc();
}

Array.prototype.__defineGetter__(0, function() { });
Array.prototype.__defineSetter__(0, setter);

for (var i in Iterator(obj))
  print(uneval(i));

~/m/trunk/mozilla/js/src> ./Linux_All_DBG.OBJ/js ~/m/files/y.js
Key value
before 9232, after 9232, break 08142000
Segmentation fault

The crash happens because NewKeyValuePair in jsiter.c gets just weakly rooted value as its val argument and passes it to js_NewArrayObject as the second element of the local array. So setter for the first can unroot and collect it.

Note that the bug would not exist if jsinterp.c would pass a pointer to a rooted location to js_CallIteratorNext as rval and I wish a year ago all those local "jsval v" would be banned from sources.
Comment 4 Brendan Eich [:brendan] 2006-09-26 11:20:27 PDT
Comment on attachment 240137 [details] [diff] [review]
Fix

>+    /* After thos point the control must flow through label out. */

s/thos/this/

>     }

Nicer with blank line here?

>+    if (!ns) {
>+        *rval = JSVAL_VOID;
>+    } else {
>+        nsobj = js_GetXMLNamespaceObject(cx, ns);
>+        if (!nsobj) {
>+            ok = JS_FALSE;
>+            goto out;
>+        }
>+        *rval = OBJECT_TO_JSVAL(ns->object);

Use nsobj instead of ns->object.

>+    /* Finishing must be in reveres order of initialization to follow LIFO. */

s/reveres/reverse/

Nice cleanup, separates XMLArray to JS Array from E4X guts, probably for net code size savings -- true?

Nominate for 1.8.1 as soon as this lands -- there's a chance to get this into rc2.

/be
Comment 5 Brendan Eich [:brendan] 2006-09-26 11:26:47 PDT
(In reply to comment #3)
> Note that the bug would not exist if jsinterp.c would pass a pointer to a
> rooted location to js_CallIteratorNext as rval and I wish a year ago all those
> local "jsval v" would be banned from sources.

Me too, but they're too easy to type.  There is precedent for passing a vp that points to the operand stack, instead of v or &v.  That's still not enforced by the type system, but better.  But thanks to you we have other bugs on mandatory explicit rooting or better implicit rooting.

Want me to patch jsiter.c, or are you on it?  Thanks,

/be
Comment 6 Igor Bukanov 2006-09-26 11:58:24 PDT
(In reply to comment #5)
> Want me to patch jsiter.c, or are you on it?

I would like to change jsinter.c to pass a pointer to a rooted location to   js_CallIteratorNext, but I do not know where to get one. Any hints?
Comment 7 Igor Bukanov 2006-09-26 12:04:03 PDT
Created attachment 240186 [details] [diff] [review]
Fix v1b

Patch to commit with nits addressed.
Comment 8 Brendan Eich [:brendan] 2006-09-26 12:13:26 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > Want me to patch jsiter.c, or are you on it?
> 
> I would like to change jsinter.c to pass a pointer to a rooted location to  
> js_CallIteratorNext, but I do not know where to get one. Any hints?

The operand stack will have room for the true or false value ultimately pushed at end_forinloop:, so you could use that as a scanned local root, provided fp->sp is set to a greater address to protect it during calls out of the interpreter code.

The SAVE_SP_AND_PC(fp) done just before the "Is this the first iteration?" comment soon after do_forinloop: would want to pre-increment sp, and the one use of sp that I see after this point and before end_forinloop would want to change from sp[i-depth] to sp[i-depth-1] of course.

The sp += i + 1 done first thing at end_forinloop: would need to change to += i, and PUSH_OPND(rval) on the next line would be STORE_OPND(-1, rval).

JSOP_FORELEM pushes the currently enumerated string id's value before control flow reaches end_forinloop, a fly in the ointment.  It could claim the protected stack slot, and adjust sp or i so that the revised STORE_OPND at end_forinloop works as before, leaving the stack two deeper for this opcode (so from initial state

  [current-obj-on-proto-chain, iterobj]

JSOP_FORELEM leaves the stack looking like

  [current-obj-on-proto-chain, iterobj, id-as-string, true-or-false]

thus its nuses of 2 and ndefs of 4 in jsopcode.tbl).

/be
Comment 9 Brendan Eich [:brendan] 2006-09-26 12:31:46 PDT
(In reply to comment #8)
> JSOP_FORELEM pushes the currently enumerated string id's value before control

Super-clarification: s/string id's value/id's string value/.  And of course with the iteration protocol, rval could be any value -- it's just the next value in the iteration.

/be
Comment 10 Igor Bukanov 2006-09-26 13:56:29 PDT
(In reply to comment #4)
> Nice cleanup, separates XMLArray to JS Array from E4X guts, probably for net
> code size savings -- true?

jsxml.c compiled with GCC 4.0 and -Os:
before the patch: 46220 bytes
after the patch:  46156 bytes

So the patch in fact saved the space indeed. But that was not the goal ;) 

> Nominate for 1.8.1 as soon as this lands -- there's a chance to get this into
> rc2.

Well, I would bet 500 USD that if this waits until FF 2.0.1, no harm would be done. yes, there is no regressions with e4x tests, but still it is not a one-line fix. 
Comment 11 Igor Bukanov 2006-09-26 14:01:58 PDT
I committed the patch from comment 7 to the trunk:

Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.127; previous revision: 3.126
done

I keep the bug as assigned since the patch fixes only the tests from comment 0 and comment 1, as the problem from comment 3 is still not addressed.
Comment 12 Igor Bukanov 2006-09-27 07:36:42 PDT
Rooting rval is not enough since there is another pointer to unrooted location that is passed to js_CallIteratorNext. That is, idp pointer. Eventually this pointer will be passed to CheckKeyValueReturn. There either through overwriting Iterator.prototype.next or when !JS_HAS_LVALUE_RETURN, the code will execute:

        if (!OBJ_GET_PROPERTY(cx, obj, INT_TO_JSID(0), &idval))
            return JS_FALSE;
        if (!JS_ValueToId(cx, idval, idp))
            return JS_FALSE;
        if (flags & JSITER_KEYVALUE)
            return JS_TRUE;
        return OBJ_GET_PROPERTY(cx, obj, INT_TO_JSID(1), rval);

If obj array here defines a getter for the second element, it can unroot and GC the first. Then the interpreter access the unrooted value. 

Again, this can be addressed in the iter.c, but that is sweeping dust under the carpet since the real problem is that idp points to unrooted location. As far as I can see there is already an extra slot besides the slot for the final true/false/what ever. That is, rval returned from js_CallIteratorNext after some processing ends up in various  fp->argv[slot] or stack locations so why not use that for rval in the first place? Then the stack top can be used for rooting of fid. Of cause, that would require to change js_CallIteratorNext to accept jsval*, not jsid*, but that is minor. 
Comment 13 Igor Bukanov 2006-09-27 07:38:12 PDT
Example exposing the problem from comment 12 (so there 4 such problems in total):

~/m/trunk/mozilla/js/src> cat ~/m/files/y.js
function get_value()
{
  // Unroot the first element
  this[0] = 0;

  // Call gc to collect atom corresponding to Math.sqrt(2)
  gc();
}

var counter = 2;
Iterator.prototype.next = function()
{
  if (counter-- <= 0) throw StopIteration;
  var a = [Math.sqrt(2), 1];
  a.__defineGetter__(1, get_value);
  return a; 
};

for (i in [1]);

~/m/trunk/mozilla/js/src> js ~/m/files/y.js 
before 9232, after 9232, break 08180000
Segmentation fault
Comment 14 Igor Bukanov 2006-09-27 07:46:15 PDT
I changing the title again to be specific for jsxml.c and will file a separated bug for jsiter.c since jsiter.c problems applies unly to 1.8.1 and later while jsxml.c bug also exists in 1.8.0.
Comment 15 Igor Bukanov 2006-09-27 08:01:34 PDT
I filed 354499 bug for iterating problems and copied the examples from comment 3 and comment 13 there. As such the bug is fixed now on the trunk since the fix for test cases from comment 0 and comment is 1 already committed.
Comment 16 Igor Bukanov 2006-09-27 08:02:43 PDT
(In reply to comment #15)
> I filed 354499 bug

I meant bug 354499 ;)
Comment 17 Bob Clary [:bc:] 2006-09-29 03:40:55 PDT
Created attachment 240596 [details]
e4x/Regress/regress-354151-01.js
Comment 18 Bob Clary [:bc:] 2006-09-29 03:41:29 PDT
Created attachment 240597 [details]
e4x/Regress/regress-354151-02.js
Comment 19 Bob Clary [:bc:] 2006-10-02 23:23:27 PDT
verified fixed 1.9 20061002 windows/linux no crash on either test. Note that the spider-based harness failed to report but there was _no crash_. Will investigate.
Comment 20 Igor Bukanov 2006-10-08 16:02:28 PDT
Comment on attachment 240186 [details] [diff] [review]
Fix v1b

This is a GC hazard fix
Comment 21 Mike Schroepfer 2006-10-09 05:33:31 PDT
Brendan - do we need for 181?
Comment 22 Brendan Eich [:brendan] 2006-10-09 09:26:15 PDT
Yes, sorry I missed this -- we should take it.  It's a well-baked GC safety fix that is needed for the immutability-assumption-violation bug 354145 fix.

We want the other dependency of 354145 too.

/be
Comment 23 Daniel Veditz [:dveditz] 2006-10-09 10:55:54 PDT
Comment on attachment 240186 [details] [diff] [review]
Fix v1b

we'll approve for 1.8.0.8 if this gets approved for rc3
Comment 24 Igor Bukanov 2006-10-09 15:07:09 PDT
Comment on attachment 240186 [details] [diff] [review]
Fix v1b

The patch does not apply on 1.8.0 branch
Comment 25 Igor Bukanov 2006-10-09 15:12:50 PDT
Created attachment 241757 [details] [diff] [review]
Fix v1b for 1.8.0 branch

The patch for trunk/1.8.1 does not apply to 1.8.0 for the following reasons:

1. 1.8.0 does not have JS_PUSH_TEMP_ROOT_MARKER macro so the patch copies that code from 1.8.0

2. GC_MARK and namespace_mark_vector on the branch takes an extra parameter to support GC_MARK_DEBUG. I fixed that via passing NULL there.

3. The branch does not have js_EqualStrings() so I replaced that by calls to js_CompareString.
Comment 26 Brendan Eich [:brendan] 2006-10-09 17:25:40 PDT
Comment on attachment 241757 [details] [diff] [review]
Fix v1b for 1.8.0 branch

I diff'ed the diffs to verify -- looks good, this should go in for rc3.

/be
Comment 27 Mike Beltzner [:beltzner, not reading bugmail] 2006-10-10 10:14:05 PDT
Comment on attachment 241757 [details] [diff] [review]
Fix v1b for 1.8.0 branch

a=beltzner on behalf of drivers for 1.8.1 RC 3
Comment 28 Mike Beltzner [:beltzner, not reading bugmail] 2006-10-10 10:22:58 PDT
Comment on attachment 241757 [details] [diff] [review]
Fix v1b for 1.8.0 branch

Oops, assuming this is the 1.8.0 branch fix, not the 1.8 branch fix, so minusing. I'll plus the other one.
Comment 29 Mike Beltzner [:beltzner, not reading bugmail] 2006-10-10 10:23:58 PDT
Comment on attachment 240186 [details] [diff] [review]
Fix v1b

a=beltzner on behalf of drivers for 1.8.1 RC 3
Comment 30 Igor Bukanov 2006-10-10 10:36:52 PDT
I committed the patch from comment 7, Fix v1b, to MOZILLA_1_8_BRANCH: 

Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.50.2.51; previous revision: 3.50.2.50
done
Comment 31 Daniel Veditz [:dveditz] 2006-10-10 15:54:17 PDT
Comment on attachment 241757 [details] [diff] [review]
Fix v1b for 1.8.0 branch

approved for 1.8.0 branch, a=dveditz
Comment 32 Igor Bukanov 2006-10-10 16:11:01 PDT
I committed the patch from comment 25 to MOZILLA_1_8_0_BRANCH:

Checking in jscntxt.h;
/cvsroot/mozilla/js/src/jscntxt.h,v  <--  jscntxt.h
new revision: 3.80.4.2.2.6; previous revision: 3.80.4.2.2.5
done
Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.104.2.3.2.11; previous revision: 3.104.2.3.2.10
done
Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.50.2.15.2.21; previous revision: 3.50.2.15.2.20
done
Comment 33 Bob Clary [:bc:] 2006-10-11 02:30:09 PDT
verified fixed 1.8 20061010 windows/mac*/linux
Comment 34 alice nodelman [:alice] [:anode] 2006-10-12 11:29:27 PDT
verified fixed 1.8.0.8 win/mac*/linux
firefox_1.8.0.8pre_2006101114_dbg
firefox_1.8.0.8pre_2006101114_opt
Comment 35 Bob Clary [:bc:] 2006-10-31 17:10:57 PST
Created attachment 244266 [details]
e4x/Regress/regress-354151-01.js

reset Array.prototype[0]
Comment 36 Bob Clary [:bc:] 2006-10-31 17:11:56 PST
Created attachment 244267 [details]
e4x/Regress/regress-354151-02.js

reset Array.prototype[0]
Comment 37 Daniel Veditz [:dveditz] 2006-11-01 17:42:34 PST
Bug not relevant to aviary/moz1.7 branches
Comment 38 Bob Clary [:bc:] 2006-11-10 11:37:12 PST
RCS file: /cvsroot/mozilla/js/tests/e4x/Regress/regress-354151-01.js,v
done
Checking in regress-354151-01.js;
/cvsroot/mozilla/js/tests/e4x/Regress/regress-354151-01.js,v  <--  regress-354151-01.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/e4x/Regress/regress-354151-02.js,v
done
Checking in regress-354151-02.js;
/cvsroot/mozilla/js/tests/e4x/Regress/regress-354151-02.js,v  <--  regress-354151-02.js
initial revision: 1.1
done

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