Closed Bug 390078 Opened 17 years ago Closed 17 years ago

GC hazard with JSstackFrame.argv[-1]

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

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

Attachments

(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, 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.
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, /be
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?). /be
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, s/bace/base/ r=me with that. /be
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 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
Status: NEW → RESOLVED
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. <http://crash-stats.mozilla.com/report/index/11a727eb-497a-11dc-809a-001a4bd43e5c> <http://crash-stats.mozilla.com/report/index/fbd1b178-4979-11dc-b2c6-001a4bd43e5c>
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.
Status: RESOLVED → VERIFIED
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. /be
Attachment #282090 - Flags: review?(brendan) → review+
Comment on attachment 282090 [details] [diff] [review] fix for 1.8.1 approved for 1.8.1.8, 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: 3.104.2.34; previous revision: 3.104.2.33 done
Keywords: fixed1.8.1.8
The bug is critical as it is a genuine GC hazard and allows to subvert JSObject.map 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 1.8.1.8 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 1.8.0.15
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.

Attachment

General

Creator:
Created:
Updated:
Size: