Bug 149311 – Null dereference loading Blink layout test http/tests/websocket/construct-in-detached-frame.html

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Jon Honeycutt]

Created attachment 261448 [details]
crashing test

Null dereference loading Blink layout test http/tests/websocket/construct-in-detached-frame.html.

Stack trace:

Crashed Thread:        0  Dispatch queue: com.apple.main-thread

Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000338
Exception Note:        EXC_CORPSE_NOTIFY

VM Regions Near 0x338:
--> 
    __TEXT                 00000001072d6000-00000001072d8000 [    8K] r-x/rwx SM=COW  /Users/USER/*/WebKit.framework/Versions/A/XPCServices/com.apple.WebKit.WebContent.Development.xpc/Contents/MacOS/com.apple.WebKit.WebContent.Development

Application Specific Information:
CRASHING TEST: temp-tests/http/tests/websocket/construct-in-detached-frame.html

Global Trace Buffer (reverse chronological seconds):
36.717854    CFNetwork                 	0x00007fff88d43b97 Explicitly setting CF cookie storage singleton
36.718200    CFNetwork                 	0x00007fff88d8f211 Explicitly setting cookie storage singleton

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x000000010ffb4b91 WebCore::WebSocket::connect(WTF::String const&, WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul> const&, int&) + 673 (memory:2635)
1   com.apple.WebCore             	0x000000010ffb487b WebCore::WebSocket::create(WebCore::ScriptExecutionContext&, WTF::String const&, WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul> const&, int&) + 427 (StdLibExtras.h:366)
2   com.apple.WebCore             	0x000000010f979302 WebCore::JSWebSocketConstructor::constructJSWebSocket1(JSC::ExecState*) + 322 (JSWebSocket.cpp:154)
3   com.apple.JavaScriptCore      	0x000000010e9afde8 JSC::LLInt::setUpCall(JSC::ExecState*, JSC::Instruction*, JSC::CodeSpecializationKind, JSC::JSValue, JSC::LLIntCallLinkInfo*) + 360 (LLIntSlowPaths.cpp:1107)
4   com.apple.JavaScriptCore      	0x000000010e9b686b llint_entry + 22948
5   com.apple.JavaScriptCore      	0x000000010e9b0ce4 vmEntryToJavaScript + 299
6   com.apple.JavaScriptCore      	0x000000010e8712d9 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 169 (JITCode.cpp:82)
7   com.apple.JavaScriptCore      	0x000000010e857a10 JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) + 10448 (Interpreter.cpp:945)
8   com.apple.JavaScriptCore      	0x000000010e56a4c5 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 469 (Completion.cpp:104)
9   com.apple.WebCore             	0x000000010fccd8ec WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&) + 284 (JSMainThreadExecState.h:62)
10  com.apple.WebCore             	0x000000010fccdb29 WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&) + 41 (ScriptController.cpp:180)
11  com.apple.WebCore             	0x000000010fcd3aac WebCore::ScriptElement::executeScript(WebCore::ScriptSourceCode const&) + 316 (ScriptElement.cpp:309)
12  com.apple.WebCore             	0x000000010fcd2756 WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport) + 1046 (StdLibExtras.h:366)
13  com.apple.WebCore             	0x000000010f4cf5eb WebCore::HTMLScriptRunner::runScript(WebCore::Element*, WTF::TextPosition const&) + 347 (ScriptElement.h:58)
14  com.apple.WebCore             	0x000000010f4cf440 WebCore::HTMLScriptRunner::execute(WTF::PassRefPtr<WebCore::Element>, WTF::TextPosition const&) + 48 (HTMLScriptRunner.cpp:191)
15  com.apple.WebCore             	0x000000010f472466 WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder() + 86 (StdLibExtras.h:366)
16  com.apple.WebCore             	0x000000010f47252d WebCore::HTMLDocumentParser::canTakeNextToken(WebCore::HTMLDocumentParser::SynchronousMode, WebCore::PumpSession&) + 93 (HTMLDocumentParser.cpp:214)
17  com.apple.WebCore             	0x000000010f4720c3 WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) + 595 (HTMLDocumentParser.cpp:259)
18  com.apple.WebCore             	0x000000010f472ddd WebCore::HTMLDocumentParser::append(WTF::PassRefPtr<WTF::StringImpl>) + 669 (DocumentParser.h:71)
19  com.apple.WebCore             	0x000000010f21561c WebCore::DecodedDataDocumentParser::flush(WebCore::DocumentWriter&) + 92 (StdLibExtras.h:366)
20  com.apple.WebCore             	0x000000010f27568b WebCore::DocumentWriter::end() + 43 (RefPtr.h:71)
21  com.apple.WebCore             	0x000000010f25d9ec WebCore::DocumentLoader::finishedLoading(double) + 268 (ResourceErrorBase.h:42)
22  com.apple.WebCore             	0x000000010f08e179 WebCore::CachedResource::checkNotify() + 153 (CachedResourceClientWalker.h:51)
23  com.apple.WebCore             	0x000000010f08a433 WebCore::CachedRawResource::finishLoading(WebCore::SharedBuffer*) + 227 (CachedRawResource.cpp:104)
24  com.apple.WebCore             	0x000000010fe05501 WebCore::SubresourceLoader::didFinishLoading(double) + 1153 (ResourceLoader.h:154)
25  com.apple.WebKit              	0x000000010d94b98d WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::MessageDecoder&) + 561 (HandleMessage.h:16)
26  com.apple.WebKit              	0x000000010d7251f1 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >) + 127 (memory:2636)
27  com.apple.WebKit              	0x000000010d727b4a IPC::Connection::dispatchOneMessage() + 126 (memory:2656)
28  com.apple.JavaScriptCore      	0x000000010eb69985 WTF::RunLoop::performWork() + 437 (functional:1742)
29  com.apple.JavaScriptCore      	0x000000010eb69d32 WTF::RunLoop::performWork(void*) + 34 (RunLoopCF.cpp:39)
30  com.apple.CoreFoundation      	0x00007fff949e2c01 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
31  com.apple.CoreFoundation      	0x00007fff949d4b1c __CFRunLoopDoSources0 + 556
32  com.apple.CoreFoundation      	0x00007fff949d403f __CFRunLoopRun + 927
33  com.apple.CoreFoundation      	0x00007fff949d3a38 CFRunLoopRunSpecific + 296
34  com.apple.HIToolbox           	0x00007fff88e673bd RunCurrentEventLoopInMode + 235
35  com.apple.HIToolbox           	0x00007fff88e67153 ReceiveNextEventCommon + 432
36  com.apple.HIToolbox           	0x00007fff88e66f93 _BlockUntilNextEventMatchingListInModeWithFilter + 71
37  com.apple.AppKit              	0x00007fff870b81e7 _DPSNextEvent + 1076
38  com.apple.AppKit              	0x00007fff8748490d -[NSApplication _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 454
39  com.apple.AppKit              	0x00007fff870ae0b8 -[NSApplication run] + 682
40  com.apple.AppKit              	0x00007fff87030396 NSApplicationMain + 1176
41  libxpc.dylib                  	0x00007fff8c70ff70 _xpc_objc_main + 793
42  libxpc.dylib                  	0x00007fff8c7116bf xpc_main + 494
43  com.apple.WebKit.WebContent.Development	0x00000001072d7424 main + 409 (XPCServiceMain.Development.mm:187)
44  libdyld.dylib                 	0x00007fff93aa15ad start + 1

[Radar WebKit Bug Importer]

<rdar://problem/22748858>

[Jiewen Tan]

Created attachment 262354 [details]
Patch

[Jiewen Tan]

Created attachment 262355 [details]
Patch

[Chris Dumez]

Comment on attachment 262355 [details]
Patch

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

r=me with changes.

> Source/WebCore/ChangeLog:3
> +        Check Frame pointers are not null before getting 

Should be on 1 line.

> Source/WebCore/ChangeLog:8
> +        This is a merge of Blink change 187441,

The description should be after the "Reviewed by" line and before the 'Test:' line.

> Source/WebCore/Modules/websockets/WebSocket.cpp:244
> +        shouldBypassMainWorldContentSecurityPolicy = document.frame() != nullptr && document.frame()->script().shouldBypassMainWorldContentSecurityPolicy();

document.frame() &&

> Source/WebCore/page/EventSource.cpp:88
>      bool shouldBypassMainWorldContentSecurityPolicy = false;

This code is duplicated. We may want to factor it in a function and call it in both WebSocket.cpp and here. e.g.
bool shouldBypassMainWorldContentSecurityPolicy(ScriptExecutionContext*);

We could have it in ContentSecurityPolicy.h like Blink.

> Source/WebCore/page/EventSource.cpp:91
> +        shouldBypassMainWorldContentSecurityPolicy = document.frame() != nullptr && document.frame()->script().shouldBypassMainWorldContentSecurityPolicy();

document.frame() &&

> LayoutTests/ChangeLog:3
> +        Check Frame pointers are not null before getting 

Should be on 1 line.

> LayoutTests/http/tests/websocket/construct-in-detached-frame.html:11
> +        testIframe.parentNode.removeChild(testIframe);

testIframe.remove() should suffice

> LayoutTests/http/tests/websocket/construct-in-detached-frame.html:14
> +function done()

Please indent with the rest.

> LayoutTests/http/tests/websocket/resources/construct-in-detached-frame.html:8
> +new webSocketClass('ws://127.0.0.1/');

Please indent.

> LayoutTests/http/tests/websocket/resources/construct-in-detached-frame.html:10
> +parentWindow.console.log(e.message);

Please indent.

[Jiewen Tan]

Created attachment 262444 [details]
Patch

[Chris Dumez]

Comment on attachment 262444 [details]
Patch

How about my earlier suggestion?

> Source/WebCore/page/EventSource.cpp:88
>      bool shouldBypassMainWorldContentSecurityPolicy = false;

This code is duplicated. We may want to factor it in a function and call it in both WebSocket.cpp and here. e.g.
bool shouldBypassMainWorldContentSecurityPolicy(ScriptExecutionContext*);

We could have it in ContentSecurityPolicy.h like Blink.

[Chris Dumez]

Comment on attachment 262444 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Check Frame pointers are not null before getting shouldBypassMainWorldContentSecurityPolicy via those pointers.

We usually use a shorter title (e.g. Fix null pointer dereference in WebSocket::connect()), then use the longer description in the Changelog description.

> Source/WebCore/ChangeLog:9
> +        This is a merge of Blink change 187441,

This is a merge of Blink r187441:

[Jiewen Tan]

Comment on attachment 262444 [details]
Patch

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

>> Source/WebCore/page/EventSource.cpp:88
>>      bool shouldBypassMainWorldContentSecurityPolicy = false;
> 
> This code is duplicated. We may want to factor it in a function and call it in both WebSocket.cpp and here. e.g.
> bool shouldBypassMainWorldContentSecurityPolicy(ScriptExecutionContext*);
> 
> We could have it in ContentSecurityPolicy.h like Blink.

Sure, where should I create this file?

[Jiewen Tan]

(In reply to comment #8)
> Comment on attachment 262444 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262444&action=review
> 
> >> Source/WebCore/page/EventSource.cpp:88
> >>      bool shouldBypassMainWorldContentSecurityPolicy = false;
> > 
> > This code is duplicated. We may want to factor it in a function and call it in both WebSocket.cpp and here. e.g.
> > bool shouldBypassMainWorldContentSecurityPolicy(ScriptExecutionContext*);
> > 
> > We could have it in ContentSecurityPolicy.h like Blink.
> 
> Sure, where should I create this file?

Forget this comment, please.

[Jiewen Tan]

Created attachment 262461 [details]
Patch

[Chris Dumez]

Comment on attachment 262461 [details]
Patch

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

> Source/WebCore/Modules/websockets/WebSocket.cpp:241
> +    bool shouldBypassMainWorldContentSecurityPolicy = scriptExecutionContext()->contentSecurityPolicy()->shouldBypassMainWorldContentSecurityPolicy(*scriptExecutionContext());

... = ContentSecurityPolicy::shouldBypassMainWorldContentSecurityPolicy(*scriptExecutionContext());

> Source/WebCore/page/ContentSecurityPolicy.cpp:1784
> +bool ContentSecurityPolicy::shouldBypassMainWorldContentSecurityPolicy(ScriptExecutionContext& context) const

It can be reused in other places as well. At least XMLHttpRequest::open().

> Source/WebCore/page/ContentSecurityPolicy.cpp:1787
> +        Document& document = downcast<Document>(context);

auto&

> Source/WebCore/page/ContentSecurityPolicy.h:133
> +    bool shouldBypassMainWorldContentSecurityPolicy(ScriptExecutionContext&) const;

Should be static.

> Source/WebCore/page/EventSource.cpp:88
> +    bool shouldBypassMainWorldContentSecurityPolicy = context.contentSecurityPolicy()->shouldBypassMainWorldContentSecurityPolicy(context);

... = ContentSecurityPolicy::shouldBypassMainWorldContentSecurityPolicy(context);

> LayoutTests/ChangeLog:7
> +

Extra line.

[Jiewen Tan]

Created attachment 262469 [details]
Patch

[Chris Dumez]

Comment on attachment 262469 [details]
Patch

Looks good.

[WebKit Commit Bot]

Comment on attachment 262469 [details]
Patch

Clearing flags on attachment: 262469

Committed r190588: <http://trac.webkit.org/changeset/190588>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

Hey Jiewen, thanks for fixing this.

Later down in WebSocket::connect() there is a call to document.frame()->loader().mixedContentChecker(). I don't see why that doesn't crash now, since it occurs if (is<Document>(*scriptExecutionContext()), the same condition as the null dereference of frame up above. That was "safe" when I added it because document.frame() was assumed to be nonnull up above, but clearly that was wrong and is now no longer the case.

I guess if it's not crashing (is it returning early on an error path?), then it might not be a problem, but it looks dangerous in light of this change... do we need to add a check to make sure frame is not null there? If so, we're going to have to rethink how to gain access to the mixed content checker, since we can't just skip that check.

[Jiewen Tan]

(In reply to comment #16)
> Hey Jiewen, thanks for fixing this.
> 
> Later down in WebSocket::connect() there is a call to
> document.frame()->loader().mixedContentChecker(). I don't see why that
> doesn't crash now, since it occurs if
> (is<Document>(*scriptExecutionContext()), the same condition as the null
> dereference of frame up above. That was "safe" when I added it because
> document.frame() was assumed to be nonnull up above, but clearly that was
> wrong and is now no longer the case.
> 
> I guess if it's not crashing (is it returning early on an error path?), then
> it might not be a problem, but it looks dangerous in light of this change...
> do we need to add a check to make sure frame is not null there? If so, we're
> going to have to rethink how to gain access to the mixed content checker,
> since we can't just skip that check.

You are right. That's something I am missing. Let me dig into it.

[Michael Catanzaro]

I filed bug #149864 so we don't forget.

[Jiewen Tan]

(In reply to comment #18)
> I filed bug #149864 so we don't forget.

Then, let us continue our discussion on the new thread. Should we consider this bug is closed?

[Michael Catanzaro]

Yup! I think it's easiest to move on to a new bug when we discover an issue after a patch has landed.