Closed Bug 631467 Opened 9 years ago Closed 9 years ago

NULL dereference of jsdc->dumbContext


(Core :: JavaScript Engine, defect)

Not set





(Reporter: sfink, Unassigned)


(Whiteboard: [fixed-in-tracemonkey])


(1 file, 1 obsolete file)

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?
Attachment #509700 - Flags: review?(gal)
Summary: NULL reference of jsdc->dumbContext → NULL dereference of jsdc->dumbContext
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?
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.
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.
Attachment #509700 - Attachment is obsolete: true
Attachment #509703 - Flags: review?(gal)
Attachment #509700 - Flags: review?(gal)
Comment on attachment 509703 [details] [diff] [review]
Avoid NULL deref on error, leave compartment on error

Looks good.
Attachment #509703 - Flags: review?(gal) → review+
Whiteboard: [fixed-in-tracemonkey]
Closed: 9 years ago
Resolution: --- → FIXED
Component: JavaScript Debugging/Profiling APIs → JavaScript Engine
You need to log in before you can comment on or make changes to this bug.