GC hazard in JS_CompileUCFunctionForPrincipals

RESOLVED FIXED

Status

()

Core
JavaScript Engine
P2
normal
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({fixed1.8.1.13})

Trunk
fixed1.8.1.13
Points:
---
Bug Flags:
blocking1.9 ?
blocking1.8.1.13 +
wanted1.8.1.x +
blocking1.8.0.next +
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] [needs branch patch] 1.8 branch regression is bug 425576)

Attachments

(3 attachments, 10 obsolete attachments)

(Assignee)

Description

10 years ago
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

10 years ago
Created attachment 295633 [details] [diff] [review]
v1

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

Comment 3

10 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

10 years ago
Created attachment 295760 [details] [diff] [review]
v2

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

10 years ago
Created attachment 295761 [details] [diff] [review]
v1-v2 delta

This is a delta between v1 merged with the trunk and v2.
(Assignee)

Comment 6

10 years ago
Created attachment 295763 [details] [diff] [review]
v1-v2 delta for real
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?

Updated

10 years ago
Attachment #295760 - Flags: review?(brendan)
Attachment #295760 - Flags: review+
Attachment #295760 - Flags: approval1.9+
(Assignee)

Comment 8

10 years ago
Created attachment 298040 [details] [diff] [review]
v3

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

10 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
Last Resolved: 10 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?
(Assignee)

Comment 11

10 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

10 years ago
Created attachment 303119 [details] [diff] [review]
v3 for the 1.8.1 branch

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

10 years ago
Created attachment 303122 [details] [diff] [review]
plain diff of trunk and 181 patches
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+
(Assignee)

Comment 16

10 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

10 years ago
Flags: in-testsuite-
Flags: in-litmus-
Is there any way to verify this change in the release candidates for Firefox 2.0.0.13?
(Assignee)

Comment 18

9 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

9 years ago
Flags: blocking1.8.0.15+

Comment 19

9 years ago
Created attachment 311259 [details] [diff] [review]
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.
Attachment #311259 - Flags: review?(igor)

Comment 20

9 years ago
Created attachment 311260 [details] [diff] [review]
1.8 to 1.8.0 plaindiff

for your convenience ...
(Assignee)

Comment 21

9 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

9 years ago
Attachment #311259 - Flags: review?(igor) → review+

Comment 22

9 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

9 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.
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|.

Comment 26

9 years ago
Created attachment 312220 [details] [diff] [review]
use the right out label
Attachment #303119 - Attachment is obsolete: true

Comment 27

9 years ago
Created attachment 312221 [details] [diff] [review]
for 1.8.0
Attachment #311259 - Attachment is obsolete: true
Attachment #311259 - Flags: approval1.8.0.15?

Comment 28

9 years ago
Created attachment 312222 [details] [diff] [review]
v3.02 for the 1.8.1 branch
Attachment #312220 - Attachment is obsolete: true

Comment 29

9 years ago
Created attachment 312223 [details] [diff] [review]
for 1.8.0 (out2, null fun instead of violating out constraints)
Attachment #312221 - Attachment is obsolete: true

Updated

9 years ago
Blocks: 425576

Updated

9 years ago
Attachment #312222 - Attachment is obsolete: true

Updated

9 years ago
Attachment #312223 - Attachment is obsolete: true

Comment 30

9 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

9 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 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 33

9 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.