Bug 231674

Summary: Support in-process testing of IPC messages
Product: WebKit Reporter: Nils <nls.wk>
Component: WebKit2Assignee: Kimmo Kinnunen <kkinnunen>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, bfulgham, cdumez, ddkilzer, ews-watchlist, fpizlo, gyuyoung.kim, hi, kkinnunen, mkwst, rniwa, ryuan.choi, sergio, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=236589
https://bugs.webkit.org/show_bug.cgi?id=236590
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch none

Description Nils 2021-10-13 06:30:22 PDT
Add entry point for in-process IPC fuzzing and export function to be used during fuzzing.
Comment 1 Nils 2021-10-13 06:44:00 PDT
<rdar://84189314>
Comment 2 Radar WebKit Bug Importer 2021-10-13 06:46:11 PDT
<rdar://problem/84195963>
Comment 3 Nils 2021-10-13 07:07:56 PDT
Created attachment 441072 [details]
Patch
Comment 4 Nils 2021-10-13 08:20:36 PDT
Created attachment 441081 [details]
Patch

Fix iOS builds
Comment 5 Nils 2021-10-13 08:58:42 PDT
Created attachment 441086 [details]
Patch
Comment 6 Kimmo Kinnunen 2021-10-15 05:13:44 PDT
It's a great start!

Once this is in, it might be very hard to fix WebKit IPC without breaking the fuzzer. As such it'd be great if:
1) It didn't expect things to be called inside the implementation ad-hoc
2) It used the expected threading model

It's not clear to me whether the "start fuzzing" message is a specific WebKit message you want to override, or do you just want "start fuzzing" message. Below is written from the standpoint that one just wants to start the testing, not override a specific message.

I think more structural approach would be:
1) Define IPC interface IPCTestingAPI.messages.in:
messages -> IPCTester NotRefCounted {
  StartMessageTesting()
}


2) Register this interface in all the project toplevel processes you want to test.

3) When receiving the message, post a task to main runloop to run the externally loaded fuzz driver

4) The fuzz driver should get WebKit function and context in, so that when the fuzz driver calls into webkit with the test case, webkit code can just use the context to setup the call state.

So the pseudocode would be:

extern "C" typedef (*OneInputFunc)(char* data, size_t sz, void* context);

extern "C" void (*messageTestDriver)(OneInputFunc, void* context);

extern "C" void testOneMessage(char* data, size_t sz, void* context)
{
    IPC::Connection* connection = reinterpret_cast<Connection*>(context);
    auto decoder = IPC::Decoder::create(data, sz, nullptr, Vector<IPC::Attachment> { });
    if (!decoder)
        return 0;
    if (!decoder->isValid())
        return 0;
    connection->dispatchIncomingMessageForTesting(WTFMove(decoder));
}

void AuxiliaryProcess::startMessageTesting()
{
    if (!messageTestDriver)
        messageTestDriver = dyldload(..);
    // When receiving the StartMessageTesting, run the tester in next mainloop iteration.
    RunLoop().dispatch([connection = m_connection] {
        m_connection->setIgnoreInvalidMessageForTesting();
        // Ref above keeps the connection alive.
        messageTestDriver(testOneMessage, connection.get());
    }
};

void Connection::dispatchIncomingMessageForTesting(Decoder decoder) {
   m_connectionQueue->dispatch([decoder = WTFMove(decoder)] {
       processIncomingMessage(WTFMove(decoder));
   });
}


--

In your current impl, you'd run your fuzzer driver loop inside one of the Connection private implementation methods for dispatching one message, re-entering the connection message dispatch logic while sending multiple messages. This is not the way it's intended to work.


Note: running the message testing in main loop will have the side-effect that the message processing loop runs concurrently. This means that if other processes send messages to the process you're testing, these messages would be processed in between your test messages. This may or may not interfere with your goal, so care should be taken if this must be further changed. Same with message sends, IIUC correctly, your current never-ending fuzz loop would prevent any messages sent by the tested process to be queued instead of actually sent. When run in main loop, sent messages would be sent business as usual.

also, you could just name the methods, variables and such in "normal webkit way". E.g. ptfFuzzNow, TryLoadPTF all break the logic of the naming. Even if PFT is a project of some kind, it would be less surprising to use just normal, general names to describe the concept.

fuzz == message testing ?
pftFuzzNow == messageTestDriver?

PTFTestOneInputConnection = messageTestOneInput ? testMessage? sendTestMessage ?
Comment 7 Wenson Hsieh 2021-10-15 14:48:47 PDT
(Adding a few folks who might have additional context)
Comment 8 Nils 2021-11-18 06:41:07 PST
Created attachment 444666 [details]
Patch
Comment 9 Kimmo Kinnunen 2021-11-23 07:16:35 PST
Created attachment 445030 [details]
Patch
Comment 10 Kimmo Kinnunen 2021-11-23 07:20:28 PST
Created attachment 445032 [details]
Patch
Comment 11 Kimmo Kinnunen 2021-12-21 05:32:18 PST
Created attachment 447713 [details]
Patch
Comment 12 Kimmo Kinnunen 2021-12-21 06:03:05 PST
Created attachment 447714 [details]
Patch
Comment 13 Kimmo Kinnunen 2021-12-21 07:59:59 PST
Created attachment 447716 [details]
Patch
Comment 14 Brent Fulgham 2022-01-20 09:37:48 PST
Comment on attachment 447716 [details]
Patch

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

r=me

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:286
> +#if ENABLE(IPC_TESTING_API)

Seems like this could just be part of the protected region of lines 279-284?

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:374
> +#if ENABLE(IPC_TESTING_API)

Seems like this could be inside the IPC_TESTING_API macro area (lines 367-371)?

> Source/WebKit/Platform/IPC/Connection.cpp:1291
> +    if (!iterator->value.isValidKey(identifier))

Why don't we want to mark the connection as dispatching an invalid message anymore?

> Source/WebKit/Platform/IPC/Decoder.h:51
> +    using BufferDeallocator = Function<void(const uint8_t*, size_t)>;

Much nicer!

> Source/WebKit/WebAuthnProcess/WebAuthnConnectionToWebProcess.h:87
> +

Nit: Extra blank line!
Comment 15 Kimmo Kinnunen 2022-01-20 11:56:45 PST
Comment on attachment 447716 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:286
>> +#if ENABLE(IPC_TESTING_API)
> 
> Seems like this could just be part of the protected region of lines 279-284?

No, people add new checks, so they should add new checks between these ifdefs. Added a comment.

>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:374
>> +#if ENABLE(IPC_TESTING_API)
> 
> Seems like this could be inside the IPC_TESTING_API macro area (lines 367-371)?

Same here. The above ifdef is part of a list, where as below ifdef is part of the function postamble . added a comment.

>> Source/WebKit/Platform/IPC/Connection.cpp:1291
>> +    if (!iterator->value.isValidKey(identifier))
> 
> Why don't we want to mark the connection as dispatching an invalid message anymore?

moved to the caller since there's two error cases (two nullptr exits)

>> Source/WebKit/WebAuthnProcess/WebAuthnConnectionToWebProcess.h:87
>> +
> 
> Nit: Extra blank line!

done
Comment 16 Kimmo Kinnunen 2022-01-20 11:57:31 PST
Created attachment 449600 [details]
Patch
Comment 17 EWS 2022-01-21 00:34:03 PST
Committed r288354 (246260@main): <https://commits.webkit.org/246260@main>

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