Closed
Bug 390078
Opened 17 years ago
Closed 17 years ago
GC hazard with JSstackFrame.argv[-1]
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
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)
3.71 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
2.53 KB,
text/plain
|
Details | |
2.22 KB,
patch
|
brendan
:
review+
dveditz
:
approval1.8.1.8+
|
Details | Diff | Splinter Review |
1.04 KB,
patch
|
igor
:
review+
caillon
:
approval1.8.0.next+
|
Details | Diff | Splinter Review |
[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.
Assignee | ||
Comment 1•17 years ago
|
||
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]
Assignee | ||
Updated•17 years ago
|
Severity: normal → critical
Assignee | ||
Comment 2•17 years ago
|
||
The fix delegates tracing of args to the caller frame. I will ask for a review after testing the patch.
Assignee | ||
Comment 3•17 years ago
|
||
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.
Assignee | ||
Comment 4•17 years ago
|
||
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)
Assignee | ||
Updated•17 years ago
|
Attachment #274506 -
Attachment is patch: true
Attachment #274506 -
Attachment mime type: text/x-patch → text/plain
Comment 5•17 years ago
|
||
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+
Assignee | ||
Comment 6•17 years ago
|
||
(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) <
Assignee | ||
Comment 7•17 years ago
|
||
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 8•17 years ago
|
||
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
Assignee | ||
Comment 9•17 years ago
|
||
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?
Assignee | ||
Updated•17 years ago
|
Attachment #274903 -
Flags: review? → review?(brendan)
Assignee | ||
Comment 10•17 years ago
|
||
On the trunk this the last patch compensates for a unnecessary GC marking that already committed fix does.
Flags: blocking1.9?
Updated•17 years ago
|
Flags: blocking1.9? → blocking1.9+
Comment 11•17 years ago
|
||
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+
Assignee | ||
Comment 12•17 years ago
|
||
This is the previous patch with fixed comments.
Attachment #274903 -
Attachment is obsolete: true
Attachment #275164 -
Flags: review+
Assignee | ||
Comment 13•17 years ago
|
||
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
Comment 14•17 years ago
|
||
Updated•17 years ago
|
Flags: in-testsuite+
Updated•17 years ago
|
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.0.14?
Flags: blocking1.8.0.13?
Assignee | ||
Comment 15•17 years ago
|
||
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+
Updated•17 years ago
|
Whiteboard: [sg:critical?]
Comment 16•17 years ago
|
||
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•17 years ago
|
||
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+
Assignee | ||
Comment 19•17 years ago
|
||
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?
Updated•17 years ago
|
Whiteboard: [sg:critical?] → [sg:critical?] need branch r=brendan
Comment 20•17 years ago
|
||
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 21•17 years ago
|
||
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+
Assignee | ||
Comment 22•17 years ago
|
||
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
Assignee | ||
Comment 23•17 years ago
|
||
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]
Assignee | ||
Comment 24•17 years ago
|
||
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•17 years ago
|
||
verified fixed 1.8.1.8 win/mac*/linux
Keywords: fixed1.8.1.8 → verified1.8.1.8
Comment 26•17 years ago
|
||
as outlined in comment 24
Attachment #284452 -
Flags: review?(igor)
Attachment #284452 -
Flags: approval1.8.0.14?
Assignee | ||
Updated•17 years ago
|
Attachment #284452 -
Flags: review?(igor) → review+
Updated•17 years ago
|
Group: security
Comment 27•17 years ago
|
||
Checking in regress-390078.js; /cvsroot/mozilla/js/tests/js1_5/GC/regress-390078.js,v <-- regress-390078.js initial revision: 1.1
Updated•17 years ago
|
Flags: blocking1.8.0.14+ → blocking1.8.0.15?
Updated•16 years ago
|
Flags: blocking1.8.0.15? → blocking1.8.0.15+
Comment 28•16 years ago
|
||
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+
You need to log in
before you can comment on or make changes to this bug.
Description
•