Last Comment Bug 476049 - JSOP_DEFVAR enables gvar optimization for non-permanent properties
: JSOP_DEFVAR enables gvar optimization for non-permanent properties
Status: VERIFIED FIXED
[sg:critical] fixed-in-tracemonkey
: fixed-seamonkey1.1.16, fixed1.8.1.22, verified1.9.0.9, verified1.9.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P1 critical (vote)
: ---
Assigned To: Igor Bukanov
:
Mentors:
Depends on:
Blocks: 475971
  Show dependency treegraph
 
Reported: 2009-01-29 15:09 PST by Igor Bukanov
Modified: 2009-08-07 16:50 PDT (History)
11 users (show)
sayrer: blocking1.9.1+
dveditz: blocking1.9.0.9+
dveditz: wanted1.9.0.x+
samuel.sidler+old: blocking1.8.1.next+
samuel.sidler+old: wanted1.8.1.x+
asac: blocking1.8.0.next+
bob: in‑testsuite+
bob: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (10.48 KB, patch)
2009-01-30 08:59 PST, Igor Bukanov
no flags Details | Diff | Splinter Review
v2 (10.85 KB, patch)
2009-01-30 13:37 PST, Igor Bukanov
no flags Details | Diff | Splinter Review
v3 (10.83 KB, patch)
2009-01-30 14:02 PST, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review
v4 (7.06 KB, patch)
2009-01-31 03:48 PST, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review
v5 (7.07 KB, patch)
2009-02-02 11:30 PST, Igor Bukanov
igor: review+
Details | Diff | Splinter Review
fix for 1.9.0 (7.05 KB, patch)
2009-02-12 10:00 PST, Igor Bukanov
dveditz: approval1.9.0.9+
Details | Diff | Splinter Review
plain diff between trunk and 1.9.0 patches (3.01 KB, text/plain)
2009-02-12 10:02 PST, Igor Bukanov
no flags Details
js1_5/Regress/regress-476049.js (browser only) (2.66 KB, text/plain)
2009-02-18 11:10 PST, Bob Clary [:bc:]
no flags Details
minimal fix for 181 (1.48 KB, patch)
2009-04-02 07:50 PDT, Igor Bukanov
dveditz: approval1.8.1.next+
asac: approval1.8.0.next+
Details | Diff | Splinter Review

Description Igor Bukanov 2009-01-29 15:09:36 PST
gvar optimization in the js_Interpret can only be applied to permanent properties as it assumes that properties's slot stays the same. But JSOP_DEFVAR fails to enforce this when it defines a variable that already exists. JSOP_DEFVAR will enable the optimization even if such existing property does not have the JSPROP_PERMANENT attribute and can be deleted. The following example demonstrates the problem:

@watson~/m/tm/js/src> cat ~/s/y.js
for (var i = 0; i != 1000; ++i)
    this["a"+i] = 0;
eval("var x");
for (var i = 0; i != 1000; ++i)
    delete this["a"+i];

@watson~/m/tm/js/src> cat ~/s/x.js
var x;
eval("delete x;");
x={};
@watson~/m/tm/js/src> ~/build/js64.tm.dbg/js -f ~/s/y.js -f ~/s/x.js
Assertion failure: slot < (obj)->map->freeslot, at /home/igor/m/tm/js/src/jsinterp.cpp:5623
Aborted

Note that the example uses 2 file to ensure that the eval that define variable in the first file will be executed before JSOP_DEFVAR for the second. In a browser this can be modeled with 2 <script> tags.

The bug is critical as it allows to read and write arbitrary data passed heap-allocated array.
Comment 1 Igor Bukanov 2009-01-30 08:59:20 PST
Created attachment 359768 [details] [diff] [review]
v1

The essence of the fix is to use the attributes of the existing property when deciding about gvar optimization. It is non-minimal since it also addresses the bug 475971. For that the bug changes js_CheckRedeclaration to always unlock the object returning the info about its properties through the parameters.
Comment 2 Brendan Eich [:brendan] 2009-01-30 12:53:10 PST
Comment on attachment 359768 [details] [diff] [review]
v1

>+          BEGIN_CASE(JSOP_DEFVAR) {
>+            uint32 attrs, gvarSlot, *useAsGVar;

Slight pref throughout for gslot and gslotp as the more terse names.

>+            if (!exists) {
>                 if (!OBJ_DEFINE_PROPERTY(cx, obj, id, JSVAL_VOID,
>                                          JS_PropertyStub, JS_PropertyStub,
>                                          attrs, &prop)) {
>                     goto error;
>                 }
>                 JS_ASSERT(prop);
>+                if (useAsGVar) {
>+                    JS_ASSERT(*useAsGVar == gvarSlot);
>+                    gvarSlot = js_CanOptimizeAsGVar(obj, prop);
>                 }
>             }
> 
>-            OBJ_DROP_PROPERTY(cx, obj2, prop);

Need the OBJ_DROP_PROPERTY call still, just above in the if (!exists) {...} then clause.

/be
Comment 3 Igor Bukanov 2009-01-30 13:00:51 PST
(In reply to comment #2)
> (From update of attachment 359768 [details] [diff] [review])
> Need the OBJ_DROP_PROPERTY call still, just above in the if (!exists) {...}
> then clause.


Yep - how could I forgot about it?
Comment 4 Igor Bukanov 2009-01-30 13:37:06 PST
Created attachment 359819 [details] [diff] [review]
v2

The new patch adds the missing OBJ_DROP_PROPERTY and replaces long-but-still-unclear names for g-locals by short names and comments.
Comment 5 Igor Bukanov 2009-01-30 13:40:14 PST
Comment on attachment 359819 [details] [diff] [review]
v2

the patch has a bug
Comment 6 Igor Bukanov 2009-01-30 14:02:35 PST
Created attachment 359825 [details] [diff] [review]
v3

In the previous patch I was using the permanent attribute only for eval scripts. Too bad our test suite could not catch this regression, so here is the test that checks it:

var x = 1;
delete x;   
if (x !== 1)
    throw "unexpected";

eval('var y = 1;');
delete y;
if ('y' in this)
    throw "unexpected";
Comment 7 Brendan Eich [:brendan] 2009-01-30 15:31:36 PST
Comment on attachment 359825 [details] [diff] [review]
v3

>+js_CanOptimizeAsGVar(JSObject *obj, JSProperty *prop)
>+{
>+    if (OBJ_IS_NATIVE(obj)) {
>+        JSScopeProperty *sprop = (JSScopeProperty *) prop;

Nit: blank line here helps readability.

>+     * If the caller wants to know the slot for gvar optimization, it must also
>+     * want to know if the property exists. We also require that *existp and

Nit: s/know if/know whether/

>+            /*
>+             * gslotp is &gslot if we want gvar optimization, gslot changes

Maybe s/,/;/ above, or s/,/ and/

>+             * from SPROP_INVALID_SLOT to an index into obj's slots array when
>+             * we can do the optimization.
>+             */

Looks great otherwise -- r=me with nits picked. Thanks,

/be
Comment 8 Igor Bukanov 2009-01-31 03:48:55 PST
Created attachment 359896 [details] [diff] [review]
v4

The previous patch was over engineered and would require significant changes to fix all the issues from the bug 467495. So here is a simple fix just to fix this bug and bug 475971. Sorry about wasting review time.

I also changed the comment in js_CheckRedeclaration about the getter/setter checks. My reading of the current text made me to suspect that this bug can also be triggered via replacement of a permanent gvar by impermanent getter/setter under the eval. But checking the actual code revealed that this is not the case. Hopefully the new comment makes this clear. At worst a property with a permanent getter can be replaced with non-permanent one that has both getter and setter (is it a bug?), but this does not affect gvar optimizations.
Comment 9 Brendan Eich [:brendan] 2009-01-31 18:40:24 PST
Comment on attachment 359896 [details] [diff] [review]
v4

(In reply to comment #8)
> Created an attachment (id=359896) [details]
> v4
> 
> The previous patch was over engineered and would require significant changes to
> fix all the issues from the bug 467495. So here is a simple fix just to fix
> this bug and bug 475971. Sorry about wasting review time.

No problem; I noted the dependency.

> I also changed the comment in js_CheckRedeclaration about the getter/setter
> checks. My reading of the current text made me to suspect that this bug can
> also be triggered via replacement of a permanent gvar by impermanent
> getter/setter under the eval. But checking the actual code revealed that this
> is not the case.

Right, only getter on top of setter or vice versa, not getter or setter on top of permanent data property.

> Hopefully the new comment makes this clear. At worst a
> property with a permanent getter can be replaced with non-permanent one that
> has both getter and setter (is it a bug?),

Yes, this seems like a bug. Please file separately or fix here.

> but this does not affect gvar optimizations.

Right.

>+        if (prop) {
>+            OBJ_DROP_PROPERTY(cx, obj2, prop);
>+#ifdef DEBUG
>+            prop = NULL;
>+#endif
>+        }
> 
>         report = JSREPORT_ERROR;

The prop dropping and ifdef-DEBUG nulling requires the report = JSREPORT_ERROR; to force a false return. Comment this at least, and consider clumping the above lines together (with a blank line before -- due to the leading major comment -- and after) or even reordering. r=me with that.

/be
Comment 10 Igor Bukanov 2009-02-02 11:30:49 PST
Created attachment 360125 [details] [diff] [review]
v5

New version addresses the comment 19.
Comment 11 Igor Bukanov 2009-02-02 12:12:48 PST
landed in tm - http://hg.mozilla.org/tracemonkey/rev/30287116aafa
Comment 12 Damon Sicore (:damons) 2009-02-03 11:39:09 PST
P1.
Comment 13 Igor Bukanov 2009-02-03 16:43:30 PST
This goes back to the pool.
Comment 14 Andreas Gal :gal 2009-02-03 16:55:25 PST
Isn't this fixed and ready to be closed?
Comment 16 Samuel Sidler (old account; do not CC) 2009-02-11 15:33:21 PST
Igor: Can you work up a 1.9.0 patch for this?
Comment 17 Igor Bukanov 2009-02-12 09:57:06 PST
Here is an updated example that explicitly demonstrates the problem both with unpatched 1.9.1 and 1.9.0. As with the example from the comment 0, it uses 2 files.

@watson~/m/30-ff/mozilla/js/src> cat ~/s/y1.js
for (var i = 0; i != 1000; ++i)
    this["a"+i] = 0;
eval("var x;");
for (var i = 0; i != 1000; ++i)
    delete this["a"+i];
@watson~/m/30-ff/mozilla/js/src> cat ~/s/y2.js
var x;
for (i = 0; i != 10; ++i) x += i;
eval("delete x;");
x={};
@watson~/m/30-ff/mozilla/js/src> ./Linux_All_DBG.OBJ/js -f ~/s/y1.js -f ~/s/y2.js
Assertion failure: slot < (obj)->map->freeslot, at jsinterp.c:5515
Aborted
@
Comment 18 Igor Bukanov 2009-02-12 10:00:17 PST
Created attachment 362016 [details] [diff] [review]
fix for 1.9.0

For 1.9.0 the patch required a trivial backport.
Comment 19 Igor Bukanov 2009-02-12 10:02:06 PST
Created attachment 362017 [details]
plain diff between trunk and 1.9.0 patches

Here is a proof of triviality of 1.9.0 backporting efforts.
Comment 20 Bob Clary [:bc:] 2009-02-18 11:10:10 PST
Created attachment 362936 [details]
js1_5/Regress/regress-476049.js (browser only)
Comment 21 Daniel Veditz [:dveditz] 2009-02-20 11:46:14 PST
Comment on attachment 362016 [details] [diff] [review]
fix for 1.9.0

Approved for 1.9.0.8, a=dveditz for release-drivers
Comment 22 Bob Clary [:bc:] 2009-02-27 05:11:40 PST
Igor, could you land this on 1.9.0?
Comment 23 Igor Bukanov 2009-02-27 05:27:16 PST
landed to 1.9.0 branch:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.504; previous revision: 3.503
done
Comment 24 Samuel Sidler (old account; do not CC) 2009-02-27 11:36:45 PST
I'm pretty sure this checkin caused mochitest hangs on the 1.9.0 branch for both fx-win32-1.9-slave09 and fx-win32-1.9-slave10.

Igor: Any ideas?
Comment 25 Igor Bukanov 2009-02-27 12:05:09 PST
(In reply to comment #24)
> Igor: Any ideas?

No at this moment.
Comment 27 Bob Clary [:bc:] 2009-03-05 11:14:13 PST
v 1.9.1, 1.9.2
Comment 28 Bob Clary [:bc:] 2009-03-23 14:10:10 PDT
v 1.9.0
Comment 29 Samuel Sidler (old account; do not CC) 2009-04-01 16:18:54 PDT
Igor: Is this something we want on the 1.8 branch?
Comment 30 Igor Bukanov 2009-04-01 17:14:13 PDT
(In reply to comment #29)
> Igor: Is this something we want on the 1.8 branch?

Yes - the same bug exists on the branch as well. But note for truly minimal fix it is sufficient to port just the last hunk of the patch from the comment 18 and change code starting from http://mxr.mozilla.org/mozilla1.8/source/js/src/jsinterp.c#4723 .

It would not give proper error reporting for the corner cases, but it would fix this bug.
Comment 31 Samuel Sidler (old account; do not CC) 2009-04-01 17:30:54 PDT
Thanks Igor. Any chance you can work up a minimal patch?
Comment 32 Igor Bukanov 2009-04-02 07:50:00 PDT
Created attachment 370651 [details] [diff] [review]
minimal fix for 181

The minimal fix makes sure that the global variable optimization is only enabled when it is safe.
Comment 33 Daniel Veditz [:dveditz] 2009-04-03 10:30:08 PDT
Comment on attachment 370651 [details] [diff] [review]
minimal fix for 181

Approved for 1.8.1.22, a=dveditz for release-drivers
Comment 34 Alexander Sack 2009-04-27 08:42:31 PDT
committed attachment 370651 [details] [diff] [review] to MOZILLA_1_8_BRANCH

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.105; previous revision: 3.181.2.104
done
Comment 35 Alexander Sack 2009-04-28 04:37:59 PDT
Comment on attachment 370651 [details] [diff] [review]
minimal fix for 181

a=asac for 1.8.0
Comment 36 Bob Clary [:bc:] 2009-06-01 16:48:19 PDT
js1_5/Regress/regress-476049.js	
http://hg.mozilla.org/tracemonkey/rev/ddfb728b2023
Comment 37 Bob Clary [:bc:] 2009-08-07 16:50:15 PDT
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-476049.js,v  <--  regress-476049.js
initial revision: 1.1

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