Closed Bug 390078 Opened 17 years ago Closed 17 years ago

GC hazard with JSstackFrame.argv[-1]


(Core :: JavaScript Engine, defect)

Not set





(Reporter: igor, Assigned: igor)



(Keywords: fixed1.8.0.15, verified1.8.1.8, Whiteboard: [sg:critical])


(4 files, 4 obsolete files)

[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, , 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.
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

Flags: blocking1.8.1.7?
Flags: blocking1.8.0.13?
Summary: Possible GC hazard with JSstackFrame.argv[-1] → GC hazard with JSstackFrame.argv[-1]
Severity: normal → critical
Attached patch fix v1 (obsolete) — Splinter Review
The fix delegates tracing of args to the caller frame. I will ask for a review after testing the patch.
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.
Attached patch fix v2 (obsolete) — Splinter Review
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.
Attachment #274451 - Attachment is obsolete: true
Attachment #274506 - Flags: review?(brendan)
Attachment #274506 - Attachment is patch: true
Attachment #274506 - Attachment mime type: text/x-patch → text/plain
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,

Attachment #274506 - Flags: review?(brendan) → review+
(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) <
Attached patch fix v2b (obsolete) — Splinter Review
Here is a patch to commit that uses JS_UPTRDIFF and where I updated the comments with a better text.
Attachment #274506 - Attachment is obsolete: true
Attachment #274590 - Flags: review+
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?).

With the bug 385393 fixed the patch became essentially an optimization. But for the branches the previous version should be used as a prototype.
Attachment #274903 - Flags: review?
Attachment #274903 - Flags: review? → review?(brendan)
On the trunk this the last patch compensates for a unnecessary GC marking that already committed fix does.
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
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,


r=me with that.

Attachment #274903 - Flags: review?(brendan) → review+
Attached patch patch to commitSplinter Review
This is the previous patch with fixed comments.
Attachment #274903 - Attachment is obsolete: true
Attachment #275164 - Flags: review+
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
Checking in js/src/jsiter.c;
/cvsroot/mozilla/js/src/jsiter.c,v  <--  jsiter.c
new revision: 3.69; previous revision: 3.68
Closed: 17 years ago
Resolution: --- → FIXED
Depends on: 390918
Flags: in-testsuite+
Depends on: 391033
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.0.14?
Flags: blocking1.8.0.13?
Comment on attachment 274590 [details] [diff] [review]
fix v2b

The patch is wrong even for branches, see bug 391033.
Attachment #274590 - Attachment is obsolete: true
Attachment #274590 - Flags: review+
Whiteboard: [sg:critical?]
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. 

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
Flags: blocking1.8.1.7?
Flags: blocking1.8.1.7+
Flags: blocking1.8.0.14?
Flags: blocking1.8.0.14+
verified fixed 1.9.0 linux/mac*/windows.
Attached patch fix for 1.8.1Splinter Review
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.
Attachment #282090 - Flags: review?(brendan)
Attachment #282090 - Flags: approval1.8.1.8?
Whiteboard: [sg:critical?] → [sg:critical?] need branch r=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.

Attachment #282090 - Flags: review?(brendan) → review+
Comment on attachment 282090 [details] [diff] [review]
fix for 1.8.1

approved for, a=dveditz for release-drivers
Attachment #282090 - Flags: approval1.8.1.8? → approval1.8.1.8+
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:; previous revision:

Keywords: fixed1.8.1.8
The bug is critical as it is a genuine GC hazard and allows to subvert to point to an arbitrary location.
Whiteboard: [sg:critical?] need branch r=brendan → [sg:critical]
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");
verified fixed win/mac*/linux
Attached patch 1.8,0 patchSplinter Review
as outlined in comment 24
Attachment #284452 - Flags: review?(igor)
Attachment #284452 - Flags: approval1.8.0.14?
Attachment #284452 - Flags: review?(igor) → review+
Group: security
Checking in regress-390078.js;
/cvsroot/mozilla/js/tests/js1_5/GC/regress-390078.js,v  <--  regress-390078.js
initial revision: 1.1
Flags: blocking1.8.0.14+ → blocking1.8.0.15?
Flags: blocking1.8.0.15? → blocking1.8.0.15+
Comment on attachment 284452 [details] [diff] [review]
1.8,0 patch

missed .14 but a=caillon for
Attachment #284452 - Flags: approval1.8.0.14? → approval1.8.0.15+
Fix committed to 1.8.0 branch
Keywords: fixed1.8.0.15
You need to log in before you can comment on or make changes to this bug.