Crash in WebCore::ParsedContentType::parseContentType when parsing invalid MIME type. This will trigger the crash: EXPECT_FALSE(isValidContentType("text/plain;text=value;=", Mode::MimeSniff)); When added to TestWebKitAPI.ParsedContentType.MimeSniff: $ ./Tools/Scripts/run-api-tests --debug --no-build --no-timeout --verbose TestWebKitAPI.ParsedContentType.MimeSniff The crash looks like this: ASSERTION FAILED: initialized() /var/build/Debug/usr/local/include/wtf/Optional.h(519) : T *WTF::Optional<WTF::StringView>::operator->() [T = WTF::StringView] 1 0x10d790f59 WTFCrash 2 0x12415d949 WTF::Optional<WTF::StringView>::operator->() 3 0x12415d071 WebCore::ParsedContentType::parseContentType(WebCore::Mode) 4 0x12415e055 WebCore::ParsedContentType::create(WTF::String const&, WebCore::Mode) 5 0x12415e17f WebCore::isValidContentType(WTF::String const&, WebCore::Mode) 6 0x10b950f60 TestWebKitAPI::ParsedContentType_MimeSniff_Test::TestBody() 7 0x10bd669ce void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) 8 0x10bd363ab void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) 9 0x10bd362d6 testing::Test::Run() 10 0x10bd374e5 testing::TestInfo::Run() 11 0x10bd383cf testing::TestCase::Run() 12 0x10bd44004 testing::internal::UnitTestImpl::RunAllTests() 13 0x10bd6ab4e bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) 14 0x10bd43adb bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) 15 0x10bd439b0 testing::UnitTest::Run() 16 0x10bb94781 RUN_ALL_TESTS() 17 0x10bb94711 TestWebKitAPI::TestsController::run(int, char**) 18 0x10bd02e3e main 19 0x7fff6b5803d5 start 20 0x2 We've also seen this crash in the field: Exception Type: EXC_BREAKPOINT (SIGTRAP) Exception Codes: 0x0000000000000001, 0x00000001c5eda1c4 Termination Signal: Trace/BPT trap: 5 Termination Reason: Namespace SIGNAL, Code 0x5 Terminating Process: exc handler [1056] Triggered by Thread: 0 Thread 0 name: Dispatch queue: com.apple.main-thread Thread 0 Crashed: 0 WebCore 0x00000001c5eda1c4 WebCore::ParsedContentType::parseContentType(WebCore::Mode) + 4224 (ParsedContentType.cpp:0) 1 WebCore 0x00000001c5ed95bc WebCore::ParsedContentType::parseContentType(WebCore::Mode) + 1144 (StringImpl.h:1101) 2 WebCore 0x00000001c5ed5134 WebCore::ParsedContentType::create(WTF::String const&, WebCore::Mode) + 160 (ParsedContentType.cpp:329) 3 WebCore 0x00000001c6408cc8 WebCore::XMLHttpRequest::responseMIMEType() const + 272 (XMLHttpRequest.cpp:873) 4 WebCore 0x00000001c640d1ac WebCore::XMLHttpRequest::createDecoder() const + 168 (XMLHttpRequest.cpp:882) 5 WebCore 0x00000001c640d5ec WebCore::XMLHttpRequest::didReceiveData(char const*, int) + 312 (XMLHttpRequest.cpp:1059) 6 WebCore 0x00000001c5c21f98 WebCore::CachedRawResource::notifyClientsDataWasReceived(char const*, unsigned int) + 364 (CachedRawResource.cpp:136) 7 WebCore 0x00000001c5c21ca0 WebCore::CachedRawResource::updateBuffer(WebCore::SharedBuffer&) + 456 (CachedRawResource.cpp:73) 8 WebCore 0x00000001c5bf69b8 WebCore::SubresourceLoader::didReceiveDataOrBuffer(char const*, int, WTF::RefPtr<WebCore::SharedBuffer, WTF::DumbPtrTraits<WebCore::SharedBuffer> >&&, long long, WebCore::DataPayloadType) + 436 (SubresourceLoader.cpp:519) 9 WebCore 0x00000001c5bf67ec WebCore::SubresourceLoader::didReceiveData(char const*, unsigned int, long long, WebCore::DataPayloadType) + 104 (SubresourceLoader.cpp:487) 10 WebKit 0x00000001c45d51c0 WebKit::WebResourceLoader::didReceiveData(IPC::DataReference const&, long long) + 280 (WebResourceLoader.cpp:212) 11 WebKit 0x00000001c472c28c void IPC::handleMessage<Messages::WebResourceLoader::DidReceiveData, WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(IPC::DataReference const&, long long)>(IPC::Decoder&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(IPC::DataReference const&, long long)) + 116 (HandleMessage.h:41) 12 WebKit 0x00000001c472be44 WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::Decoder&) + 268 (WebResourceLoaderMessageReceiver.cpp:62) 13 WebKit 0x00000001c45ceaec WebKit::NetworkProcessConnection::didReceiveMessage(IPC::Connection&, IPC::Decoder&) + 624 (NetworkProcessConnection.cpp:86) 14 WebKit 0x00000001c41c10b8 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 208 (Connection.cpp:1070) 15 WebKit 0x00000001c41c1388 IPC::Connection::dispatchOneIncomingMessage() + 196 (Connection.cpp:1139) 16 JavaScriptCore 0x00000001cc3fbb28 WTF::RunLoop::performWork() + 580 (Function.h:84) 17 JavaScriptCore 0x00000001cc3fbcd0 WTF::RunLoop::performWork(void*) + 40 (RunLoopCF.cpp:38) 18 CoreFoundation 0x00000001bca1eca0 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 28 19 CoreFoundation 0x00000001bca1ebf4 __CFRunLoopDoSource0 + 84 20 CoreFoundation 0x00000001bca1e344 __CFRunLoopDoSources0 + 196 21 CoreFoundation 0x00000001bca190e4 __CFRunLoopRun + 796 22 CoreFoundation 0x00000001bca18aa0 CFRunLoopRunSpecific + 480 23 Foundation 0x00000001bcd5eb58 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 232 24 Foundation 0x00000001bcd99a1c -[NSRunLoop(NSRunLoop) run] + 92 25 libxpc.dylib 0x00000001bc6a2700 _xpc_objc_main + 308 26 libxpc.dylib 0x00000001bc6a51a4 xpc_main + 152 27 WebKit 0x00000001c4328b3c WebKit::XPCServiceMain(int, char const**) + 384 (XPCServiceMain.mm:160) 28 libdyld.dylib 0x00000001bc8941ec start + 4
<rdar://problem/59250384>
Created attachment 390160 [details] Patch v1 Initial patch fixes the crash. Posted for feedback. I saw Darin's comments in Bug 180526 Comment #102 about Optional<StringView> possibly being redundant, but I didn't make any changes yet.
Comment on attachment 390160 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=390160&action=review I think https://mimesniff.spec.whatwg.org/#parsing-a-mime-type specifies different behavior. Also I think implementing Darin's suggestion in this bug/patch is fine. > Source/WebCore/platform/network/ParsedContentType.cpp:283 > if (m_contentType[index++] == ';') We probably want to implement https://mimesniff.spec.whatwg.org/#parsing-a-mime-type step 11.6 at this point. So at this point it should be safe to break in case of index >= contentTypeLength. > Source/WebCore/platform/network/ParsedContentType.cpp:288 > + return false; See above, this is not what MIMESniff spec wants. > Source/WebCore/platform/network/ParsedContentType.cpp:290 > String parameterName = keyRange->toString(); This should take into account that keyRange can be null (for MIMESniff mode), since parameterName being empty is fine for that spec. I.e. I expect this to be valid text/plain;=wrong;text=value
Okay, I was a bit over-zealous in adding test cases. I'm going to use this bug to ONLY fix the known crash and to add test cases only related to this crash. I will file a new bug with the additional test cases, although time constraints mean that I don't have time to address Darin's comments or likely fix the new test cases in the near term.
(In reply to David Kilzer (:ddkilzer) from comment #4) > Okay, I was a bit over-zealous in adding test cases. > > I'm going to use this bug to ONLY fix the known crash and to add test cases > only related to this crash. > > I will file a new bug with the additional test cases, although time > constraints mean that I don't have time to address Darin's comments or > likely fix the new test cases in the near term. Thanks, also quite busy but I can have a look at Darin's comments and fix the new test cases soonish.
(In reply to Rob Buis from comment #5) > (In reply to David Kilzer (:ddkilzer) from comment #4) > > Okay, I was a bit over-zealous in adding test cases. > > > > I'm going to use this bug to ONLY fix the known crash and to add test cases > > only related to this crash. > > > > I will file a new bug with the additional test cases, although time > > constraints mean that I don't have time to address Darin's comments or > > likely fix the new test cases in the near term. > > Thanks, also quite busy but I can have a look at Darin's comments and fix > the new test cases soonish. Actually, let me look at this again. I won't make the Optional<StringView> case, but I can probably fix the spec issue.
Comment on attachment 390160 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=390160&action=review >> Source/WebCore/platform/network/ParsedContentType.cpp:283 >> if (m_contentType[index++] == ';') > > We probably want to implement https://mimesniff.spec.whatwg.org/#parsing-a-mime-type step 11.6 at this point. So at this point it should be safe to break in case of index >= contentTypeLength. If you look above this check, the (index >= contentTypeLength) check already exists above: } else { if (index >= contentTypeLength) break; if (m_contentType[index] != '=' && m_contentType[index] != ';') { LOG_ERROR("Invalid Content-Type malformed parameter."); return false; } if (m_contentType[index++] == ';') continue; } I think the new test for "text/plain;text=value;=" will hit the (index >= contentTypeLength) check anyway on the next while() loop iteration in MIME sniffing mode. >> Source/WebCore/platform/network/ParsedContentType.cpp:288 >> + return false; > > See above, this is not what MIMESniff spec wants. I think I know how to fix this. Patch forthcoming. >> Source/WebCore/platform/network/ParsedContentType.cpp:290 >> String parameterName = keyRange->toString(); > > This should take into account that keyRange can be null (for MIMESniff mode), since parameterName being empty is fine for that spec. I.e. I expect this to be valid text/plain;=wrong;text=value It's valid in MIMESniff mode, and the MIME type would be "text/plain", correct?
Created attachment 390345 [details] Patch v2
Comment on attachment 390160 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=390160&action=review >>> Source/WebCore/platform/network/ParsedContentType.cpp:283 >>> if (m_contentType[index++] == ';') >> >> We probably want to implement https://mimesniff.spec.whatwg.org/#parsing-a-mime-type step 11.6 at this point. So at this point it should be safe to break in case of index >= contentTypeLength. > > If you look above this check, the (index >= contentTypeLength) check already exists above: > > } else { > if (index >= contentTypeLength) > break; > if (m_contentType[index] != '=' && m_contentType[index] != ';') { > LOG_ERROR("Invalid Content-Type malformed parameter."); > return false; > } > if (m_contentType[index++] == ';') > continue; > } > > I think the new test for "text/plain;text=value;=" will hit the (index >= contentTypeLength) check anyway on the next while() loop iteration in MIME sniffing mode. But that would prevent a crash, right? Anyway you have proven there can be a crash with certain input, so we do need a patch. >>> Source/WebCore/platform/network/ParsedContentType.cpp:288 >>> + return false; >> >> See above, this is not what MIMESniff spec wants. > > I think I know how to fix this. Patch forthcoming. Great. >>> Source/WebCore/platform/network/ParsedContentType.cpp:290 >>> String parameterName = keyRange->toString(); >> >> This should take into account that keyRange can be null (for MIMESniff mode), since parameterName being empty is fine for that spec. I.e. I expect this to be valid text/plain;=wrong;text=value > > It's valid in MIMESniff mode, and the MIME type would be "text/plain", correct? It would be valid in MIMESniff mode and the MIME type would be "text/plain;text=value". The algorithm is very forgiving, you can check the steps here: https://mimesniff.spec.whatwg.org/#parsing-a-mime-type
Comment on attachment 390345 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=390345&action=review Mostly looks finr but I think one check is not needed. > Source/WebCore/platform/network/ParsedContentType.cpp:289 > + } This block should not be needed. it should be covered by: if (mode == Mode::Rfc2045 && (!keyRange || index >= contentTypeLength)) { Can you check?
(In reply to David Kilzer (:ddkilzer) from comment #6) > (In reply to Rob Buis from comment #5) > > (In reply to David Kilzer (:ddkilzer) from comment #4) > > > Okay, I was a bit over-zealous in adding test cases. > > > > > > I'm going to use this bug to ONLY fix the known crash and to add test cases > > > only related to this crash. > > > > > > I will file a new bug with the additional test cases, although time > > > constraints mean that I don't have time to address Darin's comments or > > > likely fix the new test cases in the near term. > > > > Thanks, also quite busy but I can have a look at Darin's comments and fix > > the new test cases soonish. > > Actually, let me look at this again. I won't make the Optional<StringView> > case, but I can probably fix the spec issue. It is possible removing Optional<StringView> fixes the crash case, see https://bugs.webkit.org/show_bug.cgi?id=180526. If that patch is acceptable we maybe could keep only the unit tests in this patch. Or this could go first and then I add https://bugs.webkit.org/show_bug.cgi?id=180526. I don't mind either way, let me know.
Created attachment 390400 [details] Patch v3
(In reply to Rob Buis from comment #11) > (In reply to David Kilzer (:ddkilzer) from comment #6) > > (In reply to Rob Buis from comment #5) > > > (In reply to David Kilzer (:ddkilzer) from comment #4) > > > > Okay, I was a bit over-zealous in adding test cases. > > > > > > > > I'm going to use this bug to ONLY fix the known crash and to add test cases > > > > only related to this crash. > > > > > > > > I will file a new bug with the additional test cases, although time > > > > constraints mean that I don't have time to address Darin's comments or > > > > likely fix the new test cases in the near term. > > > > > > Thanks, also quite busy but I can have a look at Darin's comments and fix > > > the new test cases soonish. > > > > Actually, let me look at this again. I won't make the Optional<StringView> > > case, but I can probably fix the spec issue. > > It is possible removing Optional<StringView> fixes the crash case, see > https://bugs.webkit.org/show_bug.cgi?id=180526. If that patch is acceptable > we maybe could keep only the unit tests in this patch. Or this could go > first and then I add https://bugs.webkit.org/show_bug.cgi?id=180526. I don't > mind either way, let me know. As I noted in Bug 180526 Comment #112, let's use your StringView changes to fix the crash, plus the parameterName.isNull() check I added, plus my tests. I posted that as: <https://bugs.webkit.org/attachment.cgi?id=390400&action=review> Just look over the tests to make sure the results are what you expect. r=me on the StringView changes.
Comment on attachment 390400 [details] Patch v3 LGTM. Possibly Darin wants to have a look as well.
Comment on attachment 390400 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=390400&action=review Patch seems fine to me. But I am not sure switching from Optional to StringView is necessary to fix the bug. I like switching (I suggested it), but the actual fix doesn’t require it and could be much smaller. > Source/WebCore/platform/network/ParsedContentType.cpp:286 > - String parameterName = keyRange->toString(); > + String parameterName = keyRange.toString(); This seems to be where the bug lies. We are using keyRange without checking it for null. We could have written this if we were sticking with Optional. String parameterName = keyRange ? keyRange->toString() : String(); > Source/WebCore/platform/network/ParsedContentType.cpp:317 > - setContentTypeParameter(parameterName, parameterValue, mode); > + if (!parameterName.isNull()) > + setContentTypeParameter(parameterName, parameterValue, mode); I suppose this is the other half of the fix. The rest of the patch doesn’t seem necessary to me. If we *are* changing to StringView, I think we could consider getting rid of the parameterName local entirely. Just writing keyRange.toString() here seems better. No need to convert from StringView to String earlier. We need to do that for the value because the value isn’t always a substring, and so needs to be in a String not a StringView.
To be clear, I’m OK with this landing as is despite my comments.
Comment on attachment 390400 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=390400&action=review >> Source/WebCore/platform/network/ParsedContentType.cpp:286 >> + String parameterName = keyRange.toString(); > > This seems to be where the bug lies. We are using keyRange without checking it for null. We could have written this if we were sticking with Optional. > > String parameterName = keyRange ? keyRange->toString() : String(); This is exactly what I did in the previous patches I posted here. It seemed like a hack compared to replacing Optional<StringView> with StringView, which is why I eschewed it. >> Source/WebCore/platform/network/ParsedContentType.cpp:317 >> + setContentTypeParameter(parameterName, parameterValue, mode); > > I suppose this is the other half of the fix. The rest of the patch doesn’t seem necessary to me. > > If we *are* changing to StringView, I think we could consider getting rid of the parameterName local entirely. Just writing keyRange.toString() here seems better. No need to convert from StringView to String earlier. We need to do that for the value because the value isn’t always a substring, and so needs to be in a String not a StringView. Will fix before landing.
Created attachment 390416 [details] Patch for landing
Comment on attachment 390416 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=390416&action=review > Source/WebCore/ChangeLog:7 > + Reviewed by NOBODY (OOPS!). Isn't "webkit-patch land-safely" supposed to replace this with the reviewer?
Created attachment 390417 [details] Patch for landing v2
Comment on attachment 390417 [details] Patch for landing v2 Rejecting attachment 390417 [details] from commit-queue. New failing tests: editing/spelling/spellcheck-async-remove-frame.html Full output: https://webkit-queues.webkit.org/results/13320925
Created attachment 390445 [details] Archive of layout-test-results from webkit-cq-03 for mac-mojave The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: webkit-cq-03 Port: mac-mojave Platform: Mac OS X 10.14.6
Committed: <https://trac.webkit.org/changeset/256395/webkit>
Thanks for the help on this one, Rob, and for writing most of the patch!