From 2f2588cf4f021ef31621460980a2cc09490e2bea Mon Sep 17 00:00:00 2001 From: Nika Layzell Date: Fri, 26 May 2023 17:44:57 +0000 Subject: [PATCH] Bug 1828389 - Ensure IPC channel is closed with error after KillHard, r=ipc-reviewers,mccr8 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 does not impact actors which ignore processing errors. Differential Revision: https://phabricator.services.mozilla.com/D178383 --- dom/ipc/ContentParent.cpp | 23 ++++++++++++------- dom/media/ipc/RemoteDecoderManagerChild.cpp | 2 +- dom/media/ipc/RemoteDecoderManagerChild.h | 2 +- gfx/ipc/VsyncBridgeChild.cpp | 2 +- gfx/ipc/VsyncBridgeChild.h | 2 +- gfx/layers/ipc/CompositorManagerChild.cpp | 2 +- gfx/layers/ipc/CompositorManagerChild.h | 2 +- gfx/layers/ipc/ImageBridgeChild.cpp | 2 +- gfx/layers/ipc/ImageBridgeChild.h | 2 +- .../ipc/UiCompositorControllerChild.cpp | 2 +- gfx/layers/ipc/UiCompositorControllerChild.h | 2 +- gfx/layers/ipc/VideoBridgeChild.cpp | 2 +- gfx/layers/ipc/VideoBridgeChild.h | 2 +- gfx/vr/ipc/VRManagerChild.cpp | 2 +- gfx/vr/ipc/VRManagerChild.h | 2 +- ipc/glue/ProtocolUtils.cpp | 9 ++++++-- ipc/glue/ProtocolUtils.h | 5 ++-- ipc/ipdl/test/cxx/TestActorPunning.cpp | 2 +- ipc/ipdl/test/cxx/TestActorPunning.h | 2 +- ipc/ipdl/test/cxx/TestBadActor.cpp | 2 +- ipc/ipdl/test/cxx/TestBadActor.h | 2 +- 21 files changed, 43 insertions(+), 30 deletions(-) diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp index 2f4b23366e30..28f5dff602d0 100644 --- a/dom/ipc/ContentParent.cpp +++ b/dom/ipc/ContentParent.cpp @@ -1879,17 +1879,18 @@ bool ContentParent::ShutDownProcess(ShutDownMethod aMethod) { qms->AbortOperationsForProcess(mChildID); } - // If Close() fails with an error, we'll end up back in this function, but - // with aMethod = CLOSE_CHANNEL_WITH_ERROR. - - if (aMethod == CLOSE_CHANNEL) { + if (aMethod == CLOSE_CHANNEL || aMethod == CLOSE_CHANNEL_WITH_ERROR) { if (!mCalledClose) { MaybeLogBlockShutdownDiagnostics( this, "ShutDownProcess: Closing channel.", __FILE__, __LINE__); - // Close() can only be called once: It kicks off the destruction - // sequence. + // Close()/CloseWithError() can only be called once: They kick off the + // destruction sequence. mCalledClose = true; - Close(); + if (aMethod == CLOSE_CHANNEL_WITH_ERROR) { + CloseWithError(); + } else { + Close(); + } } result = true; } @@ -2074,10 +2075,11 @@ void ContentParent::ProcessingError(Result aCode, const char* aReason) { if (MsgDropped == aCode) { return; } -#ifndef FUZZING // Other errors are big deals. +#ifndef FUZZING KillHard(aReason); #endif + ShutDownProcess(CLOSE_CHANNEL_WITH_ERROR); } void ContentParent::ActorDestroy(ActorDestroyReason why) { @@ -4515,6 +4517,7 @@ void ContentParent::KillHard(const char* aReason) { ProcessHandle otherProcessHandle; if (!base::OpenProcessHandle(OtherPid(), &otherProcessHandle)) { NS_ERROR("Failed to open child process when attempting kill."); + ShutDownProcess(CLOSE_CHANNEL_WITH_ERROR); return; } @@ -4535,6 +4538,10 @@ void ContentParent::KillHard(const char* aReason) { mSubprocess->SetAlreadyDead(); } + // After we've killed the remote process, also ensure we close the IPC channel + // with an error to immediately stop all IPC communication on this channel. + ShutDownProcess(CLOSE_CHANNEL_WITH_ERROR); + // EnsureProcessTerminated has responsibilty for closing otherProcessHandle. XRE_GetIOMessageLoop()->PostTask( NewRunnableFunction("EnsureProcessTerminatedRunnable", diff --git a/dom/media/ipc/RemoteDecoderManagerChild.cpp b/dom/media/ipc/RemoteDecoderManagerChild.cpp index 08c2b6a4eb15..3c0e75ae1634 100644 --- a/dom/media/ipc/RemoteDecoderManagerChild.cpp +++ b/dom/media/ipc/RemoteDecoderManagerChild.cpp @@ -859,7 +859,7 @@ void RemoteDecoderManagerChild::DeallocateSurfaceDescriptor( }))); } -void RemoteDecoderManagerChild::HandleFatalError(const char* aMsg) const { +void RemoteDecoderManagerChild::HandleFatalError(const char* aMsg) { dom::ContentChild::FatalErrorIfNotUsingGPUProcess(aMsg, OtherPid()); } diff --git a/dom/media/ipc/RemoteDecoderManagerChild.h b/dom/media/ipc/RemoteDecoderManagerChild.h index 22b46c5307f4..effeb92d9304 100644 --- a/dom/media/ipc/RemoteDecoderManagerChild.h +++ b/dom/media/ipc/RemoteDecoderManagerChild.h @@ -107,7 +107,7 @@ class RemoteDecoderManagerChild final RemoteDecodeIn aLocation); protected: - void HandleFatalError(const char* aMsg) const override; + void HandleFatalError(const char* aMsg) override; PRemoteDecoderChild* AllocPRemoteDecoderChild( const RemoteDecoderInfoIPDL& aRemoteDecoderInfo, diff --git a/gfx/ipc/VsyncBridgeChild.cpp b/gfx/ipc/VsyncBridgeChild.cpp index 1d4f54603737..044a24f32025 100644 --- a/gfx/ipc/VsyncBridgeChild.cpp +++ b/gfx/ipc/VsyncBridgeChild.cpp @@ -119,7 +119,7 @@ void VsyncBridgeChild::ProcessingError(Result aCode, const char* aReason) { "Processing error in VsyncBridgeChild"); } -void VsyncBridgeChild::HandleFatalError(const char* aMsg) const { +void VsyncBridgeChild::HandleFatalError(const char* aMsg) { dom::ContentChild::FatalErrorIfNotUsingGPUProcess(aMsg, OtherPid()); } diff --git a/gfx/ipc/VsyncBridgeChild.h b/gfx/ipc/VsyncBridgeChild.h index c74428f9abb1..a0727ce5fe31 100644 --- a/gfx/ipc/VsyncBridgeChild.h +++ b/gfx/ipc/VsyncBridgeChild.h @@ -31,7 +31,7 @@ class VsyncBridgeChild final : public PVsyncBridgeChild { void NotifyVsync(const VsyncEvent& aVsync, const layers::LayersId& aLayersId); - void HandleFatalError(const char* aMsg) const override; + void HandleFatalError(const char* aMsg) override; private: VsyncBridgeChild(RefPtr, const uint64_t& aProcessToken); diff --git a/gfx/layers/ipc/CompositorManagerChild.cpp b/gfx/layers/ipc/CompositorManagerChild.cpp index e48bfca3202a..89e72ddc01ec 100644 --- a/gfx/layers/ipc/CompositorManagerChild.cpp +++ b/gfx/layers/ipc/CompositorManagerChild.cpp @@ -211,7 +211,7 @@ void CompositorManagerChild::ActorDestroy(ActorDestroyReason aReason) { } } -void CompositorManagerChild::HandleFatalError(const char* aMsg) const { +void CompositorManagerChild::HandleFatalError(const char* aMsg) { dom::ContentChild::FatalErrorIfNotUsingGPUProcess(aMsg, OtherPid()); } diff --git a/gfx/layers/ipc/CompositorManagerChild.h b/gfx/layers/ipc/CompositorManagerChild.h index d4bc978714f0..f92ebde4609e 100644 --- a/gfx/layers/ipc/CompositorManagerChild.h +++ b/gfx/layers/ipc/CompositorManagerChild.h @@ -83,7 +83,7 @@ class CompositorManagerChild : public PCompositorManagerChild { void ActorDestroy(ActorDestroyReason aReason) override; - void HandleFatalError(const char* aMsg) const override; + void HandleFatalError(const char* aMsg) override; void ProcessingError(Result aCode, const char* aReason) override; diff --git a/gfx/layers/ipc/ImageBridgeChild.cpp b/gfx/layers/ipc/ImageBridgeChild.cpp index f2113723167b..420ec569241b 100644 --- a/gfx/layers/ipc/ImageBridgeChild.cpp +++ b/gfx/layers/ipc/ImageBridgeChild.cpp @@ -920,7 +920,7 @@ bool ImageBridgeChild::CanSend() const { return mCanSend; } -void ImageBridgeChild::HandleFatalError(const char* aMsg) const { +void ImageBridgeChild::HandleFatalError(const char* aMsg) { dom::ContentChild::FatalErrorIfNotUsingGPUProcess(aMsg, OtherPid()); } diff --git a/gfx/layers/ipc/ImageBridgeChild.h b/gfx/layers/ipc/ImageBridgeChild.h index 8482b68b2285..5777725ae097 100644 --- a/gfx/layers/ipc/ImageBridgeChild.h +++ b/gfx/layers/ipc/ImageBridgeChild.h @@ -312,7 +312,7 @@ class ImageBridgeChild final : public PImageBridgeChild, bool InForwarderThread() override { return InImageBridgeChildThread(); } - void HandleFatalError(const char* aMsg) const override; + void HandleFatalError(const char* aMsg) override; wr::MaybeExternalImageId GetNextExternalImageId() override; diff --git a/gfx/layers/ipc/UiCompositorControllerChild.cpp b/gfx/layers/ipc/UiCompositorControllerChild.cpp index 50e9051fdef1..c1bf49bc27c8 100644 --- a/gfx/layers/ipc/UiCompositorControllerChild.cpp +++ b/gfx/layers/ipc/UiCompositorControllerChild.cpp @@ -210,7 +210,7 @@ void UiCompositorControllerChild::ProcessingError(Result aCode, } } -void UiCompositorControllerChild::HandleFatalError(const char* aMsg) const { +void UiCompositorControllerChild::HandleFatalError(const char* aMsg) { dom::ContentChild::FatalErrorIfNotUsingGPUProcess(aMsg, OtherPid()); } diff --git a/gfx/layers/ipc/UiCompositorControllerChild.h b/gfx/layers/ipc/UiCompositorControllerChild.h index fa0b6e44c1a6..bf543ee6a30f 100644 --- a/gfx/layers/ipc/UiCompositorControllerChild.h +++ b/gfx/layers/ipc/UiCompositorControllerChild.h @@ -77,7 +77,7 @@ class UiCompositorControllerChild final protected: void ActorDestroy(ActorDestroyReason aWhy) override; void ProcessingError(Result aCode, const char* aReason) override; - void HandleFatalError(const char* aMsg) const override; + void HandleFatalError(const char* aMsg) override; mozilla::ipc::IPCResult RecvToolbarAnimatorMessageFromCompositor( const int32_t& aMessage); mozilla::ipc::IPCResult RecvRootFrameMetrics(const ScreenPoint& aScrollOffset, diff --git a/gfx/layers/ipc/VideoBridgeChild.cpp b/gfx/layers/ipc/VideoBridgeChild.cpp index 6a75408650e4..cb285cd7b92e 100644 --- a/gfx/layers/ipc/VideoBridgeChild.cpp +++ b/gfx/layers/ipc/VideoBridgeChild.cpp @@ -166,7 +166,7 @@ bool VideoBridgeChild::IsSameProcess() const { return OtherPid() == base::GetCurrentProcId(); } -void VideoBridgeChild::HandleFatalError(const char* aMsg) const { +void VideoBridgeChild::HandleFatalError(const char* aMsg) { dom::ContentChild::FatalErrorIfNotUsingGPUProcess(aMsg, OtherPid()); } diff --git a/gfx/layers/ipc/VideoBridgeChild.h b/gfx/layers/ipc/VideoBridgeChild.h index ba3fb77e3017..c3014c7d7759 100644 --- a/gfx/layers/ipc/VideoBridgeChild.h +++ b/gfx/layers/ipc/VideoBridgeChild.h @@ -63,7 +63,7 @@ class VideoBridgeChild final : public PVideoBridgeChild, static void Open(Endpoint&& aEndpoint); protected: - void HandleFatalError(const char* aMsg) const override; + void HandleFatalError(const char* aMsg) override; bool DispatchAllocShmemInternal(size_t aSize, mozilla::ipc::Shmem* aShmem, bool aUnsafe); void ProxyAllocShmemNow(SynchronousTask* aTask, size_t aSize, diff --git a/gfx/vr/ipc/VRManagerChild.cpp b/gfx/vr/ipc/VRManagerChild.cpp index ec812e2e4c80..9bfc74e3af5c 100644 --- a/gfx/vr/ipc/VRManagerChild.cpp +++ b/gfx/vr/ipc/VRManagerChild.cpp @@ -589,7 +589,7 @@ void VRManagerChild::StopActivity() { Unused << SendStopActivity(); } -void VRManagerChild::HandleFatalError(const char* aMsg) const { +void VRManagerChild::HandleFatalError(const char* aMsg) { dom::ContentChild::FatalErrorIfNotUsingGPUProcess(aMsg, OtherPid()); } diff --git a/gfx/vr/ipc/VRManagerChild.h b/gfx/vr/ipc/VRManagerChild.h index cb689c218a49..7448007c0711 100644 --- a/gfx/vr/ipc/VRManagerChild.h +++ b/gfx/vr/ipc/VRManagerChild.h @@ -102,7 +102,7 @@ class VRManagerChild : public PVRManagerChild { void FireDOMVRDisplayPresentChangeEvent(uint32_t aDisplayID); void FireDOMVRDisplayConnectEventsForLoad(VRManagerEventObserver* aObserver); - void HandleFatalError(const char* aMsg) const override; + void HandleFatalError(const char* aMsg) override; void ActorDestroy(ActorDestroyReason aReason) override; void RunPuppet(const nsTArray& aBuffer, dom::Promise* aPromise, diff --git a/ipc/glue/ProtocolUtils.cpp b/ipc/glue/ProtocolUtils.cpp index 91e888e3b0b8..51fb4d7e5690 100644 --- a/ipc/glue/ProtocolUtils.cpp +++ b/ipc/glue/ProtocolUtils.cpp @@ -389,17 +389,20 @@ Maybe IProtocol::ReadActor(IPC::MessageReader* aReader, return Some(listener); } -void IProtocol::FatalError(const char* const aErrorMsg) const { +void IProtocol::FatalError(const char* const aErrorMsg) { HandleFatalError(aErrorMsg); } -void IProtocol::HandleFatalError(const char* aErrorMsg) const { +void IProtocol::HandleFatalError(const char* aErrorMsg) { if (IProtocol* manager = Manager()) { manager->HandleFatalError(aErrorMsg); return; } mozilla::ipc::FatalError(aErrorMsg, mSide == ParentSide); + if (CanSend()) { + GetIPCChannel()->CloseWithError(); + } } bool IProtocol::AllocShmem(size_t aSize, Shmem* aOutMem) { @@ -630,6 +633,8 @@ void IToplevelProtocol::NotifyImpendingShutdown() { void IToplevelProtocol::Close() { GetIPCChannel()->Close(); } +void IToplevelProtocol::CloseWithError() { GetIPCChannel()->CloseWithError(); } + void IToplevelProtocol::SetReplyTimeoutMs(int32_t aTimeoutMs) { GetIPCChannel()->SetReplyTimeoutMs(aTimeoutMs); } diff --git a/ipc/glue/ProtocolUtils.h b/ipc/glue/ProtocolUtils.h index b9d961fe5aa1..aa398453ae86 100644 --- a/ipc/glue/ProtocolUtils.h +++ b/ipc/glue/ProtocolUtils.h @@ -242,8 +242,8 @@ class IProtocol : public HasResultCodes { bool AllocUnsafeShmem(size_t aSize, Shmem* aOutMem); bool DeallocShmem(Shmem& aMem); - void FatalError(const char* const aErrorMsg) const; - virtual void HandleFatalError(const char* aErrorMsg) const; + void FatalError(const char* const aErrorMsg); + virtual void HandleFatalError(const char* aErrorMsg); protected: virtual ~IProtocol(); @@ -433,6 +433,7 @@ class IToplevelProtocol : public IProtocol { void NotifyImpendingShutdown(); void Close(); + void CloseWithError(); void SetReplyTimeoutMs(int32_t aTimeoutMs); diff --git a/ipc/ipdl/test/cxx/TestActorPunning.cpp b/ipc/ipdl/test/cxx/TestActorPunning.cpp index 5ba3b637f244..5825796f5479 100644 --- a/ipc/ipdl/test/cxx/TestActorPunning.cpp +++ b/ipc/ipdl/test/cxx/TestActorPunning.cpp @@ -23,7 +23,7 @@ mozilla::ipc::IPCResult TestActorPunningParent::RecvPun( // By default, fatal errors kill the parent process, but this makes it // hard to test, so instead we use the previous behavior and kill the // child process. -void TestActorPunningParent::HandleFatalError(const char* aErrorMsg) const { +void TestActorPunningParent::HandleFatalError(const char* aErrorMsg) { if (!!strcmp(aErrorMsg, "Error deserializing 'PTestActorPunningSubParent'")) { fail("wrong fatal error"); } diff --git a/ipc/ipdl/test/cxx/TestActorPunning.h b/ipc/ipdl/test/cxx/TestActorPunning.h index 9c19a0e0ddb9..cd88106c0cee 100644 --- a/ipc/ipdl/test/cxx/TestActorPunning.h +++ b/ipc/ipdl/test/cxx/TestActorPunning.h @@ -38,7 +38,7 @@ class TestActorPunningParent : public PTestActorPunningParent { QuitParent(); } - virtual void HandleFatalError(const char* aErrorMsg) const override; + virtual void HandleFatalError(const char* aErrorMsg) override; }; class TestActorPunningPunnedParent : public PTestActorPunningPunnedParent { diff --git a/ipc/ipdl/test/cxx/TestBadActor.cpp b/ipc/ipdl/test/cxx/TestBadActor.cpp index 42a0faf19d82..6c6991dbb225 100644 --- a/ipc/ipdl/test/cxx/TestBadActor.cpp +++ b/ipc/ipdl/test/cxx/TestBadActor.cpp @@ -19,7 +19,7 @@ void TestBadActorParent::Main() { // By default, fatal errors kill the parent process, but this makes it // hard to test, so instead we use the previous behavior and kill the // child process. -void TestBadActorParent::HandleFatalError(const char* aErrorMsg) const { +void TestBadActorParent::HandleFatalError(const char* aErrorMsg) { if (!!strcmp(aErrorMsg, "incoming message racing with actor deletion")) { fail("wrong fatal error"); } diff --git a/ipc/ipdl/test/cxx/TestBadActor.h b/ipc/ipdl/test/cxx/TestBadActor.h index 0157b898f409..cefe0288f40a 100644 --- a/ipc/ipdl/test/cxx/TestBadActor.h +++ b/ipc/ipdl/test/cxx/TestBadActor.h @@ -31,7 +31,7 @@ class TestBadActorParent : public PTestBadActorParent { QuitParent(); } - virtual void HandleFatalError(const char* aErrorMsg) const override; + virtual void HandleFatalError(const char* aErrorMsg) override; PTestBadActorSubParent* AllocPTestBadActorSubParent();