Bug 207424 – Crash in WebCore::ParsedContentType::parseContentType when parsing invalid MIME type

• Component: Page Loading
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

<rdar://problem/59250384>

[David Kilzer (:ddkilzer)]

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.

[Rob Buis]

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

[David Kilzer (:ddkilzer)]

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.

[Rob Buis]

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

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

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?

[David Kilzer (:ddkilzer)]

Created attachment 390345 [details]
Patch v2

[Rob Buis]

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

[Rob Buis]

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?

[Rob Buis]

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

[David Kilzer (:ddkilzer)]

Created attachment 390400 [details]
Patch v3

[David Kilzer (:ddkilzer)]

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

[Rob Buis]

Comment on attachment 390400 [details]
Patch v3

LGTM. Possibly Darin wants to have a look as well.

[Darin Adler]

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.

[Darin Adler]

To be clear, I’m OK with this landing as is despite my comments.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 390416 [details]
Patch for landing

[David Kilzer (:ddkilzer)]

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?

[David Kilzer (:ddkilzer)]

Created attachment 390417 [details]
Patch for landing v2

[WebKit Commit Bot]

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

[WebKit Commit Bot]

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

[David Kilzer (:ddkilzer)]

Committed:  <https://trac.webkit.org/changeset/256395/webkit>

[David Kilzer (:ddkilzer)]

Thanks for the help on this one, Rob, and for writing most of the patch!