Bug 146785 - toJSDOMWindow() does not handle objects that descend from the JS DOM Window (crashes on use)
Summary: toJSDOMWindow() does not handle objects that descend from the JS DOM Window (...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-07-09 08:21 PDT by mark.s.dittmer
Modified: 2021-05-17 09:40 PDT (History)
10 users (show)

See Also:


Attachments
Patch (3.75 KB, patch)
2015-07-11 20:28 PDT, mark.s.dittmer
no flags Details | Formatted Diff | Diff
Patch (3.72 KB, patch)
2015-07-14 06:18 PDT, mark.s.dittmer
no flags Details | Formatted Diff | Diff
Patch (3.72 KB, patch)
2015-07-14 06:29 PDT, mark.s.dittmer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mark.s.dittmer 2015-07-09 08:21:35 PDT
toJSDOMWindow() expects to be passed JSValue that is a JSDOMWindow, but it is invoked in contexts where the value may be some other Javascript object that hs the JS DOM Window in its prototype chain.

To reproduce, attempt to run the following line of Javascript:

Object.create(window).location;

The ensuing crash can be traced back to oJSDOMWindow() returning 0 (or NULL) in a context where it shouldn't because the JS DOM Window object can be readily looked up by walking the prototype chain of the Object.create-ed object.
Comment 1 Alexey Proskuryakov 2015-07-10 04:29:13 PDT
Ugh!
Comment 2 Radar WebKit Bug Importer 2015-07-10 04:29:34 PDT
<rdar://problem/21764288>
Comment 3 mark.s.dittmer 2015-07-10 05:27:06 PDT
FYI: I have a fix and regression test ready for review, but I won't be able to post it until Monday.
Comment 4 mark.s.dittmer 2015-07-11 20:28:33 PDT
Created attachment 256676 [details]
Patch
Comment 5 Mark Lam 2015-07-13 10:24:00 PDT
Comment on attachment 256676 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256676&action=review

Some suggestions below.  I’m also not convinced yet that this fix should be implemented in the toJSDOMWindow() function.  I’m currently researching the issue.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:253
> +        const ClassInfo* classInfo = asObject(value)->classInfo();

You’ve already computed the JSObject* above.  You can simplify this to:
    const ClassInfo* classInfo = object->classInfo();

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:255
> +            return jsCast<JSDOMWindow*>(asObject(value));

Ditto.  You can simplify this to:
    return jsCast<JSDOMWindow*>(object);

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:257
> +            return jsCast<JSDOMWindowShell*>(asObject(value))->window();

Ditto.  Simplify to:
    return jsCast<JSDOMWindowShell*>(object)->window();
Comment 6 Mark Lam 2015-07-13 15:16:32 PDT
(In reply to comment #5)
> Comment on attachment 256676 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256676&action=review
> 
> Some suggestions below.  I’m also not convinced yet that this fix should be
> implemented in the toJSDOMWindow() function.  I’m currently researching the
> issue.

OK, I’ve changed my mind.  We get to toJSDOMWindow() via jsDOMWindowLocation().  Hence, it is fair to expect toJSDOMWindow() to look up the window object.
Comment 7 Mark Lam 2015-07-13 15:17:15 PDT
Comment on attachment 256676 [details]
Patch

r=me with fixes.
Comment 8 Mark Lam 2015-07-13 15:20:44 PDT
For the record, here's the crashing stack trace:

(lldb) bt
* thread #1: tid = 0x24d88, 0x0000000115500f80 WebCore`WTF::RefPtr<WebCore::DOMWindow>::operator*(this=0x0000000000000598) const + 16 at RefPtr.h:68, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x598)
    frame #0: 0x0000000115500f80 WebCore`WTF::RefPtr<WebCore::DOMWindow>::operator*(this=0x0000000000000598) const + 16 at RefPtr.h:68
    frame #1: 0x0000000115500f5c WebCore`WebCore::JSDOMWindowBase::impl(this=0x0000000000000000) const + 28 at JSDOMWindowBase.h:48
    frame #2: 0x00000001155005c5 WebCore`WebCore::JSDOMWindow::impl(this=0x0000000000000000) const + 21 at JSDOMWindow.h:93
  * frame #3: 0x00000001157386eb WebCore`WebCore::jsDOMWindowLocation(exec=0x00007fff56cdf820, slotBase=0x000000011c86b100, thisValue=4774284352, (null)=PropertyName at 0x00007fff56cdf598) + 59 at JSDOMWindow.cpp:6078
    frame #4: 0x00000001125e6298 JavaScriptCore`JSC::PropertySlot::getValue(this=0x00007fff56cdf730, exec=0x00007fff56cdf820, propertyName=PropertyName at 0x00007fff56cdf5f0) const + 184 at PropertySlot.h:257
    frame #5: 0x00000001125e607b JavaScriptCore`JSC::JSValue::get(this=0x00007fff56cdf768, exec=0x00007fff56cdf820, propertyName=PropertyName at 0x00007fff56cdf640, slot=0x00007fff56cdf730) const + 91 at JSCJSValueInlines.h:704
    frame #6: 0x0000000112d996a1 JavaScriptCore`::llint_slow_path_get_by_id(exec=0x00007fff56cdf820, pc=0x000000011c9155c0) + 241 at LLIntSlowPaths.cpp:570
    frame #7: 0x0000000112da78d0 JavaScriptCore`llint_entry + 11668

This is attained by simply evaluating the following line in the Inspector console:

    Object.create(window).location
Comment 9 mark.s.dittmer 2015-07-14 06:18:54 PDT
Created attachment 256767 [details]
Patch
Comment 10 mark.s.dittmer 2015-07-14 06:29:55 PDT
Created attachment 256768 [details]
Patch
Comment 11 Mark Lam 2015-07-14 09:09:52 PDT
Comment on attachment 256768 [details]
Patch

r=me
Comment 12 WebKit Commit Bot 2015-07-22 10:22:34 PDT
Comment on attachment 256768 [details]
Patch

Clearing flags on attachment: 256768

Committed r187165: <http://trac.webkit.org/changeset/187165>
Comment 13 WebKit Commit Bot 2015-07-22 10:22:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Darin Adler 2015-07-22 14:03:05 PDT
I think this changes the meaning of the toJSDOMWindow function, and so it should be renamed.
Comment 15 mark.s.dittmer 2015-07-23 07:04:36 PDT
Darin, any suggestions? It seems to me that the role of the function is still to cast a value to a JSDOMWindow, so the name seems appropriate to me.
Comment 16 Darin Adler 2015-07-23 15:58:38 PDT
I’m not entirely sure.

I suppose that given how JavaScript works, the concept that an object that eventually in a prototype chain points to a DOM window *is* a DOM window, and a search of the prototype chain can be treated as a cast, is an OK concept.

This makes me worry that there are other cases like this, where we are checking the internal C++ class of what backs "this" objects in various DOM functions but we should really be searching the prototype chains of those objects instead. These are probably in other functions named toXXX, where XXX is something other than DOM window.
Comment 17 mark.s.dittmer 2015-07-24 05:46:25 PDT
That seems like a legitimate concern. I'm not very familiar with the codebase. If you are suggesting that the subroutine of checking the prototype chain for some condition be abstracted (and given a different name), then that seems like a good idea.
Comment 18 Darin Adler 2015-07-26 20:24:53 PDT
I’m suggesting we create some tests demonstrating the same issue on other DOM elements; I don’t think it was good to fix this only for the DOM window given the same issue exists for everything! Once we have some tests I’m sure we can quickly fix the problem.

The name of the function is not the important issue, despite the fact that it was the first thing I commented on.

If you’d like to make those tests, I would be grateful, but if not someone else contributing to WebKit should do it.
Comment 19 Alexey Shvayka 2021-05-17 09:40:36 PDT
Thank you for fixing the crash, Mark!

Per spec (step 1.1.2.3 of https://heycam.github.io/webidl/#dfn-attribute-getter), there is no prototype chain traversal.
`Object.create(window).location` should throw TypeError, like Gecko and Blink do.

Since 2015, WebIDL bindings were significantly improved, so https://bugs.webkit.org/show_bug.cgi?id=223758 just drops the prototype chain lookup.
Also, it's not safe & correct to perform getPrototypeDirect() unconditionally.