Closed
Bug 411025
Opened 16 years ago
Closed 16 years ago
GC hazard in JS_CompileUCFunctionForPrincipals
Categories
(Core :: JavaScript Engine, defect, P2)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: igor, Assigned: igor)
References
Details
(Keywords: fixed1.8.1.13, Whiteboard: [sg:critical?] [needs branch patch] 1.8 branch regression is bug 425576)
Attachments
(3 files, 10 obsolete files)
7.52 KB,
patch
|
Details | Diff | Splinter Review | |
4.29 KB,
patch
|
Details | Diff | Splinter Review | |
16.85 KB,
patch
|
asac
:
approval1.8.0.next-
|
Details | Diff | Splinter Review |
JS_CompileUCFunctionForPrincipals contains the following code: JSFunction *fun; ... fun = js_NewFunction(cx, NULL, NULL, 0, JSFUN_INTERPRETED, obj, funAtom); ... if (!js_CompileFunctionBody(cx, fun, principals, chars, length, filename, lineno)) { ... if (obj && funAtom && !OBJ_DEFINE_PROPERTY(cx, obj, ATOM_TO_JSID(funAtom), OBJECT_TO_JSVAL(fun->object), NULL, NULL, JSPROP_ENUMERATE, NULL)) { fun = NULL; } ... LAST_FRAME_CHECKS(cx, fun); return fun; Here js_NewFunction creates a function rooted only via the newborn array. If the supplied source contains other function objects, then when the compiler allocates them they displace the fun from the newborn array and unroot it. Then any last-ditch GC during the compilation later would collect the live pointer. Before the bug 404935 and on 1.8.1 branch the problem was not that bad as a pseudo-frame that the compiler creates would root fun during the compilation. Still after js_CompileFunctionBody returns, OBJ_DEFINE_PROPERTY would be called with unrooted fun or, when obj is null, the unrooted value would be returned to the caller.
Assignee | ||
Comment 1•16 years ago
|
||
The fix adds JS_PUSH_TEMP_ROOT_FUNCTION to root the function. To avoid yet another copy of almost similar macro lines to define the new rooting helper I parametrized JS_PUSH_TEMP_ROOT_COMMON to accept the count and field name arguments so all JS_PUSH_TEMP_ROOT_ can be made one-liners. With the patch all TVR macro definitions fit on one screen: #define JS_PUSH_TEMP_ROOT_COMMON(cx,x,tvr,cnt,fieldName) \ JS_BEGIN_MACRO \ JS_ASSERT((cx)->tempValueRooters != (tvr)); \ (tvr)->count = cnt; \ (tvr)->u.fieldName = (x); \ (tvr)->down = (cx)->tempValueRooters; \ (cx)->tempValueRooters = (tvr); \ JS_END_MACRO #define JS_POP_TEMP_ROOT(cx,tvr) \ JS_BEGIN_MACRO \ JS_ASSERT((cx)->tempValueRooters == (tvr)); \ (cx)->tempValueRooters = (tvr)->down; \ JS_END_MACRO #define JS_PUSH_TEMP_ROOT(cx,cnt,arr,tvr) \ JS_BEGIN_MACRO \ JS_ASSERT((ptrdiff_t)(cnt) >= 0); \ JS_PUSH_TEMP_ROOT_COMMON(cx, arr, tvr, ((ptrdiff_t) (cnt)), array); \ JS_END_MACRO #define JS_PUSH_SINGLE_TEMP_ROOT(cx,val,tvr) \ JS_PUSH_TEMP_ROOT_COMMON(cx, val, tvr, JSTVU_SINGLE, value) #define JS_PUSH_TEMP_ROOT_OBJECT(cx,obj,tvr) \ JS_PUSH_TEMP_ROOT_COMMON(cx, obj, tvr, JSTVU_SINGLE, object) #define JS_PUSH_TEMP_ROOT_STRING(cx,str,tvr) \ JS_PUSH_TEMP_ROOT_COMMON(cx, str, tvr, JSTVU_SINGLE, string) #define JS_PUSH_TEMP_ROOT_FUNCTION(cx,fun,tvr) \ JS_PUSH_TEMP_ROOT_COMMON(cx, fun, tvr, JSTVU_SINGLE, function) #define JS_PUSH_TEMP_ROOT_QNAME(cx,qn,tvr) \ JS_PUSH_TEMP_ROOT_COMMON(cx, qn, tvr, JSTVU_SINGLE, qname) #define JS_PUSH_TEMP_ROOT_XML(cx,xml_,tvr) \ JS_PUSH_TEMP_ROOT_COMMON(cx, xml_, tvr, JSTVU_SINGLE, xml) #define JS_PUSH_TEMP_ROOT_TRACE(cx,trace_,tvr) \ JS_PUSH_TEMP_ROOT_COMMON(cx, trace_, tvr, JSTVU_TRACE, trace) #define JS_PUSH_TEMP_ROOT_SPROP(cx,sprop_,tvr) \ JS_PUSH_TEMP_ROOT_COMMON(cx, sprop_, tvr, JSTVU_SPROP, sprop) #define JS_PUSH_TEMP_ROOT_WEAK_COPY(cx,weakRoots_,tvr) \ JS_PUSH_TEMP_ROOT_COMMON(cx, weakRoots_, tvr, JSTVU_WEAK_ROOTS, weakRoots) #define JS_PUSH_TEMP_ROOT_PARSE_CONTEXT(cx,pc,tvr) \ JS_PUSH_TEMP_ROOT_COMMON(cx, pc, tvr, JSTVU_PARSE_CONTEXT, parseContext) #define JS_PUSH_TEMP_ROOT_SCRIPT(cx,script_,tvr) \ JS_PUSH_TEMP_ROOT_COMMON(cx, script_, tvr, JSTVU_SCRIPT, script)
Attachment #295633 -
Flags: review?(brendan)
Comment 2•16 years ago
|
||
Comment on attachment 295633 [details] [diff] [review] v1 Thanks, nits only: >+ * jsval, use JS_PUSH_TEMP_ROOT_<KIND>. The macros reinterprets an arbitrary "... macros reinterpret ..." >+ * The macros also provide a simple way to get a single pointer to rooted GC "single rooted pointer" >+#define JS_PUSH_TEMP_ROOT_COMMON(cx,x,tvr,cnt,fieldName) \ "mem" or "kind" (per <KIND>) instead of fieldName seems more consistently brief. >@@ -1070,18 +1027,18 @@ extern JSErrorFormatString js_ErrorForma > /* Relative operations weights. */ > #define JSOW_JUMP 1 > #define JSOW_ALLOCATION 100 > #define JSOW_LOOKUP_PROPERTY 5 > #define JSOW_GET_PROPERTY 10 > #define JSOW_SET_PROPERTY 20 > #define JSOW_NEW_PROPERTY 200 > #define JSOW_DELETE_PROPERTY 30 >-#define JSOW_ENTER_SHARP 4096 >-#define JSOW_SCRIPT_JUMP 4096 >+#define JSOW_ENTER_SHARP JS_OPERATION_WEIGHT_BASE >+#define JSOW_SCRIPT_JUMP JS_OPERATION_WEIGHT_BASE From another patch, which I thought landed already? r+a=me with nits picked. /be
Attachment #295633 -
Flags: review?(brendan)
Attachment #295633 -
Flags: review+
Attachment #295633 -
Flags: approval1.9+
Assignee | ||
Comment 3•16 years ago
|
||
(In reply to comment #2) > From another patch, which I thought landed already? I had to back it out once again due to Talos failures, see bug 409109 comment 21.
Assignee | ||
Comment 4•16 years ago
|
||
The new version fixes JS_PUSH_TEMP_ROOT_COMMON macro that was without () around the cnt parameter and rewrites comments to better reflect the reality with 5 out 12 PUSH macros rooting non-GC things.
Attachment #295633 -
Attachment is obsolete: true
Attachment #295760 -
Flags: review?(brendan)
Assignee | ||
Comment 5•16 years ago
|
||
This is a delta between v1 merged with the trunk and v2.
Assignee | ||
Comment 6•16 years ago
|
||
Attachment #295761 -
Attachment is obsolete: true
Comment 7•16 years ago
|
||
From comment 0 it sounds like we need a 1.8 fix, too.
Flags: wanted1.8.1.x+
Flags: blocking1.9?
Flags: blocking1.8.1.12?
Whiteboard: [sg:critical?] less severe on 1.8 branch?
Updated•16 years ago
|
Priority: -- → P2
Updated•16 years ago
|
Flags: blocking1.8.1.12? → blocking1.8.1.13?
Updated•16 years ago
|
Attachment #295760 -
Flags: review?(brendan)
Attachment #295760 -
Flags: review+
Attachment #295760 -
Flags: approval1.9+
Assignee | ||
Comment 8•16 years ago
|
||
Here is a trunk-synchronized version of the patch that I will check in.
Attachment #295760 -
Attachment is obsolete: true
Attachment #295763 -
Attachment is obsolete: true
Attachment #298040 -
Flags: review+
Assignee | ||
Comment 9•16 years ago
|
||
I checked in the patch from comment 8 to the trunk: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%252Fcvsroot&date=explicit&mindate=1200789899&maxdate=1200790081&who=igor%25mir2.org
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 10•16 years ago
|
||
Igor, do you have time to make a branch patch for this? Or does v3 apply cleanly?
Whiteboard: [sg:critical?] less severe on 1.8 branch? → [sg:critical?] [needs branch patch] less severe on 1.8 branch?
Assignee | ||
Comment 11•16 years ago
|
||
(In reply to comment #10 by Samuel Sidler) > Igor, do you have time to make a branch patch for this? Or does v3 apply > cleanly? > I plan to have a branch patch later today.
Assignee | ||
Comment 12•16 years ago
|
||
The essence of the fix in jsapi.c required a substantial hand-merge. The rest of changes is renaming and removal of non-existing on 1.8.1 branch features to adjust the re-factoring of TVR macros from the trunk. Strictly speaking this is not necessary, but it is rather safe and until 1.8.1 is supported I prefer to sync TVR-related code.
Attachment #298040 -
Attachment is obsolete: true
Attachment #303119 -
Flags: review?(brendan)
Assignee | ||
Comment 13•16 years ago
|
||
Comment 14•16 years ago
|
||
Comment on attachment 303119 [details] [diff] [review] v3 for the 1.8.1 branch r=me, thanks. /be
Attachment #303119 -
Flags: review?(brendan)
Attachment #303119 -
Flags: review+
Attachment #303119 -
Flags: approval1.8.1.13?
Updated•16 years ago
|
Flags: blocking1.8.1.13? → blocking1.8.1.13+
Comment 15•16 years ago
|
||
Comment on attachment 303119 [details] [diff] [review] v3 for the 1.8.1 branch approved for 1.8.1.13, a=dveditz for release-drivers
Attachment #303119 -
Flags: approval1.8.1.13? → approval1.8.1.13+
Assignee | ||
Comment 16•16 years ago
|
||
I checked in the patch from comment 12 to MOZILLA_1_8_BRANCH: http://bonsai.mozilla.org/cvsquery.cgi?module=AviarySuiteBranchTinderbox&branch=MOZILLA_1_8_BRANCH&cvsroot=%2Fcvsroot&date=explicit&mindate=1203104160&maxdate=1203104171&who=igor%25mir2.org
Keywords: fixed1.8.1.13
Updated•16 years ago
|
Flags: in-testsuite-
Flags: in-litmus-
Comment 17•16 years ago
|
||
Is there any way to verify this change in the release candidates for Firefox 2.0.0.13?
Assignee | ||
Comment 18•16 years ago
|
||
(In reply to comment #17) > Is there any way to verify this change in the release candidates for Firefox > 2.0.0.13? > I am not aware about a simple test case that can do that. It should be possible to write one, but I do not think that it is a productive use of time.
Updated•16 years ago
|
Flags: blocking1.8.0.15+
Comment 19•16 years ago
|
||
i tried to merge the 1.8 patch (attachment 303119 [details] [diff] [review]) down to 1.8.0. In addition to what the 1.8 version backported, I needed to backport the JS_STATIC_ASSERT macro.
Attachment #311259 -
Flags: review?(igor)
Comment 20•16 years ago
|
||
for your convenience ...
Assignee | ||
Comment 21•16 years ago
|
||
(In reply to comment #19) > Created an attachment (id=311259) [details] > for 1.8.0 > > i tried to merge the 1.8 patch (attachment 303119 [details] [diff] [review]) down to 1.8.0. In addition > to what the 1.8 version backported, I needed to backport the JS_STATIC_ASSERT > macro. In the past when backporting the code with JS_STATIC_ASSERT to 1.8.0 branch I simply was removing the macro. Some compilers had troubles with it so to avoid the risk of regressions with other embeddings the macro was not introduced on the branch. But I do not know if this is an issue now.
Assignee | ||
Updated•16 years ago
|
Attachment #311259 -
Flags: review?(igor) → review+
Comment 22•16 years ago
|
||
Comment on attachment 311259 [details] [diff] [review] for 1.8.0 taking this for distro patches, caillon please sign off.
Attachment #311259 -
Flags: approval1.8.0.15?
Comment 23•16 years ago
|
||
caillon, if you are experiencing the compiler issues mentioned in comment #21 on any of your supported platforms let me know. Maybe just strip the JS_STATIC_ASSERT macros before landing then.
Updated•16 years ago
|
Group: security
Comment 24•16 years ago
|
||
Comment on attachment 303119 [details] [diff] [review] v3 for the 1.8.1 branch Here's a potential problem that could cause crashes during GC: >@@ -4057,37 +4057,41 @@ JS_CompileUCFunctionForPrincipals(JSCont > fun = js_NewFunction(cx, NULL, NULL, nargs, 0, obj, funAtom); > if (!fun) > goto out; >+ >+ /* From this point the control must flow through the label out. */ >+ JS_PUSH_TEMP_ROOT_FUNCTION(cx, fun, &tvr); ... >+ out: >+ cx->weakRoots.newborn[GCX_PRIVATE] = (JSGCThing *) fun; >+ JS_POP_TEMP_ROOT(cx, &tvr); But in this case, we haven't pushed the temp root or the tvr or the temp root stack, which can cause all sorts of problems.
Comment 25•16 years ago
|
||
Comment on attachment 303119 [details] [diff] [review] v3 for the 1.8.1 branch >@@ -4107,17 +4111,22 @@ JS_CompileUCFunctionForPrincipals(JSCont > } > if (obj && funAtom) { > if (!OBJ_DEFINE_PROPERTY(cx, obj, ATOM_TO_JSID(funAtom), > OBJECT_TO_JSVAL(fun->object), > NULL, NULL, JSPROP_ENUMERATE, NULL)) { > return NULL; > } > } >-out: >+ >+ out: >+ cx->weakRoots.newborn[GCX_PRIVATE] = (JSGCThing *) fun; >+ JS_POP_TEMP_ROOT(cx, &tvr); Sorry for the spam, didn't see this before... the |return NULL| there really wants to be |fun = NULL|.
Comment 26•16 years ago
|
||
Attachment #303119 -
Attachment is obsolete: true
Comment 27•16 years ago
|
||
Attachment #311259 -
Attachment is obsolete: true
Attachment #311259 -
Flags: approval1.8.0.15?
Comment 28•16 years ago
|
||
Attachment #312220 -
Attachment is obsolete: true
Comment 29•16 years ago
|
||
Attachment #312221 -
Attachment is obsolete: true
Attachment #312222 -
Attachment is obsolete: true
Attachment #312223 -
Attachment is obsolete: true
Comment 30•16 years ago
|
||
bug 425576 ended up taking the next regression set.
Whiteboard: [sg:critical?] [needs branch patch] less severe on 1.8 branch? → [sg:critical?] [needs branch patch] 1.8 branch regression is bug 425576
Comment 31•16 years ago
|
||
Comment on attachment 312223 [details] [diff] [review] for 1.8.0 (out2, null fun instead of violating out constraints) per caillon we'll use this bug...
Attachment #312223 -
Attachment is obsolete: false
Attachment #312223 -
Flags: approval1.8.0.15?
Comment 32•16 years ago
|
||
Comment on attachment 312223 [details] [diff] [review] for 1.8.0 (out2, null fun instead of violating out constraints) a=caillon for 1.8.0.15
Attachment #312223 -
Flags: approval1.8.0.15? → approval1.8.0.15+
Updated•16 years ago
|
Comment 33•16 years ago
|
||
Comment on attachment 312223 [details] [diff] [review] for 1.8.0 (out2, null fun instead of violating out constraints) this patch appears not to include the latest from bug 425576 (v2) ... removing approval for now as this has not yet landed on 1.8.0 branch. timeless, can we just take the v2 patch on top of the original one we had here? or did you add something else?
Attachment #312223 -
Flags: approval1.8.0.15+ → approval1.8.0.15-
You need to log in
before you can comment on or make changes to this bug.
Description
•