[M110] MojoIpcz: Fix nullptr deref in DataPipe

In some scenarios a DataPipe may be re-entered during its own closure
by a trap event handler on its control portal. This is currently
unsafe as it can result in nullptr dereferences on the DataPipe's
`portal_` field.

This change ensures there are no more unchecked dereferences of
`portal_`.

Fixed: 1399872

(cherry picked from commit 304e98b)

Change-Id: Ie1cf45de7a0f607d0e90b9eb52970604a3dcd922
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4139413
Reviewed-by: Alex Gough <ajgo@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1089903}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4148307
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Ken Rockot <rockot@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/5481@{#183}
Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}

mojo/core/core_ipcz.cc

@@ -128,7 +128,9 @@ MojoResult MojoQueryHandleSignalsStateIpcz(
 
   auto* data_pipe = ipcz_driver::DataPipe::FromBox(handle);
   if (data_pipe) {
-    *signals_state = data_pipe->GetSignals();
+    if (!data_pipe->GetSignals(*signals_state)) {
+      return MOJO_RESULT_INVALID_ARGUMENT;
+    }
     return MOJO_RESULT_OK;
   }
 

mojo/core/ipcz_driver/data_pipe.cc

@@ -550,17 +550,21 @@ scoped_refptr<DataPipe> DataPipe::Deserialize(
   return endpoint;
 }
 
-MojoHandleSignalsState DataPipe::GetSignals() {
-  MojoHandleSignalsState signals_state = {};
+bool DataPipe::GetSignals(MojoHandleSignalsState& signals_state) {
+  signals_state = {};
   MojoHandleSignals& satisfied = signals_state.satisfied_signals;
   MojoHandleSignals& satisfiable = signals_state.satisfiable_signals;
 
   base::AutoLock lock(lock_);
+  if (!portal_) {
+    return false;
+  }
+
   IpczPortalStatus status = {.size = sizeof(status)};
   const IpczResult result = GetIpczAPI().QueryPortalStatus(
       portal_->handle(), IPCZ_NO_FLAGS, nullptr, &status);
   if (result != IPCZ_RESULT_OK) {
-    return signals_state;
+    return false;
   }
 
   if ((status.flags & IPCZ_PORTAL_STATUS_DEAD) != 0) {
@@ -594,7 +598,7 @@ MojoHandleSignalsState DataPipe::GetSignals() {
           MOJO_HANDLE_SIGNAL_READABLE | MOJO_HANDLE_SIGNAL_NEW_DATA_READABLE;
     }
 
-    return signals_state;
+    return true;
   }
 
   DCHECK(is_producer());
@@ -605,7 +609,7 @@ MojoHandleSignalsState DataPipe::GetSignals() {
     }
   }
 
-  return signals_state;
+  return true;
 }
 
 void DataPipe::FlushUpdatesFromPeer() {

mojo/core/ipcz_driver/data_pipe.h

@@ -150,8 +150,10 @@ class DataPipe : public Object<DataPipe> {
       base::span<PlatformHandle> handles);
 
   // Returns Mojo signals to reflect the effective state of this DataPipe and
-  // its control portal.
-  MojoHandleSignalsState GetSignals();
+  // its control portal within `signals_state`. Returns true on success or false
+  // if the DataPipe's signal state is unspecified due to impending closure. In
+  // the latter case `signals_state` is zeroed out.
+  bool GetSignals(MojoHandleSignalsState& signals_state);
 
  private:
   ~DataPipe() override;

mojo/core/ipcz_driver/mojo_trap.cc

@@ -86,7 +86,10 @@ bool GetEventResultForSignalsState(const MojoHandleSignalsState& state,
 bool PopulateEventForDataPipe(DataPipe& pipe,
                               MojoHandleSignals trigger_signals,
                               MojoTrapEvent& event) {
-  event.signals_state = pipe.GetSignals();
+  if (!pipe.GetSignals(event.signals_state)) {
+    return false;
+  }
+
   return GetEventResultForSignalsState(event.signals_state, trigger_signals,
                                        event.result);
 }
@@ -419,7 +422,13 @@ void MojoTrap::HandleEvent(const IpczTrapEvent& event) {
       .trigger_context = trigger->trigger_context,
   };
   if (trigger->data_pipe) {
-    PopulateEventForDataPipe(*trigger->data_pipe, trigger->signals, mojo_event);
+    if (!PopulateEventForDataPipe(*trigger->data_pipe, trigger->signals,
+                                  mojo_event)) {
+      // This event may be spurious if the DataPipe itself is closing but its
+      // its control portal is not yet closed. In that case it's safe to drop
+      // without firing.
+      return;
+    }
   } else {
     PopulateEventForMessagePipe(trigger->signals, *event.status, mojo_event);
   }