Closed Bug 411025 Opened 16 years ago Closed 16 years ago

GC hazard in JS_CompileUCFunctionForPrincipals

Categories

(Core :: JavaScript Engine, defect, P2)

defect

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)

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.
Attached patch v1 (obsolete) — Splinter Review
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 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+
(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.
Attached patch v2 (obsolete) — Splinter Review
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)
Attached patch v1-v2 delta (obsolete) — Splinter Review
This is a delta between v1 merged with the trunk and v2.
Attached patch v1-v2 delta for real (obsolete) — Splinter Review
Attachment #295761 - Attachment is obsolete: true
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?
Priority: -- → P2
Flags: blocking1.8.1.12? → blocking1.8.1.13?
Attachment #295760 - Flags: review?(brendan)
Attachment #295760 - Flags: review+
Attachment #295760 - Flags: approval1.9+
Attached patch v3 (obsolete) — Splinter Review
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+
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
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?
(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.
Attached patch v3 for the 1.8.1 branch (obsolete) — Splinter Review
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)
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?
Flags: blocking1.8.1.13? → blocking1.8.1.13+
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+
Flags: in-testsuite-
Flags: in-litmus-
Is there any way to verify this change in the release candidates for Firefox 2.0.0.13?
(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.
Flags: blocking1.8.0.15+
Attached patch for 1.8.0 (obsolete) — Splinter Review
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)
for your convenience ...
(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.
Attachment #311259 - Flags: review?(igor) → review+
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?
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.
Group: security
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 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|.
Attached patch use the right out label (obsolete) — Splinter Review
Attachment #303119 - Attachment is obsolete: true
Attached patch for 1.8.0 (obsolete) — Splinter Review
Attachment #311259 - Attachment is obsolete: true
Attachment #311259 - Flags: approval1.8.0.15?
Attached patch v3.02 for the 1.8.1 branch (obsolete) — Splinter Review
Attachment #312220 - Attachment is obsolete: true
Blocks: 425576
Attachment #312222 - Attachment is obsolete: true
Attachment #312223 - Attachment is obsolete: true
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 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 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+
No longer blocks: 425576
Depends on: 425576
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.