Last Comment Bug 390078 - GC hazard with JSstackFrame.argv[-1]
: GC hazard with JSstackFrame.argv[-1]
Status: VERIFIED FIXED
[sg:critical]
: fixed1.8.0.15, verified1.8.1.8
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- critical (vote)
: ---
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 390918 391033
Blocks:
  Show dependency treegraph
 
Reported: 2007-07-29 15:41 PDT by Igor Bukanov
Modified: 2008-03-20 12:00 PDT (History)
4 users (show)
brendan: blocking1.9+
dveditz: blocking1.8.1.8+
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next+
dveditz: wanted1.8.0.x+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v1 (5.07 KB, patch)
2007-07-30 05:17 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
fix v2 (5.34 KB, patch)
2007-07-30 13:40 PDT, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review
fix v2b (5.39 KB, patch)
2007-07-31 01:00 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
Updated patch to reflect the changes from bug 385393 (3.71 KB, patch)
2007-08-02 02:41 PDT, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review
patch to commit (3.71 KB, patch)
2007-08-03 12:54 PDT, Igor Bukanov
igor: review+
Details | Diff | Splinter Review
js1_5/GC/regress-390078.js (2.53 KB, text/plain)
2007-08-05 19:23 PDT, Bob Clary [:bc:]
no flags Details
fix for 1.8.1 (2.22 KB, patch)
2007-09-24 02:37 PDT, Igor Bukanov
brendan: review+
dveditz: approval1.8.1.8+
Details | Diff | Splinter Review
1.8,0 patch (1.04 KB, patch)
2007-10-11 04:34 PDT, Alexander Sack
igor: review+
caillon: approval1.8.0.next+
Details | Diff | Splinter Review

Description Igor Bukanov 2007-07-29 15:41:39 PDT
[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 2007-07-30 04:48:30 PDT
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 2007-07-30 05:17:08 PDT
Created attachment 274451 [details] [diff] [review]
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 2007-07-30 06:24:21 PDT
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 2007-07-30 13:40:33 PDT
Created attachment 274506 [details] [diff] [review]
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] 2007-07-30 19:13:54 PDT
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 2007-07-31 00:10:15 PDT
(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 2007-07-31 01:00:15 PDT
Created attachment 274590 [details] [diff] [review]
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] 2007-07-31 11:03:42 PDT
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 2007-08-02 02:41:11 PDT
Created attachment 274903 [details] [diff] [review]
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 2007-08-02 11:25:56 PDT
On the trunk this the last patch compensates for a unnecessary GC marking that already committed fix does.
Comment 11 Brendan Eich [:brendan] 2007-08-02 14:47:19 PDT
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 2007-08-03 12:54:31 PDT
Created attachment 275164 [details] [diff] [review]
patch to commit

This is the previous patch with fixed comments.
Comment 13 Igor Bukanov 2007-08-03 12:57:33 PDT
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:] 2007-08-05 19:23:04 PDT
Created attachment 275375 [details]
js1_5/GC/regress-390078.js
Comment 15 Igor Bukanov 2007-08-06 13:08:49 PDT
Comment on attachment 274590 [details] [diff] [review]
fix v2b

The patch is wrong even for branches, see bug 391033.
Comment 16 Bob Clary [:bc:] 2007-08-13 02:01:41 PDT
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] 2007-08-28 14:12:10 PDT
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:] 2007-09-18 15:50:14 PDT
verified fixed 1.9.0 linux/mac*/windows.
Comment 19 Igor Bukanov 2007-09-24 02:37:02 PDT
Created attachment 282090 [details] [diff] [review]
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] 2007-09-25 19:45:43 PDT
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] 2007-09-26 10:24:20 PDT
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 2007-09-27 07:57:47 PDT
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 2007-09-27 11:16:15 PDT
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 2007-09-27 11:40:24 PDT
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:] 2007-10-05 04:10:52 PDT
verified fixed 1.8.1.8 win/mac*/linux
Comment 26 Alexander Sack 2007-10-11 04:34:31 PDT
Created attachment 284452 [details] [diff] [review]
1.8,0 patch

as outlined in comment 24
Comment 27 Bob Clary [:bc:] 2007-10-18 19:27:15 PDT
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) 2008-02-19 08:55:17 PST
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) 2008-03-20 12:00:51 PDT
Fix committed to 1.8.0 branch

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