390078 - GC hazard with JSstackFrame.argv[-1]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

[This can be invalid as I have not tried to come up with an example yet but I file this in case the following analysis would be true].

js_TraceStackFrame from jsgc.c, http://lxr.mozilla.org/seamonkey/source/js/src/jsgc.c#1943 , contains the following comments, 

    JS_CALL_VALUE_TRACER(trc, (jsval)fp->thisp, "this");
    
    /*
     * Mark fp->argv, even though in the common case it will be marked via our
     * caller's frame, or via a JSStackHeader if fp was pushed by an external
     * invocation.
     *
     * The hard case is when there is not enough contiguous space in the stack
     * arena for actual, missing formal, and local root (JSFunctionSpec.extra)
     * slots.  In this case, fp->argv points to new space in a new arena, and
     * marking the caller's operand stack, or an external caller's allocated
     * stack tracked by a JSStackHeader, will not mark all the values stored
     * and addressable via fp->argv.
     *
     * But note that fp->argv[-2] will be marked via the caller, even when the
     * arg-vector moves.  And fp->argv[-1] will be marked as well, and we mark
     * it redundantly just above this comment.
     */

The last sentence  of this comment block is not true, as the trace code above the comment traces fp->thisp, the initial value stored in fp->argv[-1]. The code  does not trace the current value stored there. Thus if a native method uses argv[-1] to store its temporaries and the argv was reallocated, then there is a GC hazard in that call. I'll check if this is indeed true.

Comment 1

[Igor Bukanov]

This is a real hazard as the following example demonstrates:

~/m/trunk/mozilla/js/src> cat ~/m/y.js
var a = new Array(10*1000);
a[0] = { toString: function() { gc(); return ".*9"; }};;
a[1] = "g";

for (var i = 0; i != 10*1000; ++i) {
    String(new Number(123456789));
}

"".match.apply(123456789, a);

~/m/trunk/mozilla/js/src> ./Linux_All_DBG.OBJ/js ~/m/y.js
before 498528, after 46160, break 08437000
Segmentation fault

Comment 2

[Igor Bukanov]

Attachment: fix v1

The fix delegates tracing of args to the caller frame. I will ask for a review after testing the patch.

Comment 3

[Igor Bukanov]

The last patch regressed exactly as described in bug 385393 comment 125. Remooving that fp->sp = vp from jsinterp.c fixes that, but I do not see why it matters.

Comment 4

[Igor Bukanov]

Attachment: fix v2

The new version does not mess with fp->sp as that prevents generating useful errors reports for callee/thisp/arguments. Instead it checks if argv overlaps with spbase..sp and skips tracing of the common area.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 274506 [details] [diff] [review]
fix v2

>+            if ((size_t)(vp - fp->down->spbase) <
>+                (size_t)(fp->down->sp - fp->down->spbase)) {

Could use JS_UPTRDIFF for both < operands. Might even fit on one line (looks like not here, but still -- all other pointer subtracting code of this sort (single-test bounded on both ends range tests) does use JS_UPTRDIFF, AFAIK.

Thanks for fixing this,

/be

Comment 6

[Igor Bukanov]

(In reply to comment #5)
> Could use JS_UPTRDIFF for both < operands. Might even fit on one line

JS_UPTRDIFF takes more space than size_t cast so it would not fit. From the updated patch:

-            if ((size_t)(vp - fp->down->spbase) <
+            if (JS_UPTRDIFF(vp, fp->down->spbase) <

Comment 7

[Igor Bukanov]

Attachment: fix v2b

Here is a patch to commit that uses JS_UPTRDIFF and where I updated the comments with a better text.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 274590 [details] [diff] [review]
fix v2b

Thanks. I can only appeal to consistency in favor of the longer form (JS_UPTRDIFF does cast so you get byte-address differences, but that's not important here, and the casts should not be necessary to avoid pointer scaling; also it may have mattered for Win16, but who cares?).

/be

Comment 9

[Igor Bukanov]

Attachment: Updated patch to reflect the changes from bug 385393

With the bug 385393 fixed the patch became essentially an optimization. But for the branches the previous version should be used as a prototype.

Comment 10

[Igor Bukanov]

On the trunk this the last patch compensates for a unnecessary GC marking that already committed fix does.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 274903 [details] [diff] [review]
Updated patch to reflect the changes from bug 385393

>+             * traced via the above spbase code and, when sp > spbace + depth,

s/bace/base/

r=me with that.

/be

Comment 12

[Igor Bukanov]

Attachment: patch to commit

This is the previous patch with fixed comments.

Comment 13

[Igor Bukanov]

I checked in the patch from comment 12 to the trunk:

Checking in js/src/jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.230; previous revision: 3.229
done
Checking in js/src/jsiter.c;
/cvsroot/mozilla/js/src/jsiter.c,v  <--  jsiter.c
new revision: 3.69; previous revision: 3.68
done

Comment 14

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/GC/regress-390078.js

Comment 15

[Igor Bukanov]

Comment on attachment 274590 [details] [diff] [review]
fix v2b

The patch is wrong even for branches, see bug 391033.

Comment 16

[Bob Clary [:bc] (inactive)]

My js test run for 8/11 showed crashes on mac ppc opt firefox builds with this test (comment 14) in the browser on two different machines. I couldn't reproduce at the moment with a nightly build but was able to get two crashes to occur with an opt cvs build used in the test. 

<http://crash-stats.mozilla.com/report/index/11a727eb-497a-11dc-809a-001a4bd43e5c>
<http://crash-stats.mozilla.com/report/index/fbd1b178-4979-11dc-b2c6-001a4bd43e5c>

Comment 17

[Daniel Veditz [:dveditz]]

Blocking for the 1.8 branch, but looks like we need an updated branch patch that includes at least the regression fix for bug 391033

Comment 18

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 linux/mac*/windows.

Comment 19

[Igor Bukanov]

Attachment: fix for 1.8.1

To avoid the risk of regressions the fix for 1.8.1 branch just unconditionally marks argv[-1] and argv[-2]. The latter strictly speaking is not necessary since I am not aware of native code that uses argv[-2] to root a value and a copy of the original value stored in argv[-2] is always rooted. But I prefer to play safe here.

If an optimization is desired, then instead of back-porting regression fixes for this bug it would better to bring to the branch a patch based on bug 394551.

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 282090 [details] [diff] [review]
fix for 1.8.1

Sure, let's not overdo backporting without some measurements showing a real perf prob in slightly overscanning the stack.

/be

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 282090 [details] [diff] [review]
fix for 1.8.1

approved for 1.8.1.8, a=dveditz for release-drivers

Comment 22

[Igor Bukanov]

I checked in the patch from comment 19 to MOZILLA_1_8_BRANCH:

Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.104.2.34; previous revision: 3.104.2.33
done

Comment 23

[Igor Bukanov]

The bug is critical as it is a genuine GC hazard and allows to subvert JSObject.map to point to an arbitrary location.

Comment 24

[Igor Bukanov]

The bug present on 1.8.0 branch and the affected vode is in js_GC, line 1768:

 GC_MARK_JSVALS(cx, nslots, fp->argv, "arg");

Comment 25

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.1.8 win/mac*/linux

Comment 26

[Alexander Sack]

Attachment: 1.8,0 patch

as outlined in comment 24

Comment 27

[Bob Clary [:bc] (inactive)]

Checking in regress-390078.js;
/cvsroot/mozilla/js/tests/js1_5/GC/regress-390078.js,v  <--  regress-390078.js
initial revision: 1.1

Comment 28

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 284452 [details] [diff] [review]
1.8,0 patch

missed .14 but a=caillon for 1.8.0.15

Comment 29

[Christopher Aillon (sabbatical, not receiving bugmail)]

Fix committed to 1.8.0 branch