IPC: Avoid closing Channel endpoints on error

We never want to explicitly close a Channel endpoint until after the
corresponding child process is dead. This prevents that from happening
in some cases where it's still possible today.

[email protected]

Bug: 859386,861607,863430
Change-Id: I2858b6e011e06a000ae976c1e6ae96595cf67a85
Reviewed-on: https://chromium-review.googlesource.com/1135712
Commit-Queue: Ken Rockot <[email protected]>
Reviewed-by: Ken Rockot <[email protected]>
Reviewed-by: John Abd-El-Malek <[email protected]>
Cr-Commit-Position: refs/heads/master@{#575115}

ipc/DEPS

@@ -7,18 +7,11 @@ include_rules = [
 ]
 
 specific_include_rules = {
-  "ipc_test_base\.h": [
-    "+mojo/core/test",
-  ],
-  "ipc_mojo_(bootstrap_unittest|perftest)\.cc": [
-    "+mojo/core/embedder",
-    "+mojo/core/test",
-  ],
-  "ipc_.*perftest.*\.cc": [
+  "ipc_(test|perftest)_(base|util)\.(cc|h)": [
     "+mojo/core/embedder",
     "+mojo/core/test",
   ],
-  "run_all_(unit|perf)tests\.cc": [
+  ".*(unit|perf)tests?\.cc": [
     "+mojo/core/embedder",
     "+mojo/core/test",
   ],

ipc/ipc_channel_mojo_unittest.cc

@@ -22,12 +22,14 @@
 #include "base/memory/shared_memory.h"
 #include "base/memory/shared_memory_mapping.h"
 #include "base/message_loop/message_loop.h"
+#include "base/optional.h"
 #include "base/path_service.h"
 #include "base/pickle.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/stringprintf.h"
 #include "base/synchronization/waitable_event.h"
+#include "base/test/bind_test_util.h"
 #include "base/test/test_io_thread.h"
 #include "base/test/test_shared_memory_util.h"
 #include "base/test/test_timeouts.h"
@@ -44,6 +46,8 @@
 #include "ipc/ipc_test.mojom.h"
 #include "ipc/ipc_test_base.h"
 #include "ipc/ipc_test_channel_listener.h"
+#include "mojo/core/embedder/embedder.h"
+#include "mojo/public/cpp/bindings/lib/validation_errors.h"
 #include "mojo/public/cpp/system/wait.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -107,7 +111,10 @@ class TestListenerBase : public IPC::Listener {
 
   void set_sender(IPC::Sender* sender) { sender_ = sender; }
   IPC::Sender* sender() const { return sender_; }
-  void RunQuitClosure() { std::move(quit_closure_).Run(); }
+  void RunQuitClosure() {
+    if (quit_closure_)
+      std::move(quit_closure_).Run();
+  }
 
  private:
   IPC::Sender* sender_ = nullptr;
@@ -241,6 +248,120 @@ TEST_F(IPCChannelMojoTest, SendFailWithPendingMessages) {
   DestroyChannel();
 }
 
+class ListenerThatBindsATestStructPasser : public IPC::Listener,
+                                           public IPC::mojom::TestStructPasser {
+ public:
+  ListenerThatBindsATestStructPasser() : binding_(this) {}
+
+  bool OnMessageReceived(const IPC::Message& message) override { return true; }
+
+  void OnChannelConnected(int32_t peer_pid) override {}
+
+  void OnChannelError() override { NOTREACHED(); }
+
+  void OnAssociatedInterfaceRequest(
+      const std::string& interface_name,
+      mojo::ScopedInterfaceEndpointHandle handle) override {
+    CHECK_EQ(interface_name, IPC::mojom::TestStructPasser::Name_);
+    binding_.Bind(
+        IPC::mojom::TestStructPasserAssociatedRequest(std::move(handle)));
+  }
+
+ private:
+  // IPC::mojom::TestStructPasser:
+  void Pass(IPC::mojom::TestStructPtr) override { NOTREACHED(); }
+
+  mojo::AssociatedBinding<IPC::mojom::TestStructPasser> binding_;
+};
+
+class ListenerThatExpectsNoError : public IPC::Listener {
+ public:
+  ListenerThatExpectsNoError(base::OnceClosure connect_closure,
+                             base::OnceClosure quit_closure)
+      : connect_closure_(std::move(connect_closure)),
+        quit_closure_(std::move(quit_closure)) {}
+
+  bool OnMessageReceived(const IPC::Message& message) override {
+    base::PickleIterator iter(message);
+    std::string should_be_ok;
+    EXPECT_TRUE(iter.ReadString(&should_be_ok));
+    EXPECT_EQ(should_be_ok, "OK");
+    std::move(quit_closure_).Run();
+    return true;
+  }
+
+  void OnChannelConnected(int32_t peer_pid) override {
+    std::move(connect_closure_).Run();
+  }
+
+  void OnChannelError() override { NOTREACHED(); }
+
+ private:
+  base::OnceClosure connect_closure_;
+  base::OnceClosure quit_closure_;
+};
+
+DEFINE_IPC_CHANNEL_MOJO_TEST_CLIENT(
+    IPCChannelMojoNoImplicitChanelClosureClient) {
+  base::RunLoop wait_to_connect_loop;
+  base::RunLoop wait_to_quit_loop;
+  ListenerThatExpectsNoError listener(wait_to_connect_loop.QuitClosure(),
+                                      wait_to_quit_loop.QuitClosure());
+  Connect(&listener);
+  wait_to_connect_loop.Run();
+
+  IPC::mojom::TestStructPasserAssociatedPtr passer;
+  channel()->GetAssociatedInterfaceSupport()->GetRemoteAssociatedInterface(
+      &passer);
+
+  // This avoids hitting DCHECKs in the serialization code meant to stop us from
+  // making such "mistakes" as the one we're about to make below.
+  mojo::internal::SerializationWarningObserverForTesting suppress_those_dchecks;
+
+  // Send an invalid message. The TestStruct argument is not allowed to be null.
+  // This will elicit a validation error in the parent process, but should not
+  // actually disconnect the channel.
+  passer->Pass(nullptr);
+
+  // Wait until the parent says it's OK to quit, so it has time to verify its
+  // expected behavior.
+  wait_to_quit_loop.Run();
+
+  Close();
+}
+
+TEST_F(IPCChannelMojoTest, NoImplicitChannelClosure) {
+  // Verifies that OnChannelError is not invoked due to conditions other than
+  // peer closure (e.g. a malformed inbound message). Instead we should always
+  // be able to handle validation errors via Mojo bad message reporting.
+
+  // NOTE: We can't create a RunLoop before Init() is called, but we have to set
+  // the default ProcessErrorCallback (which we want to reference the RunLoop)
+  // before Init() launches a child process. Hence the base::Optional here.
+  base::Optional<base::RunLoop> wait_for_error_loop;
+  bool process_error_received = false;
+  mojo::core::SetDefaultProcessErrorCallback(
+      base::BindLambdaForTesting([&](const std::string&) {
+        process_error_received = true;
+        wait_for_error_loop->Quit();
+      }));
+
+  Init("IPCChannelMojoNoImplicitChanelClosureClient");
+
+  wait_for_error_loop.emplace();
+  ListenerThatBindsATestStructPasser listener;
+  CreateChannel(&listener);
+  ASSERT_TRUE(ConnectChannel());
+
+  wait_for_error_loop->Run();
+  EXPECT_TRUE(process_error_received);
+
+  // Tell the child it can quit and wait for it to shut down.
+  ListenerThatExpectsOK::SendOK(channel());
+  EXPECT_TRUE(WaitForClientShutdown());
+  DestroyChannel();
+}
+
 struct TestingMessagePipe {
   TestingMessagePipe() {
     EXPECT_EQ(MOJO_RESULT_OK, mojo::CreateMessagePipe(nullptr, &self, &peer));

ipc/ipc_mojo_bootstrap.cc

@@ -131,6 +131,7 @@ class ChannelAssociatedGroupController
     connector_->set_connection_error_handler(
         base::Bind(&ChannelAssociatedGroupController::OnPipeError,
                    base::Unretained(this)));
+    connector_->set_enforce_errors_from_incoming_receiver(false);
     connector_->SetWatcherHeapProfilerTag("IPC Channel");
   }
 
@@ -318,13 +319,29 @@ class ChannelAssociatedGroupController
   }
 
   void RaiseError() override {
-    if (task_runner_->BelongsToCurrentThread()) {
-      connector_->RaiseError();
-    } else {
-      task_runner_->PostTask(
-          FROM_HERE,
-          base::Bind(&ChannelAssociatedGroupController::RaiseError, this));
-    }
+    // We ignore errors on channel endpoints, leaving the pipe open. There are
+    // good reasons for this:
+    //
+    //   * We should never close a channel endpoint in either process as long as
+    //     the child process is still alive. The child's endpoint should only be
+    //     closed implicitly by process death, and the browser's endpoint should
+    //     only be closed after the child process is confirmed to be dead. Crash
+    //     reporting logic in Chrome relies on this behavior in order to do the
+    //     right thing.
+    //
+    //   * There are two interesting conditions under which RaiseError() can be
+    //     implicitly reached: an incoming message fails validation, or the
+    //     local endpoint drops a response callback without calling it.
+    //
+    //   * In the validation case, we also report the message as bad, and this
+    //     will imminently trigger the common bad-IPC path in the browser,
+    //     causing the browser to kill the offending renderer.
+    //
+    //   * In the dropped response callback case, the net result of ignoring the
+    //     issue is generally innocuous. While indicative of programmer error,
+    //     it's not a severe failure and is already covered by separate DCHECKs.
+    //
+    // See https://crbug.com/861607 for additional discussion.
   }
 
   bool PrefersSerializedMessages() override { return true; }

ipc/ipc_test.mojom

@@ -25,6 +25,12 @@ interface PingReceiver {
   Ping() => ();
 };
 
+struct TestStruct {};
+
+interface TestStructPasser {
+  Pass(TestStruct s);
+};
+
 interface IndirectTestDriver {
   GetPingReceiver(associated PingReceiver& request);
 };