Bug 220491 - [CoreIPC] null-ptr in WebCore::TextIndicatorWindow::setTextIndicator
Summary: [CoreIPC] null-ptr in WebCore::TextIndicatorWindow::setTextIndicator
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-01-08 18:01 PST by Ryosuke Niwa
Modified: 2021-01-26 19:12 PST (History)
14 users (show)

See Also:


Attachments
Test (requires WTR in macOS) (2.74 KB, text/html)
2021-01-08 18:01 PST, Ryosuke Niwa
no flags Details
Patch (2.97 KB, patch)
2021-01-14 04:05 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (7.14 KB, patch)
2021-01-15 01:34 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (7.02 KB, patch)
2021-01-15 08:01 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (7.25 KB, patch)
2021-01-15 14:03 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (7.22 KB, patch)
2021-01-19 06:46 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (5.35 KB, patch)
2021-01-21 07:37 PST, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2021-01-08 18:01:11 PST
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>
Comment 1 Ryosuke Niwa 2021-01-08 18:01:31 PST
Created attachment 417323 [details]
Test (requires WTR in macOS)
Comment 2 Rob Buis 2021-01-14 04:05:34 PST
Created attachment 417610 [details]
Patch
Comment 3 Rob Buis 2021-01-14 04:07:15 PST
This does not seem like a security problem to me. I do wonder where a test would go?
Comment 4 Ryosuke Niwa 2021-01-14 17:35:29 PST
(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.
Comment 5 Rob Buis 2021-01-15 01:34:58 PST
Created attachment 417685 [details]
Patch
Comment 6 Rob Buis 2021-01-15 08:01:11 PST
Created attachment 417700 [details]
Patch
Comment 7 Rob Buis 2021-01-15 09:28:07 PST
(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.
Comment 8 Alex Christensen 2021-01-15 13:50:48 PST
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.
Comment 9 Rob Buis 2021-01-15 13:59:33 PST
(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.
Comment 10 Rob Buis 2021-01-15 14:03:33 PST
Created attachment 417733 [details]
Patch
Comment 11 youenn fablet 2021-01-19 05:48:25 PST
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?
Comment 12 Rob Buis 2021-01-19 06:46:40 PST
Created attachment 417873 [details]
Patch
Comment 13 Rob Buis 2021-01-19 07:08:33 PST
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.
Comment 14 youenn fablet 2021-01-21 04:55:40 PST
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?
Comment 15 Rob Buis 2021-01-21 07:37:35 PST
Created attachment 418041 [details]
Patch
Comment 16 Rob Buis 2021-01-21 08:26:59 PST
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.
Comment 17 EWS 2021-01-21 09:17:47 PST
Committed r271694: <https://trac.webkit.org/changeset/271694>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418041 [details].
Comment 18 Tim Horton 2021-01-21 12:26:11 PST
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).
Comment 19 Ryosuke Niwa 2021-01-21 16:19:34 PST
(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.
Comment 20 Tim Horton 2021-01-21 16:23:34 PST
(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.
Comment 21 Tim Horton 2021-01-21 16:23:58 PST
(in that, it will promptly become useless dead weight)
Comment 22 Ryosuke Niwa 2021-01-21 16:26:38 PST
(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.