GC hazard with JSstackFrame.argv[-1]

VERIFIED FIXED

Status

()

defect
--
critical
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

({fixed1.8.0.15, verified1.8.1.8})

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
blocking1.8.1.8 +
wanted1.8.1.x +
blocking1.8.0.next +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(4 attachments, 4 obsolete attachments)

(Assignee)

Description

12 years ago
[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

12 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

12 years ago
Severity: normal → critical
(Assignee)

Comment 2

12 years ago
Posted 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.
(Assignee)

Comment 3

12 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

12 years ago
Posted 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)
(Assignee)

Updated

12 years ago
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+
(Assignee)

Comment 6

12 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

12 years ago
Posted 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
(Assignee)

Comment 9

12 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

12 years ago
Attachment #274903 - Flags: review? → review?(brendan)
(Assignee)

Comment 10

12 years ago
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+
(Assignee)

Comment 12

12 years ago
This is the previous patch with fixed comments.
Attachment #274903 - Attachment is obsolete: true
Attachment #275164 - Flags: review+
(Assignee)

Comment 13

12 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
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Depends on: 390918

Updated

12 years ago
Flags: in-testsuite+
(Assignee)

Updated

12 years ago
Depends on: 391033
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.0.14?
Flags: blocking1.8.0.13?
(Assignee)

Comment 15

12 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+
Whiteboard: [sg:critical?]

Comment 16

12 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>
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+

Comment 18

12 years ago
verified fixed 1.9.0 linux/mac*/windows.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 19

12 years ago
Posted 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+
(Assignee)

Comment 22

12 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

12 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

12 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

12 years ago
verified fixed 1.8.1.8 win/mac*/linux

Comment 26

12 years ago
Posted patch 1.8,0 patchSplinter Review
as outlined in comment 24
Attachment #284452 - Flags: review?(igor)
Attachment #284452 - Flags: approval1.8.0.14?
(Assignee)

Updated

12 years ago
Attachment #284452 - Flags: review?(igor) → review+
Group: security

Comment 27

12 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
Flags: blocking1.8.0.14+ → blocking1.8.0.15?

Updated

11 years ago
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.