Bug 220491 – [CoreIPC] null-ptr in WebCore::TextIndicatorWindow::setTextIndicator

• Component: WebKit2
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Using the new IPC testing code I added in https://trac.webkit.org/r268239,
we can reproduce the following crash in macOS ASAN builds:

=================================================================
ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000121587d58 bp 0x7ffee8e44c10 sp 0x7ffee8e442a0 T0)

    #0 0x121587d58 in -[WebTextIndicatorView initWithFrame:textIndicator:margin:offset:] TextIndicatorWindow.mm:162
    #1 0x12158e665 in WebCore::TextIndicatorWindow::setTextIndicator(WTF::Ref<WebCore::TextIndicator, WTF::RawPtrTraits<WebCore::TextIndicator> >, CGRect, WebCore::TextIndicatorWindowLifetime) TextIndicatorWindow.mm:469
    #2 0x113beed46 in WebKit::WebViewImpl::setTextIndicator(WebCore::TextIndicator&, WebCore::TextIndicatorWindowLifetime) WebViewImpl.mm:3493
    #3 0x114035d61 in WebKit::PageClientImpl::setTextIndicator(WTF::Ref<WebCore::TextIndicator, WTF::RawPtrTraits<WebCore::TextIndicator> >, WebCore::TextIndicatorWindowLifetime) PageClientImplMac.mm:548
    #4 0x113de016d in WebKit::WebPageProxy::setTextIndicator(WebCore::TextIndicatorData const&, unsigned long long) WebPageProxy.cpp:6398
    #5 0x114d03810 in void IPC::callMemberFunctionImpl<WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(WebCore::TextIndicatorData const&, unsigned long long), std::__1::tuple<WebCore::TextIndicatorData, unsigned long long>, 0ul, 1ul>(WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)(WebCore::TextIndicatorData const&, unsigned long long), std::__1::tuple<WebCore::TextIndicatorData, unsigned long long>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul>) HandleMessage.h:42
    #6 0x114d02c88 in void IPC::callMemberFunction<WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(WebCore::TextIndicatorData const&, unsigned long long), std::__1::tuple<WebCore::TextIndicatorData, unsigned long long>, std::__1::integer_sequence<unsigned long, 0ul, 1ul> >(std::__1::tuple<WebCore::TextIndicatorData, unsigned long long>&&, WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)(WebCore::TextIndicatorData const&, unsigned long long)) HandleMessage.h:48
    #7 0x114cb5455 in void IPC::handleMessage<Messages::WebPageProxy::SetTextIndicator, WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(WebCore::TextIndicatorData const&, unsigned long long)>(IPC::Decoder&, WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)(WebCore::TextIndicatorData const&, unsigned long long)) HandleMessage.h:120
    #8 0x114ca89ad in WebKit::WebPageProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&) WebPageProxyMessageReceiver.cpp:1412
    #9 0x112c709b5 in IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&) MessageReceiverMap.cpp:123
    #10 0x113b6e39c in WebKit::AuxiliaryProcessProxy::dispatchMessage(IPC::Connection&, IPC::Decoder&) AuxiliaryProcessProxy.cpp:216
    #11 0x113f4fca7 in WebKit::WebProcessProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&) WebProcessProxy.cpp:808
    #12 0x11279a103 in IPC::Connection::dispatchMessage(IPC::Decoder&) Connection.cpp:1038
    #13 0x11279ba77 in IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) Connection.cpp:1138
    #14 0x112798889 in IPC::Connection::dispatchIncomingMessages() Connection.cpp:1242
    #15 0x1127bacde in IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_8::operator()() Connection.cpp:999
    #16 0x1127bac4c in WTF::Detail::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_8, void>::call() Function.h:52
    #17 0x10a0629ae in WTF::Function<void ()>::operator()() const Function.h:83
    #18 0x10a0fbe68 in WTF::RunLoop::performWork() RunLoop.cpp:128
    #19 0x10a0ff185 in WTF::RunLoop::performWork(void*) RunLoopCF.cpp:46

<rdar://problem/71841069>

[Ryosuke Niwa]

Created attachment 417323 [details]
Test (requires WTR in macOS)

[Rob Buis]

Created attachment 417610 [details]
Patch

[Rob Buis]

This does not seem like a security problem to me. I do wonder where a test would go?

[Ryosuke Niwa]

(In reply to Rob Buis from comment #3)
> This does not seem like a security problem to me. I do wonder where a test
> would go?

LayoutTests/ipc probably.

[Rob Buis]

Created attachment 417685 [details]
Patch

[Rob Buis]

Created attachment 417700 [details]
Patch

[Rob Buis]

(In reply to Ryosuke Niwa from comment #4)
> (In reply to Rob Buis from comment #3)
> > This does not seem like a security problem to me. I do wonder where a test
> > would go?
> 
> LayoutTests/ipc probably.

It seems this will only work for ASAN/Debug builds. Instead I added a unit test.

[Alex Christensen]

I've had success making similar layout tests like this:

<!DOCTYPE html><!-- webkit-test-runner [ IPCTestingAPIEnabled=true ] -->
<html>
<body>
<script>
if (window.testRunner)
    testRunner.dumpAsText();
if (window.IPC)
    IPC.sendMessage(...);
</script>
This test passes if it does not crash.
</body>
</html>

It has the same output in debug and release.  It only did anything in the debug/asan builds, but it's better than nothing, and I think it's more sustainable than adding API tests for this.

[Rob Buis]

(In reply to Alex Christensen from comment #8)
> I've had success making similar layout tests like this:
> 
> <!DOCTYPE html><!-- webkit-test-runner [ IPCTestingAPIEnabled=true ] -->
> <html>
> <body>
> <script>
> if (window.testRunner)
>     testRunner.dumpAsText();
> if (window.IPC)
>     IPC.sendMessage(...);
> </script>
> This test passes if it does not crash.
> </body>
> </html>
> 
> It has the same output in debug and release.  It only did anything in the
> debug/asan builds, but it's better than nothing, and I think it's more
> sustainable than adding API tests for this.

Thanks, looks like I missed the if (window.IPC) part, will redo my patch.

[Rob Buis]

Created attachment 417733 [details]
Patch

[youenn fablet]

Comment on attachment 417733 [details]
Patch

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

> Source/WebCore/page/mac/TextIndicatorWindow.mm:164
> +    if (_textIndicator->contentImage()) {

if (auto* image = _textIndicator->contentImage()) {
   ...
}
Also, is there a possibility that we have contentImage() which is null, but either contentImageWithHighlight or contentImageWithoutSelection which are not null?

[Rob Buis]

Created attachment 417873 [details]
Patch

[Rob Buis]

Comment on attachment 417733 [details]
Patch

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

>> Source/WebCore/page/mac/TextIndicatorWindow.mm:164
>> +    if (_textIndicator->contentImage()) {
> 
> if (auto* image = _textIndicator->contentImage()) {
>    ...
> }
> Also, is there a possibility that we have contentImage() which is null, but either contentImageWithHighlight or contentImageWithoutSelection which are not null?

Done. The test IPC can be so formed AFAIK that all three can be null. So I null checked contentImageWithHighlight in this same method, but it looks like contentImageWithoutSelection is not exposed with a getter.

[youenn fablet]

Comment on attachment 417873 [details]
Patch

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

> LayoutTests/ChangeLog:8
> +        Add test for this.

Might not be needed.

> LayoutTests/ipc/set-text-indicator.html:8
> +    IPC.sendMessage('UI', IPC.webPageProxyID, IPC.messages.WebPageProxy_SetTextIndicator.name, [{type: 'float', value: 598}, {type: 'float', value: 352}, {type: 'float', value: 597}, {type: 'float', value: 947.9962715781635}, {type: 'float', value: 0}, {type: 'float', value: 942}, {type: 'float', value: 233.5502697674895}, {type: 'float', value: 3103.98319661806}, {type: 'Vector', value: [[{type: 'float', value: 262}, {type: 'float', value: 1691.8218636988486}, {type: 'float', value: 390}, {type: 'float', value: 3255.5734463359736}], [{type: 'float', value: 245}, {type: 'float', value: 2805.6525082537914}, {type: 'float', value: 1587.8585914997557}, {type: 'float', value: 205}], [{type: 'float', value: 668}, {type: 'float', value: 1930.6632966420898}, {type: 'float', value: 286.4821983543896}, {type: 'float', value: 107}], [{type: 'float', value: 210}, {type: 'float', value: 21.44449100848898}, {type: 'float', value: 252.77757277214582}, {type: 'float', value: 736.6757847636829}], [{type: 'float', value: 1380.3264570234599}, {type: 'float', value: 387.09616515322466}, {type: 'float', value: 316}, {type: 'float', value: 366}], [{type: 'float', value: 1321.8796914377176}, {type: 'float', value: 1464.9902412682602}, {type: 'float', value: 327}, {type: 'float', value: 569.3400524583093}], [{type: 'float', value: 644.0885995610852}, {type: 'float', value: 1953.1505093058456}, {type: 'float', value: 511.1745496780984}, {type: 'float', value: 42}], [{type: 'float', value: 168}, {type: 'float', value: 617}, {type: 'float', value: 964.3373436967936}, {type: 'float', value: 756}], [{type: 'float', value: 385.12263561140804}, {type: 'float', value: 167.97526671216778}, {type: 'float', value: 154}, {type: 'float', value: 533}], [{type: 'float', value: 198.85771539870234}, {type: 'float', value: 315.55225980593696}, {type: 'float', value: 864}, {type: 'float', value: 1164.3220920210106}], [{type: 'float', value: 1402.4797727647326}, {type: 'float', value: 491}, {type: 'float', value: 482}, {type: 'float', value: 224}], [{type: 'float', value: 269.17031456719684}, {type: 'float', value: 544}, {type: 'float', value: 602}, {type: 'float', value: 287.3884180705148}], [{type: 'float', value: 179}, {type: 'float', value: 251.6630350908998}, {type: 'float', value: 149}, {type: 'float', value: 489}]]}, {type: 'float', value: 927}, {type: 'float', value: 705}, {type: 'float', value: 498}, {type: 'float', value: 830}, {type: 'float', value: 58}, {type: 'bool', value: 1}, {type: 'float', value: 341}, {type: 'float', value: 1203.8009453924515}, {type: 'float', value: 1291.9587067321006}, {type: 'float', value: 1073.9720902903282}, {type: 'uint8_t', value: 2}, {type: 'uint8_t', value: 2}, {type: 'uint16_t', value: 12367}, {type: 'bool', value: 0}, {type: 'bool', value: 0}, {type: 'bool', value: 0}, {type: 'uint64_t', value: 641}]);

How does this work exactly? If we change TextIndicatorData, will the test start to fail. Or will it be simply rejected as IPC decoding will fail?
I am guessing there is no way to add an Internals API that would create a carefully crafted TextIndicatorData to repro this issue?
Can the test be reduced a bit?

[Rob Buis]

Created attachment 418041 [details]
Patch

[Rob Buis]

Comment on attachment 417873 [details]
Patch

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

>> LayoutTests/ipc/set-text-indicator.html:8
>> +    IPC.sendMessage('UI', IPC.webPageProxyID, IPC.messages.WebPageProxy_SetTextIndicator.name, [{type: 'float', value: 598}, {type: 'float', value: 352}, {type: 'float', value: 597}, {type: 'float', value: 947.9962715781635}, {type: 'float', value: 0}, {type: 'float', value: 942}, {type: 'float', value: 233.5502697674895}, {type: 'float', value: 3103.98319661806}, {type: 'Vector', value: [[{type: 'float', value: 262}, {type: 'float', value: 1691.8218636988486}, {type: 'float', value: 390}, {type: 'float', value: 3255.5734463359736}], [{type: 'float', value: 245}, {type: 'float', value: 2805.6525082537914}, {type: 'float', value: 1587.8585914997557}, {type: 'float', value: 205}], [{type: 'float', value: 668}, {type: 'float', value: 1930.6632966420898}, {type: 'float', value: 286.4821983543896}, {type: 'float', value: 107}], [{type: 'float', value: 210}, {type: 'float', value: 21.44449100848898}, {type: 'float', value: 252.77757277214582}, {type: 'float', value: 736.6757847636829}], [{type: 'float', value: 1380.3264570234599}, {type: 'float', value: 387.09616515322466}, {type: 'float', value: 316}, {type: 'float', value: 366}], [{type: 'float', value: 1321.8796914377176}, {type: 'float', value: 1464.9902412682602}, {type: 'float', value: 327}, {type: 'float', value: 569.3400524583093}], [{type: 'float', value: 644.0885995610852}, {type: 'float', value: 1953.1505093058456}, {type: 'float', value: 511.1745496780984}, {type: 'float', value: 42}], [{type: 'float', value: 168}, {type: 'float', value: 617}, {type: 'float', value: 964.3373436967936}, {type: 'float', value: 756}], [{type: 'float', value: 385.12263561140804}, {type: 'float', value: 167.97526671216778}, {type: 'float', value: 154}, {type: 'float', value: 533}], [{type: 'float', value: 198.85771539870234}, {type: 'float', value: 315.55225980593696}, {type: 'float', value: 864}, {type: 'float', value: 1164.3220920210106}], [{type: 'float', value: 1402.4797727647326}, {type: 'float', value: 491}, {type: 'float', value: 482}, {type: 'float', value: 224}], [{type: 'float', value: 269.17031456719684}, {type: 'float', value: 544}, {type: 'float', value: 602}, {type: 'float', value: 287.3884180705148}], [{type: 'float', value: 179}, {type: 'float', value: 251.6630350908998}, {type: 'float', value: 149}, {type: 'float', value: 489}]]}, {type: 'float', value: 927}, {type: 'float', value: 705}, {type: 'float', value: 498}, {type: 'float', value: 830}, {type: 'float', value: 58}, {type: 'bool', value: 1}, {type: 'float', value: 341}, {type: 'float', value: 1203.8009453924515}, {type: 'float', value: 1291.9587067321006}, {type: 'float', value: 1073.9720902903282}, {type: 'uint8_t', value: 2}, {type: 'uint8_t', value: 2}, {type: 'uint16_t', value: 12367}, {type: 'bool', value: 0}, {type: 'bool', value: 0}, {type: 'bool', value: 0}, {type: 'uint64_t', value: 641}]);
> 
> How does this work exactly? If we change TextIndicatorData, will the test start to fail. Or will it be simply rejected as IPC decoding will fail?
> I am guessing there is no way to add an Internals API that would create a carefully crafted TextIndicatorData to repro this issue?
> Can the test be reduced a bit?

Yes, the Vector is now reduced and less precision is needed, but I think the remaining fields are still mandatory.

[EWS]

Committed r271694: <https://trac.webkit.org/changeset/271694>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418041 [details].

[Tim Horton]

Hmm, I don't quite understand how you addressed Youenn's concern. The minute someone changes the layout of TextIndicatorData (which is totally not unusual) this test is going to become completely useless (won't crash, but because the decode fails).

[Ryosuke Niwa]

(In reply to Tim Horton from comment #18)
> Hmm, I don't quite understand how you addressed Youenn's concern. The minute
> someone changes the layout of TextIndicatorData (which is totally not
> unusual) this test is going to become completely useless (won't crash, but
> because the decode fails).

I don't think there is really a way around it.

[Tim Horton]

(In reply to Ryosuke Niwa from comment #19)
> (In reply to Tim Horton from comment #18)
> > Hmm, I don't quite understand how you addressed Youenn's concern. The minute
> > someone changes the layout of TextIndicatorData (which is totally not
> > unusual) this test is going to become completely useless (won't crash, but
> > because the decode fails).
> 
> I don't think there is really a way around it.

Not really sure what the point of the test is, then.

[Tim Horton]

(in that, it will promptly become useless dead weight)

[Ryosuke Niwa]

(In reply to Tim Horton from comment #21)
> (in that, it will promptly become useless dead weight)

True. Maybe we need to have a wider discussion about this. Maybe the right course of action is not to add a test in these cases.