Closed Bug 476049 Opened 15 years ago Closed 15 years ago

JSOP_DEFVAR enables gvar optimization for non-permanent properties

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

(4 keywords, Whiteboard: [sg:critical] fixed-in-tracemonkey)

Attachments

(5 files, 4 obsolete files)

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.
Flags: blocking1.9.1?
Flags: blocking1.9.0.7?
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #359768 - Flags: review?(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
(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?
Attached patch v2 (obsolete) — Splinter Review
The new patch adds the missing OBJ_DROP_PROPERTY and replaces long-but-still-unclear names for g-locals by short names and comments.
Attachment #359768 - Attachment is obsolete: true
Attachment #359819 - Flags: review?(brendan)
Attachment #359768 - Flags: review?(brendan)
Comment on attachment 359819 [details] [diff] [review]
v2

the patch has a bug
Attachment #359819 - Attachment is obsolete: true
Attachment #359819 - Flags: review?(brendan)
Attached patch v3 (obsolete) — Splinter Review
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";
Attachment #359825 - Flags: review?(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
Attachment #359825 - Flags: review?(brendan) → review+
Attached patch v4 (obsolete) — Splinter Review
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.
Attachment #359825 - Attachment is obsolete: true
Attachment #359896 - Flags: review?(brendan)
Blocks: 475971
Attachment #359896 - Flags: review?(brendan) → review+
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
Flags: wanted1.9.0.x+
Whiteboard: [sg:critical]
Attached patch v5Splinter Review
New version addresses the comment 19.
Attachment #359896 - Attachment is obsolete: true
Attachment #360125 - Flags: review+
landed in tm - http://hg.mozilla.org/tracemonkey/rev/30287116aafa
Whiteboard: [sg:critical] → [sg:critical] fixed-in-tracemonkey
Flags: blocking1.9.0.7? → blocking1.9.0.8?
Flags: blocking1.9.0.8? → blocking1.9.0.8+
Flags: blocking1.9.1? → blocking1.9.1+
P1.
Priority: -- → P1
This goes back to the pool.
Assignee: igor → general
Isn't this fixed and ready to be closed?
http://hg.mozilla.org/mozilla-central/rev/30287116aafa
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Igor: Can you work up a 1.9.0 patch for this?
Assignee: general → igor
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
@
Attached patch fix for 1.9.0Splinter Review
For 1.9.0 the patch required a trivial backport.
Attachment #362016 - Flags: approval1.9.0.8?
Here is a proof of triviality of 1.9.0 backporting efforts.
Flags: in-testsuite+
Flags: in-litmus-
Comment on attachment 362016 [details] [diff] [review]
fix for 1.9.0

Approved for 1.9.0.8, a=dveditz for release-drivers
Attachment #362016 - Flags: approval1.9.0.8? → approval1.9.0.8+
Igor, could you land this on 1.9.0?
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
Keywords: fixed1.9.0.8
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?
(In reply to comment #24)
> Igor: Any ideas?

No at this moment.
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
Igor: Is this something we want on the 1.8 branch?
Flags: wanted1.8.1.x?
(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.
Thanks Igor. Any chance you can work up a minimal patch?
Flags: wanted1.8.1.x?
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.next+
The minimal fix makes sure that the global variable optimization is only enabled when it is safe.
Attachment #370651 - Flags: approval1.8.1.next?
Comment on attachment 370651 [details] [diff] [review]
minimal fix for 181

Approved for 1.8.1.22, a=dveditz for release-drivers
Attachment #370651 - Flags: approval1.8.1.next? → approval1.8.1.next+
Group: core-security
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
Keywords: fixed1.8.1.22
Flags: blocking1.8.0.next+
Comment on attachment 370651 [details] [diff] [review]
minimal fix for 181

a=asac for 1.8.0
Attachment #370651 - Flags: approval1.8.0.next+
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-476049.js,v  <--  regress-476049.js
initial revision: 1.1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: