commit 5ec49204833805a91f65f7575cf01439fe658c3b
author Ken Rockot <rockot@google.com> Mon Nov 28 21:52:01 2022
committer Takuto Ikuta <tikuta@google.com> Fri Dec 02 02:16:41 2022
tree 3e3f29c0715a44312b0541dfabf9b7ad424ab450
parent 2078848791a884fe813128b9e5dddd9a28f83f46

MojoIpcz: Eliminate DataPipe deadlocks

This eliminates unsafe DataPipe and MojoTrap re-entrancy caused by
flush operations on a DataPipe while the DataPipe's or MojoTrap's
internal lock is held.

The general fix is to ensure such locks are released before issuing
flush operations, and in some cases to forego flushing in favor of
basic queries of the pipe's state.

Fixed: 1386093
Change-Id: I99c865c8b2282bbfcf3c02301b6971744ba95953
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4052941
Reviewed-by: Alex Gough <ajgo@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/main@{#1076436}
NOKEYCHECK=True
GitOrigin-RevId: 2317b7635c97a0ab2d6467415abf4bb100aeab2f

src/ipcz/portal.cc

diff --git a/src/ipcz/portal.cc b/src/ipcz/portal.cc
index 270b747..14357e7 100644
--- a/src/ipcz/portal.cc
+++ b/src/ipcz/portal.cc
@@ -12,6 +12,7 @@
 #include "ipcz/local_router_link.h"
 #include "ipcz/operation_context.h"
 #include "ipcz/router.h"
+#include "ipcz/trap_event_dispatcher.h"
 #include "third_party/abseil-cpp/absl/types/span.h"
 #include "util/log.h"
 #include "util/ref_counted.h"
@@ -237,13 +238,14 @@
 
 IpczResult Portal::CommitGet(size_t num_data_bytes_consumed,
                              absl::Span<IpczHandle> handles) {
+  TrapEventDispatcher dispatcher;
   absl::MutexLock lock(&mutex_);
   if (!in_two_phase_get_) {
     return IPCZ_RESULT_FAILED_PRECONDITION;
   }
 
-  IpczResult result =
-      router_->CommitGetNextIncomingParcel(num_data_bytes_consumed, handles);
+  IpczResult result = router_->CommitGetNextIncomingParcel(
+      num_data_bytes_consumed, handles, dispatcher);
   if (result == IPCZ_RESULT_OK) {
     in_two_phase_get_ = false;
   }

src/ipcz/router.cc

diff --git a/src/ipcz/router.cc b/src/ipcz/router.cc
index 63b0bb6..ea05b2a 100644
--- a/src/ipcz/router.cc
+++ b/src/ipcz/router.cc
@@ -494,10 +494,11 @@
   return IPCZ_RESULT_OK;
 }
 
-IpczResult Router::CommitGetNextIncomingParcel(size_t num_data_bytes_consumed,
-                                               absl::Span<IpczHandle> handles) {
+IpczResult Router::CommitGetNextIncomingParcel(
+    size_t num_data_bytes_consumed,
+    absl::Span<IpczHandle> handles,
+    TrapEventDispatcher& dispatcher) {
   const OperationContext context{OperationContext::kAPICall};
-  TrapEventDispatcher dispatcher;
   {
     absl::MutexLock lock(&mutex_);
     if (inward_edge_) {

src/ipcz/router.h

diff --git a/src/ipcz/router.h b/src/ipcz/router.h
index 9da6073..78c9f25 100644
--- a/src/ipcz/router.h
+++ b/src/ipcz/router.h
@@ -27,6 +27,7 @@
 class NodeLink;
 class RemoteRouterLink;
 struct RouterLinkState;
+class TrapEventDispatcher;
 
 // The Router is the main primitive responsible for routing parcels between ipcz
 // portals. This class is thread-safe.
@@ -164,7 +165,8 @@
   // consuming some (possibly all) bytes and handles from that parcel. Once a
   // parcel is fully consumed, it's removed from the inbound queue.
   IpczResult CommitGetNextIncomingParcel(size_t num_data_bytes_consumed,
-                                         absl::Span<IpczHandle> handles);
+                                         absl::Span<IpczHandle> handles,
+                                         TrapEventDispatcher& dispatcher);
 
   // Attempts to install a new trap on this Router, to invoke `handler` as soon
   // as one or more conditions in `conditions` is met. This method effectively