Last Comment Bug 631467 - NULL dereference of jsdc->dumbContext
: NULL dereference of jsdc->dumbContext
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
: jsd
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2011-02-04 00:08 PST by Steve Fink [:sfink] [:s:]
Modified: 2011-07-08 00:24 PDT (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Fix null deref, eliminate dead code (1.88 KB, patch)
2011-02-04 00:14 PST, Steve Fink [:sfink] [:s:]
no flags Details | Diff | Splinter Review
Avoid NULL deref on error, leave compartment on error (1.87 KB, patch)
2011-02-04 00:38 PST, Steve Fink [:sfink] [:s:]
gal: review+
Details | Diff | Splinter Review

Description Steve Fink [:sfink] [:s:] 2011-02-04 00:08:49 PST
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:] 2011-02-04 00:14:44 PST
Created attachment 509700 [details] [diff] [review]
Fix null deref, eliminate dead code
Comment 2 Andreas Gal :gal 2011-02-04 00:24:18 PST
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:] 2011-02-04 00:31:32 PST
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:] 2011-02-04 00:38:47 PST
Created attachment 509703 [details] [diff] [review]
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 2011-02-04 00:54:43 PST
Comment on attachment 509703 [details] [diff] [review]
Avoid NULL deref on error, leave compartment on error

Looks good.
Comment 6 Steve Fink [:sfink] [:s:] 2011-03-03 10:53:19 PST
Comment 7 Steve Fink [:sfink] [:s:] 2011-04-28 14:01:36 PDT

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