JS_TraceObject may not trace reserved slots

VERIFIED FIXED

Status

()

Core
JavaScript Engine
P2
normal
VERIFIED FIXED
10 years ago
3 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

Trunk
Points:
---
Bug Flags:
blocking1.9 +
wanted1.8.1.x +
in-testsuite ?
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?][not needed on the branch?])

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

10 years ago
JS_TraceObject from jsobj.c contains:

    nslots = (scope->object != obj)
             ? STOBJ_NSLOTS(obj)
             : LOCKED_OBJ_NSLOTS(obj);
 
     for (i = 0; i != nslots; ++i) {
         v = STOBJ_GET_SLOT(obj, i);
         if (JSVAL_IS_TRACEABLE(v)) {
             JS_SET_TRACING_DETAILS(trc, js_PrintObjectSlotName, obj, i);
             JS_CallTracer(trc, JSVAL_TO_TRACEABLE(v), JSVAL_TRACE_KIND(v));
         }
     }

This code will not trace the reserved slots if the objects own the scope and  LOCKED_OBJ_NSLOTS(obj) == JSSLOT_FREE(clasp), that is, when obj->map->freeslot == JSSLOT_FREE(clasp). 

I hit this bug when working on bug 411575 where the patch lead to this situation. Currently it is unclear if this hazard can be triggered without that patch.
(Assignee)

Comment 1

10 years ago
The bug happens when, after adding reserved slots to an object that does not own scope, a new slot-less property property is added to the object. That triggers initialization of the object map which sets map.freeslot to JSSLOT_FREE(clasp) without calling js_AllocSlot later. Thus freeslot is stuck at JSSLOT_FREE(clasp) causing GC to ignore the reserved slots when tracing.

This is exactly the situation that the patch for bug 411575 triggered.
(Assignee)

Comment 2

10 years ago
Here is a test case demonstrating the GC hazard in js shell. It exploits the fact that the engine uses reserved slots to store regular expression literals for functions. 

~/m/ff/mozilla/js/src $ cat ~/s/x.js
function f()
{
    return /test/;
}

var expect = uneval(f());

f.__defineGetter__('a', function() { return 10; });

gc();

var actual = uneval(f());

if (ectaul !== expect)
    throw "Bug!";
~/m/ff/mozilla/js/src $ Linux_All_DBG.OBJ/js ~/s/x.js
before 32768, after 32768, break 08cc8000
segmentation fault

Flags: blocking1.9?
Flags: blocking1.8.1.14?
(Assignee)

Comment 3

10 years ago
Created attachment 311675 [details] [diff] [review]
v1

The fix makes sure that js_GetMutableScope takes into account the reserved slots when initializing the new map.
Attachment #311675 - Flags: review?(brendan)
(Assignee)

Comment 4

10 years ago
Created attachment 311798 [details] [diff] [review]
v2

In addition to changes from the previous version the new patch eliminates LOCKED_OBJ_NSLOTS and instead uses scope->map.freeslot as scope->map.freeslot cannot exceed STOBJ_NSLOTS(obj). This makes the code more explicit.

Ideally it would be nice to assert in js_TraceObject that scope->map.freeslot points beyond the reserved slots, but since JSClass.reserveSlots may, say, allocate new GC things, it can not be used when asserting during GC.
Attachment #311675 - Attachment is obsolete: true
Attachment #311798 - Flags: review?(brendan)
Attachment #311675 - Flags: review?(brendan)
This should block 1.9
Whiteboard: [sg:critical?]

Updated

10 years ago
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Comment on attachment 311798 [details] [diff] [review]
v2

>-/*
>- * Macros for accessing slots in obj while obj is locked (if thread-safe) and
>- * when slot must be bounded by the map->freeslot.
>- */
>-#define LOCKED_OBJ_NSLOTS(obj)                                                \
>-   JS_MIN((obj)->map->freeslot, STOBJ_NSLOTS(obj))

Good to get rid of this, thanks.

>+    uint32 reserveEnd;

Suggest freeslot as better name.

/be
Attachment #311798 - Flags: review?(brendan)
Attachment #311798 - Flags: review+
Attachment #311798 - Flags: approval1.9?
(Assignee)

Comment 7

10 years ago
Created attachment 312349 [details] [diff] [review]
v3

The new version of the patch addresses the suggestions from the comment 6 and uses newscope->map.freeslot, not obj->map->freeslot, for less indirection:

--- /home/igor/m/ff/mozilla/quilt.Dr1357/js/src/jsscope.c	2008-03-28 21:10:15.000000000 +0100
+++ js/src/jsscope.c	2008-03-28 21:06:53.000000000 +0100
@@ -66,3 +66,3 @@ js_GetMutableScope(JSContext *cx, JSObje
     JSClass *clasp;
-    uint32 reserveEnd;
+    uint32 freeslot;
 
@@ -78,10 +78,10 @@ js_GetMutableScope(JSContext *cx, JSObje
     obj->map = js_HoldObjectMap(cx, &newscope->map);
-    JS_ASSERT(obj->map->freeslot == JSSLOT_FREE(STOBJ_GET_CLASS(obj)));
+    JS_ASSERT(newscope->map.freeslot == JSSLOT_FREE(STOBJ_GET_CLASS(obj)));
     clasp = STOBJ_GET_CLASS(obj);
     if (clasp->reserveSlots) {
-        reserveEnd = JSSLOT_FREE(clasp) + clasp->reserveSlots(cx, obj);
-        if (reserveEnd > STOBJ_NSLOTS(obj))
-            reserveEnd = STOBJ_NSLOTS(obj);
-        if (obj->map->freeslot < reserveEnd)
-            obj->map->freeslot = reserveEnd;
+        freeslot = JSSLOT_FREE(clasp) + clasp->reserveSlots(cx, obj);
+        if (freeslot > STOBJ_NSLOTS(obj))
+            freeslot = STOBJ_NSLOTS(obj);
+        if (newscope->map.freeslot < freeslot)
+            newscope->map.freeslot = freeslot;
     }
Attachment #311798 - Attachment is obsolete: true
Attachment #312349 - Flags: review+
Attachment #312349 - Flags: approval1.9?
Attachment #311798 - Flags: approval1.9?

Comment 8

10 years ago
Igor - what is the test coverage on this?
(Assignee)

Comment 9

10 years ago
(In reply to comment #8)
> Igor - what is the test coverage on this?
> 

The comment 2 gives a test case for js shell.

Comment 10

10 years ago
Comment on attachment 312349 [details] [diff] [review]
v3

bc can we get a shell testcase?
Attachment #312349 - Flags: approval1.9? → approval1.9+

Comment 11

10 years ago
Created attachment 312427 [details]
js/js1_5/extensions/regress-424964.js

Updated

10 years ago
Flags: in-testsuite+
Flags: in-litmus-
(Assignee)

Comment 12

10 years ago
The bug is critical since it allows to replace to replace regexp literal objects in a function by a script-controllable things (including overwriting all fields of the object with bits from script suplied double values).

Here is a variation of the example from the comment 2 that replaces the regexp with script-constructed object:

~/m/ff/mozilla $ cat ~/s/x.js
function f(text) { return /test/.test(text); }

function a() { return 10; }

function like_regexp_test(str)
{
    throw "Subverted regexp test arg: "+str;
}

var expect = f("test");
gc();
f.__defineGetter__('a', a);
gc();

var regexp_literal_replacement = { test: like_regexp_test };
var actual = f("test");

if (actual !== expect)
    throw "Bug:";
~/m/ff/mozilla $ ~/m/ff/mozilla/js/src/Linux_All_DBG.OBJ/js ~/s/x.js
before 16384, after 16384, break 0931e000
before 16384, after 16384, break 0931e000
uncaught exception: Subverted regexp test arg: test


Note that the example uses gc() from the shell, in the browser it can be emulated via code like:

for (var i = 0; i != 4e6; ++i) var tmp = i + 0.1;
Whiteboard: [sg:critical?] → [sg:critical]
(Assignee)

Comment 13

10 years ago
I checked in the patch from the comment 7 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1206797880&maxdate=1206798015&who=igor%25mir2.org
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Depends on: 425936

Comment 14

10 years ago
Created attachment 316270 [details]
js1_5/extensions/regress-424964-02.js

from comment 12.

Also updated the version of gc() to match Igor's latest recommended emulation as it better identified a fault in an older build prior to this bug being fixed.

/cvsroot/mozilla/js/tests/browser.js,v  <--  browser.js
new revision: 1.7; previous revision: 1.

Comment 15

10 years ago
v 1.9.0
Status: RESOLVED → VERIFIED
How easily/safely will this back-port to the 1.8 branch?
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.15?
Flags: blocking1.8.1.15+
(Assignee)

Comment 17

10 years ago
(In reply to comment #16)
> How easily/safely will this back-port to the 1.8 branch?
> 

The branch requires a separated new patch since the amount of changes on the trunk does not permit a simple backport.
Igor: how's a branch patch coming on this one?
(Assignee)

Comment 19

10 years ago
(In reply to comment #18)
> Igor: how's a branch patch coming on this one?
> 

I am not sure that I would have a patch ready for 1.8.1.15. The problem is to make sure that it is a regression-free.
(Assignee)

Comment 20

10 years ago
Note about the test case from the comment 12. It does not expose the bug on 1.8 branch. AFAICS the bug is only visible when a function clone its regexp literal in JSOP_REGEXP bytecode. This does not happens on the branch with the example. OWith 1.8 code base the parent of the compiled regexp literal object is the same global object that ends the scope chain of call object (if any) during the execution of the function with the regexp literal. 

Thus the code would not clone the regexp and there is no GC hazard because the untraced slots of the function object contains the regexp object that also stored in JSScript containing the function. To expose the bug on 1.8 branch one would need a script that is not compiled with COMPILE_AND_GO and then it is necessary to execute the script with a global that is different from script.__parent__. So here is an example for the shell that organizes that:

~/m/181-ff/mozilla/js/src $ cat ~/s/x.js

var s = evalcx('Script("0, function() { return /1/;}");');
var f = s();

var expect = uneval(f());

f.__defineGetter__('a', Math.sin);

gc();

var actual = uneval(f());

if (actual !== expect)
    throw "Bug!";
~/m/181-ff/mozilla/js/src $ ./Linux_All_DBG.OBJ/js ~/s/x.js
before 9232, after 9232, break 092f1000
segmentation fault
Whiteboard: [sg:critical] → [sg:critical][not needed on the branch?]
Flags: blocking1.8.1.15+ → blocking1.8.1.16?
Whiteboard: [sg:critical][not needed on the branch?] → [sg:critical?][not needed on the branch?]
I'm not quite sure what you're saying in comment 20 -- this bug affects the 1.8 branch spidermonkey but not in a way that Firefox web content could get to?
(Assignee)

Comment 22

10 years ago
(In reply to comment #21)
> I'm not quite sure what you're saying in comment 20 -- this bug affects the 1.8
> branch spidermonkey but not in a way that Firefox web content could get to?

At this moment I do not know if the necessary code path can be triggered from FF. My guts tell me that this is possible and the bug has to be fixed. But the risk for regressions is huge especially given that the 1.9 fix is of no help here.

I suspect that the best thing to do would be to add a small performance penalty for functions with regexp literals and use an extra malloc to store them removing the need to fix the reserved slot issue.
A "huge" regression risk for a problem we're not sure anyone can get to from web content (or will discover) doesn't sound like a good tradeoff on branch. Is the malloc approach something you could whip up safely, or should we just punt on the branch version of this one?
Given the regression risk on branch and the doubt about how to cause this code path we're not going to block on this one on the 1.8 branch. We'll fix it later if we have to, or if the alternate approach turns out to be less risky.
Flags: blocking1.8.1.17?

Comment 25

8 years ago
when this bug is opened, the test should be checked in.
Flags: in-testsuite+ → in-testsuite?
Group: core-security
You need to log in before you can comment on or make changes to this bug.