1828389 - IPDL constructor failures should cause actor teardown

Status: RESOLVED FIXED

(Core :: IPC, defect, P3)

Description

[Christian Holler (:decoder)]

In experimental IPC fuzzing, we found the following crash on mozilla-central revision f14ed3bab724+ (fuzzing-asan-nyx-opt build):

=================================================================
==1911==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fffcdeb41f0 bp 0x7ffffffe67d0 sp 0x7ffffffe6680 T0)
    #0 0x7fffcdeb41f0 in mozilla::dom::ExternalHelperAppParent::RecvOnStopRequest(nsresult const&) uriloader/exthandler/ExternalHelperAppParent.cpp:150:14
    #1 0x7fffcdf1afb6 in mozilla::dom::PExternalHelperAppParent::OnMessageReceived(IPC::Message const&) objdir-ff-aflpp/ipc/ipdl/PExternalHelperAppParent.cpp:288:52
    #2 0x7fffda63791c in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) objdir-ff-aflpp/ipc/ipdl/PContentParent.cpp:6655:32
    #3 0x7fffcd39f4d6 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) ipc/glue/MessageChannel.cpp:1806:25
    [...]

I think this is yet another issue in the generated IPDL code, similar to bug 1827440 (patch from that bug is applied).

My guess is the missing null check is here:

https://searchfox.org/mozilla-central/source/__GENERATED__/ipc/ipdl/PExternalHelperAppParent.cpp#291

auto maybe__code = IPC::ReadParam<nsresult>((&(reader__))); fails to read a code (probably due to truncated message?) but the if check following on maybe__code doesn't fail.

In my local build, line 288 of frame #1 is this:

mozilla::ipc::IPCResult __ok = (this)->RecvOnStopRequest(std::move(code));

So we crash when trying to std::move the null reference.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Christian Holler (:decoder)]

Also got similar null crashes for RecvOnDataAvailable and RecvOnStartRequest.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

It seems more likely to me based on the file in comment 1 that the actual issue here is that mListener is null: https://searchfox.org/mozilla-central/rev/8329a650e3b4f866176ae54016702eb35fb8b0d6/uriloader/exthandler/ExternalHelperAppParent.cpp#150. The code cannot be a null reference, as the value returned from maybe__code actually always contains a nsresult unconditionally - it might just be default-initialized.

The listener could have failed to be set if the RecvExternalHelperAppConstructor message returned a failure here: https://searchfox.org/mozilla-central/rev/8329a650e3b4f866176ae54016702eb35fb8b0d6/dom/ipc/ContentParent.cpp#4733,4736.

it looks like we don't actually destroy the actor if the Constructor message handler returns an error, which means that the actor is still available to receive messages in a bugged state like this one:

https://searchfox.org/mozilla-central/source/__GENERATED__/__android-armv7__/ipc/ipdl/PContentParent.cpp#9539-9544

mozilla::ipc::IPCResult __ok = (static_cast<ContentParent*>(this))->RecvPExternalHelperAppConstructor(actor, uri, std::move(loadInfoArgs), std::move(aMimeContentType), std::move(aContentDisposition), std::move(aContentDispositionHint), std::move(aContentDispositionFilename), std::move(aForceSave), std::move(aContentLength), std::move(aWasFileChannel), aReferrer, std::move(aContext), std::move(aShouldCloseWindow));
if ((!(__ok))) {
    mozilla::ipc::ProtocolErrorBreakpoint("Handler returned error code!");
    // Error handled in mozilla::ipc::IPCResult
    return MsgProcessingError;
}

We should perhaps make sure that we actually shut down the actor in this case.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

I've self-assigned this for now to make sure I don't forget about it, but don't know when I'll be able to get around to changing something here. It might be easier to instead change how IPC_FAIL is handled to cause the toplevel actor to always be torn down in those cases, or something similar to that, though I worry that could cause issues as it is a no-op for some actors right now (like PBackground).

Comment 6

[Nika Layzell [:nika] (ni? for response)]

not a regression

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1828389 - Ensure IPC channel is closed with error after KillHard, r=#ipc-reviewers

This patch changes KillHard() such that the IPC channel is immediately
shut down with an error after a KillHard() is performed. This is done by
fixing the previously-broken CLOSE_CHANNEL_WITH_ERROR support in
ShutDownProcess, and calling that method after KillHard().

This ensures that after the process has been killed, no further messages
will be delivered and processed, even if they were sent before the
process was killed.

In addition, the assertions and KillHard calls which are disabled for
fuzzing were changed to also shut down the channel, making fuzzing IPC
errors cause the connection to be terminated like it is in production
for these actors.

This change not impact actors which ignore processing errors.

Depends on D178382

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bbd8b8bbf677
Ensure IPC channel is closed with error after KillHard, r=ipc-reviewers,mccr8

Comment 9

[Serban Stanca [:SerbanS]]

Backed out for mozilla::ThreadEventTarget::Dispatch xpcshell related crashes.

• Backout link
• Push with failures
• Failure Log
• Failure line: PROCESS-CRASH | netwerk/test/unit_ipc/test_chunked_responses_wrap.js | application crashed [@ mozilla::ThreadEventTarget::Dispatch]

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ef28befde81
Ensure IPC channel is closed with error after KillHard, r=ipc-reviewers,mccr8

Comment 11

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/1ef28befde81