Mis-diagnosed in bug 245624 I somehow made a WPE build that triggers this crash, where the quirksUserAgentStyleSheet string is null... (gdb) bt #0 0x00007fa2a3141589 in WTF::StringView::StringView(WTF::StringImpl const*) (string=0x0, this=<optimized out>) at WTF/Headers/wtf/text/StringConcatenate.h:430 #1 WTF::StringTypeAdapter<WTF::StringImpl*, void>::writeTo<unsigned char>(unsigned char*) const (destination=0x7fa2940e84b0 "", this=<optimized out>) at WTF/Headers/wtf/text/StringConcatenate.h:188 #2 WTF::stringTypeAdapterAccumulator<unsigned char, WTF::StringTypeAdapter<WTF::String, void> >(unsigned char*, WTF::StringTypeAdapter<WTF::String, void>) (result=0x7fa2940e84b0 "", adapter=...) at WTF/Headers/wtf/text/StringConcatenate.h:423 #3 WTF::stringTypeAdapterAccumulator<unsigned char, WTF::StringTypeAdapter<WTF::String, void>, WTF::StringTypeAdapter<WTF::String, void> >(unsigned char*, WTF::StringTypeAdapter<WTF::String, void>, WTF::StringTypeAdapter<WTF::String, void>) (result=<optimized out>, adapter=..., adapters=...) at WTF/Headers/wtf/text/StringConcatenate.h:430 #4 WTF::tryMakeStringImplFromAdaptersInternal<WTF::StringTypeAdapter<WTF::String, void>, WTF::StringTypeAdapter<WTF::String, void> >(unsigned int, bool, WTF::StringTypeAdapter<WTF::String, void>, WTF::StringTypeAdapter<WTF::String, void>) (length=348, areAllAdapters8Bit=<optimized out>, adapter=..., adapters=...) at WTF/Headers/wtf/text/StringConcatenate.h:444 #5 0x00007fa2a33f90bd in WTF::tryMakeStringFromAdapters<WTF::StringTypeAdapter<WTF::String, void>, WTF::StringTypeAdapter<WTF::String, void> >(WTF::StringTypeAdapter<WTF::String, void>, WTF::StringTypeAdapter<WTF::String, void>) (adapter=..., adapters=...) at WTF/Headers/wtf/text/StringConcatenate.h:475 #6 WTF::tryMakeString<WTF::String, WTF::String>(WTF::String, WTF::String) (strings=..., strings=...) at WTF/Headers/wtf/text/StringConcatenate.h:481 #7 WTF::StringAppend<WTF::String, WTF::String>::operator WTF::String() const (this=0x7fff176fe608) at WTF/Headers/wtf/text/StringOperators.h:38 #8 0x00007fa2a5dc046c in WebCore::Style::UserAgentStyle::initDefaultStyleSheet() () at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebCore/style/UserAgentStyle.cpp:158 #9 0x00007fa2a5da6460 in WebCore::Style::Resolver::Resolver(WebCore::Document&) (this=0x7fa0aa000a80, document=...) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebCore/style/StyleResolver.cpp:147 #10 0x00007fa2a5daabfb in WebCore::Style::Resolver::create(WebCore::Document&) (document=...) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebCore/style/StyleResolver.cpp:139 #11 WebCore::Style::Scope::createDocumentResolver() (this=0x7fa2940c4400) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebCore/style/StyleScope.cpp:104 #12 0x00007fa2a5daa5be in WebCore::Style::Scope::resolver() (this=0x7fa2940c4400) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebCore/style/StyleScope.cpp:89 #13 0x00007fa2a5dbd310 in WebCore::Style::TreeResolver::resolve() (this=0x7fff176fe818) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebCore/style/StyleTreeResolver.cpp:909 #14 0x00007fa2a529a6ad in WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType) (this=0x7fa2420e0000, type=<optimized out>) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebCore/dom/Document.cpp:2091 #15 0x00007fa2a529bc61 in WebCore::Document::createRenderTree() (this=0x7fa2420e0000) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebCore/dom/Document.cpp:2501 #16 0x00007fa2a529bcfe in WebCore::Document::didBecomeCurrentDocumentInFrame() (this=0x7fa2420e0000) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebCore/dom/Document.cpp:2513 #17 0x00007fa2a58077c2 in WebCore::Frame::setDocument(WTF::RefPtr<WebCore::Document, WTF::RawPtrTraits<WebCore::Document>, WTF::DefaultRefDerefTraits<WebCore::Document> >&&) (this=0x7fa2940a0f00, newDocument=...) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebCore/page/Frame.cpp:313 #18 0x00007fa2a57084a1 in WebCore::DocumentWriter::begin(WTF::URL const&, bool, WebCore::Document*, WebCore::ProcessQualified<WTF::UUID>) (this=0x7fa24201d060, urlReference=..., dispatch=false, ownerDocument=0x0, documentIdentifier=...) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebCore/loader/DocumentWriter.cpp:180 #19 0x00007fa2a57030cc in WebCore::DocumentLoader::commitData(WebCore::SharedBuffer const&) (this=0x7fa24201d000, data=...) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebCore/loader/DocumentLoader.cpp:1235 #20 0x00007fa2a5702a5c in WebCore::DocumentLoader::finishedLoading() (this=0x7fa24201d000) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebCore/loader/DocumentLoader.cpp:493 #21 0x00007fa2a570ad07 in WebCore::DocumentLoader::maybeLoadEmpty() (this=0x7fa24201d000) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebCore/loader/DocumentLoader.cpp:2043 #22 0x00007fa2a570afe4 in WebCore::DocumentLoader::startLoadingMainResource() (this=0x7fa24201d000) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebCore/loader/DocumentLoader.cpp:2070 #23 0x00007fa2a5722e92 in WebCore::FrameLoader::init() (this=0x7fa294028340) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebCore/loader/FrameLoader.cpp:351 #24 0x00007fa2a33e6812 in WebKit::WebFrame::initWithCoreMainFrame(WebKit::WebPage&, WebCore::Frame&) (this=0x7fa294004480, page=<optimized out>, coreFrame=...) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebKit/WebProcess/WebPage/WebFrame.cpp:115 #25 0x00007fa2a33c27b7 in WebKit::WebPage::WebPage(WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters&&) (this=0x7fa2940e4100, pageID=..., parameters=...) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebKit/WebProcess/WebPage/WebPage.cpp:725 #26 0x00007fa2a33c12b3 in WebKit::WebPage::create(WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters&&) (pageID=..., parameters=...) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebKit/WebProcess/WebPage/WebPage.cpp:465 #27 0x00007fa2a32fffd5 in WebKit::WebProcess::createWebPage(WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters&&) (this=0x7fa294030210, pageID=..., parameters=...) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebKit/WebProcess/WebProcess.cpp:837 #28 0x00007fa2a2eed2d1 in IPC::callMemberFunctionImpl<WebKit::WebProcess, void (WebKit::WebProcess::*)(WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters&&), std::tuple<WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters>, 0ul, 1ul>(WebKit::WebProcess*, void (WebKit::WebProcess::*)(WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters&&), std::tuple<WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters>&&, std::integer_sequence<unsigned long, 0ul, 1ul>) (object=0x7fa294030210, function=<optimized out>, args=...) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebKit/Platform/IPC/HandleMessage.h:131 #29 IPC::callMemberFunction<WebKit::WebProcess, void (WebKit::WebProcess::*)(WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters&&), std::tuple<WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters>, std::integer_sequence<unsigned long, 0ul, 1ul> >(std::tuple<WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters>&&, WebKit::WebProcess*, void (WebKit::WebProcess::*)(WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters&&)) (args=..., object=0x7fa294030210, function=<optimized out>) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebKit/Platform/IPC/HandleMessage.h:137 #30 IPC::handleMessage<Messages::WebProcess::CreateWebPage, WebKit::WebProcess, void (WebKit::WebProcess::*)(WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters&&)>(IPC::Connection&, IPC::Decoder&, WebKit::WebProcess*, void (WebKit::WebProcess::*)(WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebKit::WebPageCreationParameters&&)) (connection=<optimized out>, decoder=..., object=0x7fa294030210, function=<optimized out>) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebKit/Platform/IPC/HandleMessage.h:259 #31 WebKit::WebProcess::didReceiveWebProcessMessage(IPC::Connection&, IPC::Decoder&) (this=0x7fa294030210, connection=<optimized out>, decoder=...) at DerivedSources/WebKit/WebProcessMessageReceiver.cpp:280 #32 0x00007fa2a30c462e in IPC::Connection::dispatchMessage(IPC::Decoder&) (this=0x7fa2940501a0, decoder=...) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebKit/Platform/IPC/Connection.cpp:1105 #33 0x00007fa2a30c4913 in IPC::Connection::dispatchMessage(std::unique_ptr<IPC::Decoder, std::default_delete<IPC::Decoder> >) (this=0x7fa2940501a0, message=std::unique_ptr<IPC::Decoder> = {...}) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebKit/Platform/IPC/Connection.cpp:1150 #34 0x00007fa2a30c4c3e in IPC::Connection::dispatchOneIncomingMessage() (this=0x7fa2940501a0) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebKit/Platform/IPC/Connection.cpp:1219 #35 0x00007fa2a4449ccd in WTF::Function<void ()>::operator()() const (this=<optimized out>) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WTF/wtf/Function.h:82 #36 WTF::RunLoop::performWork() (this=0x7fa2940100e0) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WTF/wtf/RunLoop.cpp:133 #37 0x00007fa2a44a7f8d in WTF::RunLoop::RunLoop()::$_1::operator()(void*) const (userData=0x7fa2940e8460, userData@entry=0x7fa2940100e0, this=<optimized out>) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WTF/wtf/glib/RunLoopGLib.cpp:80 #38 WTF::RunLoop::RunLoop()::$_1::__invoke(void*) (userData=0x7fa2940e8460) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WTF/wtf/glib/RunLoopGLib.cpp:79 #39 0x00007fa2a44a739e in WTF::RunLoop::$_0::operator()(_GSource*, int (*)(void*), void*) const (source=0x11df990, callback=0x7fa2a44a7f80 <WTF::RunLoop::RunLoop()::$_1::__invoke(void*)>, userData=0x7fa2940100e0, this=<optimized out>) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WTF/wtf/glib/RunLoopGLib.cpp:53 #40 WTF::RunLoop::$_0::__invoke(_GSource*, int (*)(void*), void*) (source=0x11df990, callback=0x7fa2a44a7f80 <WTF::RunLoop::RunLoop()::$_1::__invoke(void*)>, userData=0x7fa2940100e0) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WTF/wtf/glib/RunLoopGLib.cpp:45 #41 0x00007fa2a2088faf in g_main_context_dispatch () at /lib64/libglib-2.0.so.0 #42 0x00007fa2a20de2c8 in g_main_context_iterate.constprop () at /lib64/libglib-2.0.so.0 #43 0x00007fa2a20886cf in g_main_loop_run () at /lib64/libglib-2.0.so.0 #44 0x00007fa2a44a7983 in WTF::RunLoop::run() () at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WTF/wtf/glib/RunLoopGLib.cpp:108 #45 0x00007fa2a33ff1f1 in WebKit::AuxiliaryProcessMainBase<WebKit::WebProcess, true>::run(int, char**) (this=0x7fff176ffee0, argc=3, argv=<optimized out>) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebKit/Shared/AuxiliaryProcessMain.h:71 #46 WebKit::AuxiliaryProcessMain<WebKit::WebProcessMainWPE>(int, char**) (argc=3, argv=<optimized out>) at /usr/src/debug/wpewebkit-2.38.0-1.fc36.x86_64/Source/WebKit/Shared/AuxiliaryProcessMain.h:97 #47 0x00007fa2a24b0550 in __libc_start_call_main () at /lib64/libc.so.6 #48 0x00007fa2a24b0609 in __libc_start_main_impl () at /lib64/libc.so.6 #49 0x0000000000401075 in _start ()
Pull request: https://github.com/WebKit/WebKit/pull/4683
What line of code is crashing? I can’t find the code that is missing a null check. Null strings are supposed to be treated the same as empty strings by this code.
With these flags I can reproduce the crash, building with clang 14: CFLAGS='-O2 -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fstack-protector-strong -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection' export CFLAGS CXXFLAGS='-O2 -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fstack-protector-strong -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection' export CXXFLAGS I added log statements: diff --git a/Source/WTF/wtf/text/StringView.h b/Source/WTF/wtf/text/StringView.h index 5814931863e9..d1fd709cd910 100644 --- a/Source/WTF/wtf/text/StringView.h +++ b/Source/WTF/wtf/text/StringView.h @@ -393,9 +393,11 @@ inline StringView::StringView(const StringImpl& string) inline StringView::StringView(const StringImpl* string) { + WTFLogAlways("StringImpl 0 %p", string); if (!string) return; + WTFLogAlways("StringImpl 1 %p", string); setUnderlyingString(string); if (string->is8Bit()) initialize(string->characters8(), string->length()); Runtime: ... StringImpl 0 0x7f7772427000 StringImpl 1 0x7f7772427000 StringImpl 0 0x7f77c41098e0 StringImpl 1 0x7f77c41098e0 StringImpl 0 (nil) StringImpl 1 (nil) StringImpl 0 0x7fc9661086a0 StringImpl 1 0x7fc9661086a0 StringImpl 0 0x7fc966118120 StringImpl 1 0x7fc966118120 How are we reaching the "1" call with a nil string?! Could this be a compiler bug?
That's very confusing. I don't know how this could happen. Compilers do like to optimize away nullptr checks like these if they detect undefined behavior, but I'm not sure how that could be happening here. For example, checks for if (this != nullptr) will surely be optimized away. But that's a totally different case than what's happening here. Although it's *probably* unrelated, it reminds me a little of bug #245968, which I reported yesterday, where the point where it's crashing should be unreachable due to a similar check, in that case if (!cache_node). Odd. That one is built with GCC, though. > Could this be a compiler bug? It kinda looks like it. That said, in every similar situation where I previously suspected a GCC or Clang compiler bug, I was always wrong....
(In reply to Philippe Normand from comment #0) > #1 WTF::StringTypeAdapter<WTF::StringImpl*, void>::writeTo<unsigned char>(unsigned char*) const (destination=0x7fa2940e84b0 "", this=<optimized out>) at WTF/Headers/wtf/text/StringConcatenate.h:188 This should test for a non-null StringImpl pointer. Even if the test in the StringView constructor would do the early return, the later call to StringView::getCharactersWithUpconvert() on this object would crash cause the StringView wouldn't have any character data initialized, due to the StringImpl being null to begin with. Clang possibly understands this to the point of eliminating that null check, but it only brings the crash forward anyway.
(In reply to Zan Dobersek from comment #5) > (In reply to Philippe Normand from comment #0) > > #1 WTF::StringTypeAdapter<WTF::StringImpl*, void>::writeTo<unsigned char>(unsigned char*) const (destination=0x7fa2940e84b0 "", this=<optimized out>) at WTF/Headers/wtf/text/StringConcatenate.h:188 > > This should test for a non-null StringImpl pointer. Even if the test in the > StringView constructor would do the early return, the later call to > StringView::getCharactersWithUpconvert() on this object would crash cause > the StringView wouldn't have any character data initialized, due to the > StringImpl being null to begin with. Clang possibly understands this to the > point of eliminating that null check, but it only brings the crash forward > anyway. Ah! So the current version of my PR would be correct? I can expand the commit message as well, thanks for the explanation.
(In reply to Zan Dobersek from comment #5) > (In reply to Philippe Normand from comment #0) > > #1 WTF::StringTypeAdapter<WTF::StringImpl*, void>::writeTo<unsigned char>(unsigned char*) const (destination=0x7fa2940e84b0 "", this=<optimized out>) at WTF/Headers/wtf/text/StringConcatenate.h:188 > > This should test for a non-null StringImpl pointer. Even if the test in the > StringView constructor would do the early return, the later call to > StringView::getCharactersWithUpconvert() on this object would crash cause > the StringView wouldn't have any character data initialized, due to the > StringImpl being null to begin with. Clang possibly understands this to the > point of eliminating that null check, but it only brings the crash forward > anyway. With a null StringImpl, StringView::getCharactersWithUpconvert() doesn't crash, but it calls into StringImpl::copyCharacters() and causes undefined behavior when calling memcpy() which expects the source destination to be non-null. This memcpy() call still survives, so there's no crashes on other compilers. With Clang 14, the compiler takes memcpy()'s non-null requirements for granted and makes optimizations based on that, to the point where it assumes the StringImpl will never be null because data based on it will be passed to memcpy(), removing the early return in the StringView constructor. This is avoidable by doing an early return in StringImpl::copyCharacters() in case that the numCharacters value is zero (which happens when StringImpl is null and the passed-in length is also zero).
(In reply to Zan Dobersek from comment #5) > This should test for a non-null StringImpl pointer. Even if the test in the > StringView constructor would do the early return, the later call to > StringView::getCharactersWithUpconvert() on this object would crash cause > the StringView wouldn't have any character data initialized, due to the > StringImpl being null to begin with. Clang possibly understands this to the > point of eliminating that null check, but it only brings the crash forward > anyway. Why would StringView::getCharactersWithUpconvert crash? StringView supports calls to getCharactersWithUpconvert when the StringView points to null. This concept that "the StringView wouldn't have any character data initialized" doesn't make logical sense. This is a StringView with a character pointer of null and a length of 0. There is no character data to be initialized.
(In reply to Zan Dobersek from comment #7) > With a null StringImpl, StringView::getCharactersWithUpconvert() doesn't > crash, but it calls into StringImpl::copyCharacters() and causes undefined > behavior when calling memcpy() which expects the source destination to be > non-null. This memcpy() call still survives, so there's no crashes on other > compilers. Agreed. We should add the null check either in StringView::getCharactersWithUpconvert before calling StringImpl::copyCharacters or in StringImpl::copyCharacters.
The compiler is really smart and discovers code later that is using the pointer in way that is invalid if it’s null and then reasons it must not be null and so optimizes out an unnecessary null check. We need to decide that the contract of StringImpl::copyCharacters allows passing a null with a length of zero. If we want it to allow a null, then an if statement needs to go there before calling memcpy. If we want it to require a non-null pointer we should add an assertion and add checks at any calls sites that would otherwise pass null pointers.
(In reply to Zan Dobersek from comment #7) > This is avoidable by doing an early return in StringImpl::copyCharacters() > in case that the numCharacters value is zero (which happens when StringImpl > is null and the passed-in length is also zero). I do not think we should do this version. I’d rather use the null pointer check which can sometimes be optimized out by the compiler rather than the length check, which almost certainly can’t be optimized out.
One reason I like the idea of putting the null check in StringView::getCharactersWithUpconvert is that it’s just allocated memory so one more null check is *highly* unlikely to have performance effects. Not sure that’s true of all other call sites of StringImpl::copyCharacters. On the other hand, maybe this issue exists at some other call sites?
(In reply to Darin Adler from comment #12) > One reason I like the idea of putting the null check in > StringView::getCharactersWithUpconvert is that it’s just allocated memory so > one more null check is *highly* unlikely to have performance effects. > Alright, I'll update the PR then. > Not sure that’s true of all other call sites of StringImpl::copyCharacters. > > On the other hand, maybe this issue exists at some other call sites? I have no evidence of this yet. I wonder though, would it make sense to add an ASSERT(source) in that method, at least immediately before the memcpy() call?
(In reply to Philippe Normand from comment #13) > I wonder though, would it make sense to add an ASSERT(source) in that > method, at least immediately before the memcpy() call? Yes, we should add that.
Committed 255330@main (c736ca45a340): <https://commits.webkit.org/255330@main> Reviewed commits have been landed. Closing PR #4683 and removing active labels.
(In reply to Darin Adler from comment #14) > (In reply to Philippe Normand from comment #13) > > I wonder though, would it make sense to add an ASSERT(source) in that > > method, at least immediately before the memcpy() call? > > Yes, we should add that. https://bugs.webkit.org/show_bug.cgi?id=246267