Note: There are a few cases of duplicates in user autocompletion which are being worked on.

NULL dereference of jsdc->dumbContext

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: sfink, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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?
(Reporter)

Comment 1

7 years ago
Created attachment 509700 [details] [diff] [review]
Fix null deref, eliminate dead code
Attachment #509700 - Flags: review?(gal)
(Reporter)

Updated

7 years ago
Summary: NULL reference of jsdc->dumbContext → NULL dereference of jsdc->dumbContext

Comment 2

7 years ago
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?
(Reporter)

Comment 3

7 years ago
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.
(Reporter)

Comment 4

7 years ago
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.
Attachment #509700 - Attachment is obsolete: true
Attachment #509703 - Flags: review?(gal)
Attachment #509700 - Flags: review?(gal)

Comment 5

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

Looks good.
Attachment #509703 - Flags: review?(gal) → review+
(Reporter)

Comment 6

7 years ago
http://hg.mozilla.org/tracemonkey/rev/b8c39d2a847a
Whiteboard: [fixed-in-tracemonkey]
(Reporter)

Comment 7

6 years ago
http://hg.mozilla.org/mozilla-central/rev/b8c39d2a847a
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Component: JavaScript Debugging/Profiling APIs → JavaScript Engine
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.