Last Comment Bug 630209 - Unsafe usage of JS_Compile*/JS_ExecuteScript in shell and tests
: Unsafe usage of JS_Compile*/JS_ExecuteScript in shell and tests
Status: RESOLVED FIXED
fixed-in-tracemonkey
: dev-doc-needed
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 648208
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-31 08:22 PST by Igor Bukanov
Modified: 2011-04-06 22:46 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (110.80 KB, patch)
2011-02-05 12:14 PST, Igor Bukanov
jorendorff: review+
Details | Diff | Splinter Review
v2 (112.87 KB, patch)
2011-02-10 16:36 PST, Igor Bukanov
igor: review+
shaver: approval2.0-
Details | Diff | Splinter Review

Description Igor Bukanov 2011-01-31 08:22:25 PST
Our js shell implementations still contain code like (see http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/shell/xpcshell.cpp#491 for an example):

JSScript *script = JS_Compile...(cx,);
JS_ExecuteScript(cx, ..., script, ...);

This is GC-unsafe since nothing roots the script that is invisible to the conservative scanner.

One way to fix this would be to change JSScript that the public API exposes to be an object that wraps the script. Internally we would use ScriptData or something to denote the current JSScript. To stay fully source compatible with the current code the private slot of the script object would need to point to itself. This way the code that properly uses JS_GetScriptObject may continue to pass the private value to JS_ExecuteScript.

Alternatively we can break an API and return, say, JSScriptObject from the compilation routines to denote the script object. JS_ExecuteScript then would take this object as a parameter. This way we can also remove JS_GetScriptObject and JS_DestroyScript.
Comment 1 Brendan Eich [:brendan] 2011-01-31 08:30:33 PST
JS_ExecuteScript pushes a frame that roots the script. This is long-standing. What is the bug?

/be
Comment 2 Mike Shaver (:shaver -- probably not reading bugmail closely) 2011-01-31 08:36:07 PST
Between CompileScript and ExecuteScript, the script isn't rooted and is subject to collection, no?
Comment 3 Igor Bukanov 2011-01-31 09:08:22 PST
(In reply to comment #1)
> JS_ExecuteScript pushes a frame that roots the script. This is long-standing.
> What is the bug?

Before the frame is pushed the Execute function from jsinterp.cpp runs OBJ_TO_INNER_OBJECT that may potentially runs the GC. That can collect the script. It also reports an error for a non-native scope which may run the GC during the error construction making JSScript * invalid on the error path.

Although the consequences of these problems may not matter in practice, it is rather telling that we have not yet updated our shells and tests with proper rooting examples. Hence the idea of this bug to make the usage error-proof.

(In reply to comment #2)
> Between CompileScript and ExecuteScript, the script isn't rooted and is subject
> to collection, no?

It can be collected. So the only safe usage is to call ExecuteScript immediately and not to use the script after that.
Comment 4 Luke Wagner [:luke] 2011-01-31 09:19:44 PST
To simplify things for the API user, perhaps we could root the script object input at the JSAPI boundary?
Comment 5 Luke Wagner [:luke] 2011-01-31 09:22:00 PST
Actually, now that I look, Waldo seems to have added a AutoScriptRooter into the strict-mode-eval path of js::Execute.  Perhaps we could just hoist that.
Comment 6 Igor Bukanov 2011-01-31 11:54:35 PST
(In reply to comment #5)
> Actually, now that I look, Waldo seems to have added a AutoScriptRooter into
> the strict-mode-eval path of js::Execute.  Perhaps we could just hoist that.

We can do that, but given the amount of recent changes to the API I do not see a point of keeping easy to misuse forms of JS_Compile* around. After checking the code it seems the simplest thing to do would be to change the signature to return JSObject corresponding to the script wrapper, not JSScript, and modify JS_ExecuteScript to take that object.
Comment 7 Luke Wagner [:luke] 2011-01-31 13:01:17 PST
So then JSScript* could be removed from the JSAPI interface altogether?  That sounds awesome.  FWIW, JSScript/script-object subtleties are a common frustration.
Comment 8 Igor Bukanov 2011-01-31 13:48:49 PST
(In reply to comment #7)
> So then JSScript* could be removed from the JSAPI interface altogether? 

Not so fast! It is hard to remove JSScript due to its extensive use in the debug API. But it should be possible to remove its usage from jsapi.h so at least for the common Compile/Execute usage one would not need to worry about rooting and JS_GetScriptObject calls.
Comment 9 Luke Wagner [:luke] 2011-01-31 14:03:07 PST
(In reply to comment #8)
> (In reply to comment #7)
> > So then JSScript* could be removed from the JSAPI interface altogether? 
> 
> Not so fast! It is hard to remove JSScript due to its extensive use in the
> debug API.

Sure; I generally took "JSAPI interface" to mean jsapi.h, not jsdbgapi.h.
Comment 10 Igor Bukanov 2011-02-05 12:14:36 PST
Created attachment 510042 [details] [diff] [review]
v1

The patch changes JS_Compile* to return JSObject * that roots JSScript and changes JS_ExceuteScript to take this object. This allowed to remove JS_GetScriptObject/JS_NewScriptObject and to remove JSScript definition from the list of public types. With this the usage of JS_Compile* becomes noticeably simpler.
Comment 11 Brendan Eich [:brendan] 2011-02-05 12:27:50 PST
Every <script> in a page will now allocate an object for the script, where it didn't used to. We can hope that this will be in the noise, but I wouldn't be surprised to see it ding one of our talos tests. If you qref -m "try: -a" and then push to try, maybe you can find out.

/be
Comment 12 Jason Orendorff [:jorendorff] 2011-02-09 13:32:34 PST
I'll review this tomorrow. At first glance, I'm a fan.
Comment 13 Jason Orendorff [:jorendorff] 2011-02-10 09:39:10 PST
Comment on attachment 510042 [details] [diff] [review]
v1

The major concern here is changing the API this close to a release. More on that in a minute. Here are the review comments, all nits.

In jsd_xpc.cpp, jsdScript::CreatePPLineMap():
>-    if (scriptOwner)
>-        JS_DestroyScript (cx, script);
>+    /* Make sure that the script is rooted via scriptObj until the end of
>+     * script usage. */
>+    if (scriptObj)
>+        JS_AnchorPtr(scriptObj);

Please use jimb's JS::Anchor<JSObject *> here instead. It's basically the same
thing, but prettier. The class's destructor serves the purpose of this
JS_AnchorPtr call, forcing the value to remain in memory for the lifetime of
the JS::Anchor. jimb also used some magic so the destructor can be inlined.

In jsapi-tests/testScriptObject.cpp:
>-BEGIN_TEST(testScriptObject_ScriptlessScriptObjects)
>-{
>-    /* JS_NewScriptObject(cx, NULL) should return a fresh object each time. */
>-    jsvalRoot script_object1(cx, OBJECT_TO_JSVAL(JS_NewScriptObject(cx, NULL)));
>-    CHECK(!JSVAL_IS_PRIMITIVE(script_object1.value()));

Hooray!

In jsapi-tests/testTrap.cpp:
>-    JSObject *scrobj = JS_NewScriptObject(cx, script);
>+    JSObject *scrobj = JS_CompileScript(cx, global, source, strlen(source), __FILE__, 1);

Feel free to rename this to scriptObj while you're in here, if you like.

In jsapi-tests/testXDR.cpp:
>     CHECK(frozen);
>     memcpy(frozen, p, nbytes);
>     JS_XDRDestroy(w);
>-
>+        
>     // thaw
>     script = NULL;

Nit: You added a few spaces.

In jsapi.cpp, CompileUCScriptForPrincipalsCommon
>+    JSObject *scriptObject = NULL;

Maybe call this scriptObj like everywhere else?

In JS_CompileFile and JS_CompileFileHandleForPrincipals:
>+    if (script) {
>+        scriptObj = js_NewScriptObject(cx, script);
>+        if (!scriptObj)
>+            js_DestroyScript(cx, script);
>     }

While you're there, please move this repeated code into CompileFileHelper and
have it return a JSObject *.

Then reconsider the do-while(false) idiom in JS_CompileFile. I don't think it's
necessary, but you decide.
Comment 14 Jason Orendorff [:jorendorff] 2011-02-10 10:15:06 PST
Comment on attachment 510042 [details] [diff] [review]
v1

(In reply to comment #13)
> The major concern here is changing the API this close to a release. More on
> that in a minute.

Jim just announced a major breaking change in mozilla.dev.tech.js-engine and all hell doesn't seem to have broken loose, but assuming this gets approval, please post about it there and in mozilla.dev.platform. Make it clear that this will break essentially all embeddings using the JSAPI.

Jim's change is necessary for ES5 compliance. This is a GC safety thing, which I rate as even more important given the frequency at which we keep making this mistake. So I would approve this for Firefox 4 if it were me.

r=me, setting approval2.0?
Comment 15 Igor Bukanov 2011-02-10 16:36:21 PST
Created attachment 511563 [details] [diff] [review]
v2

The new version addresses the nits.
Comment 16 Brendan Eich [:brendan] 2011-02-11 11:45:33 PST
(In reply to comment #11)
> Every <script> in a page will now allocate an object for the script, 

But we took this hit already, didn't we? Bug 438633.

This patch is probably less breaking than the JSPropertyOp -> JSStrictPropertyOp one from the amo and fbug reading I've done. But this bug is not a blocker, or at least, I can't make that case. Is someone going to try?

/be
Comment 17 Igor Bukanov 2011-02-11 13:38:35 PST
(In reply to comment #16)
> (In reply to comment #11)
> > Every <script> in a page will now allocate an object for the script, 

<script> tag implementation uses JS_Evaluate which avoids an extra object overhead.

> 
> But we took this hit already, didn't we? Bug 438633.

Yes, the patch effectively removes an extra awkwardness of using GetScriptObject/GetPrivate from API users that bug 438633 landed in August forced on users of any non-trivial Compile/Execute pair.

As for the blocking case consider that with this bug it would be easier to adjust the code to account for bug 438633 changes.
Comment 18 David Mandelin [:dmandelin] 2011-02-15 18:53:48 PST
(In reply to comment #14)
> This is a GC safety thing, which I rate as even more important given the 
> frequency at which we keep making this mistake. So I would approve this for 
> Firefox 4 if it were me.

The patch is big, but the changes seem fairly mechanical so it seems reasonable. I would say that if it's only for the shells (as comment 0 sort-of implies), landing it has only downside for the release, but if it helps Firefox then it seems safe enough that we should give it a shot. So, does it help Firefox too?

If we do decide to land this, I think we should be sure to land it by Thursday so it gets in the next beta.
Comment 19 Mike Shaver (:shaver -- probably not reading bugmail closely) 2011-02-15 18:58:02 PST
Comment on attachment 511563 [details] [diff] [review]
v2

I don't think that we should take this before we ship FF4.  Blockers go in, everything else waits.  Risk and opportunity calculus are very conservative, and we want everyone working on (patching, reviewing, testing, bisecting, etc.) blockers.
Comment 21 Chris Leary [:cdleary] (not checking bugmail) 2011-03-31 14:44:43 PDT
http://hg.mozilla.org/mozilla-central/rev/c919a7271ac1

Note You need to log in before you can comment on or make changes to this bug.