Bug 146785 – toJSDOMWindow() does not handle objects that descend from the JS DOM Window (crashes on use)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

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.

[Alexey Proskuryakov]

Ugh!

[Radar WebKit Bug Importer]

<rdar://problem/21764288>

FYI: I have a fix and regression test ready for review, but I won't be able to post it until Monday.

Created attachment 256676 [details]
Patch

[Mark Lam]

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();

[Mark Lam]

(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.

[Mark Lam]

Comment on attachment 256676 [details]
Patch

r=me with fixes.

[Mark Lam]

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

Created attachment 256767 [details]
Patch

Created attachment 256768 [details]
Patch

[Mark Lam]

Comment on attachment 256768 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 256768 [details]
Patch

Clearing flags on attachment: 256768

Committed r187165: <http://trac.webkit.org/changeset/187165>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

I think this changes the meaning of the toJSDOMWindow function, and so it should be renamed.

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.

[Darin Adler]

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.

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.

[Darin Adler]

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.

[Alexey Shvayka]

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.