Closed Bug 631467 Opened 9 years ago Closed 9 years ago
NULL dereference of jsdc->dumb
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?
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.
Comment on attachment 509703 [details] [diff] [review] Avoid NULL deref on error, leave compartment on error Looks good.
Attachment #509703 - Flags: review?(gal) → review+
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.