631467 - NULL dereference of jsdc->dumbContext

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

In _newJSDContext() inside of jsd_high.c, there are a couple of issues:

1. If JS_NewContext() fails and returns NULL, we'll goto label_newJSDContext_failure and end up calling JS_EndRequest(NULL), which will do a NULL ptr dereference.

2. The |if( call )| test is redundant. If call were NULL, we would've already jumped to the error label.

3. call = JS_EnterCrossCompartmentCall(jsdc->dumbContext, jsdc->glob) is a no-op. jsdc->dumbContext is already guaranteed to be in the same compartment as jsdc->glob, since the compartment and global were just created a few lines earlier. This comes from bug 600580.

The only thing I see that could possibly be in a foreign compartment here is scopeobj, but currently it's unused. Also, all callers with js/jsd pass in NULL. Smells like dead code?

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Fix null deref, eliminate dead code

Comment 2

[Andreas Gal :gal]

Comment on attachment 509700 [details] [diff] [review]
Fix null deref, eliminate dead code

Why is this dead code? What enters the compartment for JS_InitStandardClases?

Comment 3

[Steve Fink [:sfink] [:s:]]

Well, if you're asking the question, then it probably isn't dead code. So let me try to explain my assumptions:

There's only one compartment involved here. It is created with the line

    jsdc->glob = JS_NewCompartmentAndGlobalObject(jsdc->dumbContext, &global_class, NULL);

I assume that that means that jsdc->dumbContext will be initialized to that compartment and never leave it. Is that incorrect? Will jsdc->dumbContext only enter a compartment if something else happens?

...oh. Looking at the code, JS_NewCompartmentAndGlobalObject does not affect the preexisting cx->compartment. Um, ok, help me out here -- how do contexts initially enter/get assigned a compartment? Someday I'd like to understand this stuff.

But you're right, that isn't dead code. I'll update the patch to just deal with the NULL deref.

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: Avoid NULL deref on error, leave compartment on error

Ok, I *think* I have the logic right. If so, this should fix the NULL deref problem, and also leaves the compartment if JS_InitStandardClasses fails. I don't know if the latter is even necessary, since the whole context is getting thrown out anyway.

Comment 5

[Andreas Gal :gal]

Comment on attachment 509703 [details] [diff] [review]
Avoid NULL deref on error, leave compartment on error

Looks good.

Comment 6

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/tracemonkey/rev/b8c39d2a847a

Comment 7

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/mozilla-central/rev/b8c39d2a847a