Bug 221441 - [CoreIPC] Nullptr crash in IPC::Decoder::decode<WebCore::ResourceError>
Summary: [CoreIPC] Nullptr crash in IPC::Decoder::decode<WebCore::ResourceError>
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
Keywords: InRadar
Depends on:
Reported: 2021-02-04 18:07 PST by Ryosuke Niwa
Modified: 2021-02-05 17:12 PST (History)
15 users (show)

See Also:

Test (6.24 KB, text/html)
2021-02-04 18:07 PST, Ryosuke Niwa
no flags Details
Patch (6.63 KB, patch)
2021-02-05 14:03 PST, Darin Adler
andersca: review+
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-02-04 18:07:43 PST
Created attachment 419350 [details]

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

==53800==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fff2040301d bp 0x7ffeed1a8070 sp 0x7ffeed1a7e98 T0)

    #0 0x7fff2040301d in CFDictionaryGetCount+0x6 (CoreFoundation:x86_64h+0x2201d)
    #1 0x114ea3fcc in IPC::ArgumentCoder<WebCore::ResourceError, void>::decodePlatformData(IPC::Decoder&, WebCore::ResourceError&) WebCoreArgumentCodersMac.mm:435
    #2 0x1161ecbb7 in IPC::ArgumentCoder<WebCore::ResourceError, void>::decode(IPC::Decoder&, WebCore::ResourceError&) WebCoreArgumentCoders.cpp:1297
    #3 0x1140151a8 in bool IPC::Decoder::decode<WebCore::ResourceError>(WebCore::ResourceError&) Decoder.h:91
    #4 0x1151050c5 in API::Error::decode(IPC::Decoder&, WTF::RefPtr<API::Object, WTF::RawPtrTraits<API::Object>, WTF::DefaultRefDerefTraits<API::Object> >&) APIError.cpp:95
    #5 0x114ffd6dc in WebKit::UserData::decode(IPC::Decoder&, WTF::RefPtr<API::Object, WTF::RawPtrTraits<API::Object>, WTF::DefaultRefDerefTraits<API::Object> >&) UserData.cpp:385
    #6 0x114ffde1a in WebKit::UserData::decode(IPC::Decoder&, WTF::RefPtr<API::Object, WTF::RawPtrTraits<API::Object>, WTF::DefaultRefDerefTraits<API::Object> >&) UserData.cpp:368
    #7 0x114ffd348 in WebKit::UserData::decode(IPC::Decoder&, WebKit::UserData&) UserData.cpp:151
    #8 0x1140621fc in IPC::ArgumentCoder<WebKit::UserData, void>::decode(IPC::Decoder&) ArgumentCoder.h:56
    #9 0x114061fc3 in IPC::Decoder& IPC::Decoder::operator>><WebKit::UserData>(WTF::Optional<WebKit::UserData>&) Decoder.h:107
    #10 0x114680201 in IPC::TupleDecoderImpl<WebKit::UserData>::decode(IPC::Decoder&) ArgumentCoders.h:317
    #11 0x1161d042e in IPC::TupleDecoderImpl<WTF::String, WebKit::UserData>::decode(IPC::Decoder&) ArgumentCoders.h:304
    #12 0x1161d032d in IPC::TupleDecoder<2ul, WTF::String, WebKit::UserData>::decode(IPC::Decoder&) ArgumentCoders.h:328
    #13 0x1161d024d in IPC::ArgumentCoder<std::__1::tuple<WTF::String, WebKit::UserData>, void>::decode(IPC::Decoder&) ArgumentCoders.h:348
    #14 0x1161d0087 in IPC::Decoder& IPC::Decoder::operator>><std::__1::tuple<WTF::String, WebKit::UserData> >(WTF::Optional<std::__1::tuple<WTF::String, WebKit::UserData> >&) Decoder.h:107
    #15 0x1161cfe25 in void IPC::handleMessage<Messages::WebConnection::HandleMessage, WebKit::WebConnection, void (WebKit::WebConnection::*)(WTF::String const&, WebKit::UserData const&)>(IPC::Decoder&, WebKit::WebConnection*, void (WebKit::WebConnection::*)(WTF::String const&, WebKit::UserData const&)) HandleMessage.h:114
    #16 0x1161cfcbf in WebKit::WebConnection::didReceiveMessage(IPC::Connection&, IPC::Decoder&) WebConnectionMessageReceiver.cpp:41
    #17 0x114311bbf in IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&) MessageReceiverMap.cpp:118
    #18 0x1152518fc in WebKit::AuxiliaryProcessProxy::dispatchMessage(IPC::Connection&, IPC::Decoder&) AuxiliaryProcessProxy.cpp:216
    #19 0x11563ee47 in WebKit::WebProcessProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&) WebProcessProxy.cpp:808
    #20 0x113dfe8a3 in IPC::Connection::dispatchMessage(IPC::Decoder&) Connection.cpp:1038
    #21 0x113e00217 in IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) Connection.cpp:1138
    #22 0x113dfd029 in IPC::Connection::dispatchIncomingMessages() Connection.cpp:1242
    #23 0x113e1f32e in IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_8::operator()() Connection.cpp:999
    #24 0x113e1f29c 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
    #25 0x10b67674e in WTF::Function<void ()>::operator()() const Function.h:83
    #26 0x10b711818 in WTF::RunLoop::performWork() RunLoop.cpp:128
    #27 0x10b714b35 in WTF::RunLoop::performWork(void*) RunLoopCF.cpp:46

Comment 1 Darin Adler 2021-02-05 10:41:30 PST
The bug here is in decodeNSError. The problem here is that the hasUnderlyingError support does not work properly when the userInfo dictionary is nullptr. Trivial to fix. I’d be happy to fix this but I don’t know how to make regression tests for an issue like this.
Comment 2 Darin Adler 2021-02-05 10:42:52 PST
I’m going to grab this and fix it, but I don’t plan to add a regression test.

Ryosuke, let me know if that’s not right.
Comment 3 Darin Adler 2021-02-05 10:57:39 PST
I read the encode function more carefully and it’s clear it won’t ever encode a null, so the fix here is just to add a null check. But I also did a tiny refactor to get rid of the need to use WARN_UNUSED_RETURN.
Comment 4 Darin Adler 2021-02-05 10:59:54 PST
Since this is just a null pointer dereference, I am thinking that maybe it’s not security sensitive?
Comment 5 Darin Adler 2021-02-05 14:03:12 PST
Created attachment 419459 [details]
Comment 6 Darin Adler 2021-02-05 16:16:27 PST
Committed r272450: <https://trac.webkit.org/changeset/272450>
Comment 7 Ryosuke Niwa 2021-02-05 17:07:46 PST
(In reply to Darin Adler from comment #4)
> Since this is just a null pointer dereference, I am thinking that maybe it’s
> not security sensitive?

Doesn't seem like it. Maybe we can add a test then.
Comment 8 Ryosuke Niwa 2021-02-05 17:08:49 PST
(In reply to Darin Adler from comment #2)
> I’m going to grab this and fix it, but I don’t plan to add a regression test.
> Ryosuke, let me know if that’s not right.

So if it's just a nullptr crash without any security implications, we want to add a test like any other bug.
Comment 9 Darin Adler 2021-02-05 17:10:09 PST
The only way to test it is the IPC fuzzing; not sure.
Comment 10 Ryosuke Niwa 2021-02-05 17:11:49 PST
Hm... this IPC test might be a bit problematic in that it may break right away when the format of WebConnection_HandleMessage changes so maybe it's okay not to add it.
Comment 11 Ryosuke Niwa 2021-02-05 17:12:25 PST
(In reply to Darin Adler from comment #9)
> The only way to test it is the IPC fuzzing; not sure.

FWIW, IPC fuzzing API is available in debug & ASAN release builds so if we added API, we can test it but it's true that this input is probably not going to be valid in a long term.