Bug 231674 – Support in-process testing of IPC messages

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Nils]

Add entry point for in-process IPC fuzzing and export function to be used during fuzzing.

[Nils]

<rdar://84189314>

[Radar WebKit Bug Importer]

<rdar://problem/84195963>

[Nils]

Created attachment 441072 [details]
Patch

[Nils]

Created attachment 441081 [details]
Patch

Fix iOS builds

[Nils]

Created attachment 441086 [details]
Patch

[Kimmo Kinnunen]

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 ?

[Wenson Hsieh]

(Adding a few folks who might have additional context)

[Nils]

Created attachment 444666 [details]
Patch

[Kimmo Kinnunen]

Created attachment 445030 [details]
Patch

[Kimmo Kinnunen]

Created attachment 445032 [details]
Patch

[Kimmo Kinnunen]

Created attachment 447713 [details]
Patch

[Kimmo Kinnunen]

Created attachment 447714 [details]
Patch

[Kimmo Kinnunen]

Created attachment 447716 [details]
Patch

[Brent Fulgham]

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!

[Kimmo Kinnunen]

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

[Kimmo Kinnunen]

Created attachment 449600 [details]
Patch

[EWS]

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