As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 498543 - Audit users of JS_THIS_OBJECT in SpiderMonkey for correctly null-checking the result
: Audit users of JS_THIS_OBJECT in SpiderMonkey for correctly null-checking the...
Status: RESOLVED FIXED
[good first bug]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Felix Fung (:felix)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 408416
  Show dependency treegraph
 
Reported: 2009-06-15 18:47 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-12-02 06:34 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Null-checking JS_THIS_OBJECT Results (21.93 KB, patch)
2011-09-28 00:31 PDT, Felix Fung (:felix)
jwalden+bmo: review-
Details | Diff | Splinter Review
Null-checking JS_THIS_OBJECT Results (21.36 KB, patch)
2011-11-16 15:34 PST, Felix Fung (:felix)
jwalden+bmo: review+
Details | Diff | Splinter Review

Description User image Jeff Walden [:Waldo] (remove +bmo to email) 2009-06-15 18:47:25 PDT
I see at least a handful of users who aren't null-checking, even passing on the result to methods that I predict don't expect a null-check.
Comment 1 User image Wesley W. Garland 2009-06-15 18:50:32 PDT
Guilty here. Thought it was infallible. Shame on me! Docs are quite clear in this regard.
Comment 2 User image Jeff Walden [:Waldo] (remove +bmo to email) 2009-06-15 18:58:15 PDT
Another fun little wrinkle: assuming JS_InstanceOf is null-safe doesn't work, because that appears capable of double-reporting with certain methods of use (when the argv passed to JS_InstanceOf is non-null), and it appears we have some such uses.
Comment 3 User image Brendan Eich [:brendan] 2009-06-15 20:51:26 PDT
(In reply to comment #2)
> Another fun little wrinkle: assuming JS_InstanceOf is null-safe doesn't work,
> because that appears capable of double-reporting with certain methods of use
> (when the argv passed to JS_InstanceOf is non-null), and it appears we have
> some such uses.

What double-reporting? It's an old API "feature" that passing non-null argv to JS_InstanceOf helps it report the error for the caller, and with better blame.

Good catch on thisObject fallibility. My secret shame! It's all "with"'s fault. We should try to get rid of thisObject if we can.

/be
Comment 4 User image Brendan Eich [:brendan] 2009-06-15 20:53:16 PDT
Comment 3 is talking about OBJ_THIS_OBJECT and JSObjectOps.thisObject, of course.

JS_THIS/JS_THIS_OBJECT/JS_ComputeThis are a whole rerun of that bad old show, for JSFastNatives. Change the channel!

/be
Comment 5 User image Jeff Walden [:Waldo] (remove +bmo to email) 2009-06-15 22:53:55 PDT
JS_THIS_OBJECT returns null, but before it does so it must report the error -- OOM, say, for a this-object to wrap a JSString.  Now consider passing that in to JS_InstanceOf with a non-null vp.  If you do that, then you hit an arm that tries to convert the callee value to a function (which could fail, of course) to report a nice error message.  Why isn't that second report a double-report?  What am I missing?
Comment 6 User image Brendan Eich [:brendan] 2009-06-15 23:11:29 PDT
You're right -- that's a failure to check for failure :-P.

I thought you were saying there's a double-report under JS_InstanceOf, but you're right that treating JS_THIS_OBJECT, or any API, as infallible is a bug that can result in double error reports/exceptions.

If the first API called is really fallible, the embedding has to check, no way around it short of C++ exceptions.

Trying to suppress second report due to cx->throwing is the way to madness: it will hide bugs and break legitimate code paths (generators, e.g.). Just noting, not suggesting you proposed this.

/be
Comment 7 User image Jason Orendorff [:jorendorff] 2009-10-19 07:35:19 PDT
Bug 515496 could make JS_THIS_OBJECT infallible.
Comment 8 User image Felix Fung (:felix) 2011-09-28 00:31:24 PDT
Created attachment 562996 [details] [diff] [review]
Null-checking JS_THIS_OBJECT Results
Comment 9 User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-16 10:20:52 PST
Comment on attachment 562996 [details] [diff] [review]
Null-checking JS_THIS_OBJECT Results

Review of attachment 562996 [details] [diff] [review]:
-----------------------------------------------------------------

I despise bracing single-line ifs like that, but I guess the style guidelines wrongly say to do that now.  :-(

I think you might be missing a new instance in dom/workers/Events.cpp, solely going by the count of results from a grep there, and the count of changes you've made to it.  I think you're missing a fair number more in dom/workers/WorkerScope.cpp.

I should probably look at this again, due to the combination of new instances in the previous paragraph and the xpconnect files having been moved and renamed between the creation of this patch and now.
Comment 10 User image Felix Fung (:felix) 2011-11-16 15:34:11 PST
Created attachment 575019 [details] [diff] [review]
Null-checking JS_THIS_OBJECT Results

- Updated patch
- Note: WorkerScope.cpp seems to have been taken care of...
Comment 11 User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-30 14:52:53 PST
Comment on attachment 575019 [details] [diff] [review]
Null-checking JS_THIS_OBJECT Results

Review of attachment 575019 [details] [diff] [review]:
-----------------------------------------------------------------

js/xpconnect/wrappers/XrayWrapper.cpp seems to have a call that doesn't get null-checked, but other than that, this looks fine.  Please fix that one before you push?  No need for another review just for that, I think.
Comment 13 User image Marco Bonardo [::mak] 2011-12-02 03:21:59 PST
https://hg.mozilla.org/mozilla-central/rev/c2e914523306
Comment 14 User image :Ms2ger (⌚ UTC+1/+2) 2011-12-02 06:15:10 PST
Why are some of these returning false and some true?
Comment 15 User image Felix Fung (:felix) 2011-12-02 06:34:51 PST
The only ones returning true are those in dom/workers/EventTarget.cpp and I happened to follow the return value used for a failed GetPrivate.

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