476049 - JSOP_DEFVAR enables gvar optimization for non-permanent properties

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Igor Bukanov]

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]

Attachment: 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]]

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]

(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]

Attachment: 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]

Comment on attachment 359819 [details] [diff] [review]
v2

the patch has a bug

Comment 6

[Igor Bukanov]

Attachment: 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]]

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]

Attachment: 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]]

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]

Attachment: v5

New version addresses the comment 19.

Comment 11

[Igor Bukanov]

landed in tm - http://hg.mozilla.org/tracemonkey/rev/30287116aafa

Comment 12

[Damon Sicore (:damons)]

P1.

Comment 13

[Igor Bukanov]

This goes back to the pool.

Comment 14

[Andreas Gal :gal]

Isn't this fixed and ready to be closed?

Comment 15

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/30287116aafa

Comment 16

[Samuel Sidler (old account; do not CC)]

Igor: Can you work up a 1.9.0 patch for this?

Comment 17

[Igor Bukanov]

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]

Attachment: fix for 1.9.0

For 1.9.0 the patch required a trivial backport.

Comment 19

[Igor Bukanov]

Attachment: 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] (inactive)]

Attachment: js1_5/Regress/regress-476049.js (browser only)

Comment 21

[Daniel Veditz [:dveditz]]

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] (inactive)]

Igor, could you land this on 1.9.0?

Comment 23

[Igor Bukanov]

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)]

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]

(In reply to comment #24)
> Igor: Any ideas?

No at this moment.

Comment 26

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e8af3632a973

Comment 27

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2

Comment 28

[Bob Clary [:bc] (inactive)]

v 1.9.0

Comment 29

[Samuel Sidler (old account; do not CC)]

Igor: Is this something we want on the 1.8 branch?

Comment 30

[Igor Bukanov]

(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)]

Thanks Igor. Any chance you can work up a minimal patch?

Comment 32

[Igor Bukanov]

Attachment: 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]]

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]

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]

Comment on attachment 370651 [details] [diff] [review]
minimal fix for 181

a=asac for 1.8.0

Comment 36

[Bob Clary [:bc] (inactive)]

js1_5/Regress/regress-476049.js	
http://hg.mozilla.org/tracemonkey/rev/ddfb728b2023

Comment 37

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-476049.js,v  <--  regress-476049.js
initial revision: 1.1