Reland "MojoIpcz: Tolerate handle duplication failure"
This is a reland of commit 94878b58bc86871783e5008de04490ccab6b0ede
Original change's description:
> MojoIpcz: Tolerate handle duplication failure
>
> Windows handle duplication may fail during message transmission.
> Assuming the application is well behaved and handles used are otherwise
> valid, this can occur whenever either of the processes involved are
> terminating or fully terminated.
>
> The MojoIpcz driver was DCHECKing that this doesn't fail, which is
> wrong. This removes the DCHECK and adds graceful handling of any
> resulting object serialization failure.
>
> Bug: 1374568
> Change-Id: I33107a7f5cff16dba4dc5a7e83a605a00fe0d8ac
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3953791
> Reviewed-by: Alex Gough <ajgo@chromium.org>
> Commit-Queue: Ken Rockot <rockot@google.com>
> Cr-Commit-Position: refs/heads/main@{#1059557}
Bug: 1374568
Change-Id: I5b9aee1b1e760b5c30ca7f6bbf193cdbedba923f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3956510
Auto-Submit: Ken Rockot <rockot@google.com>
Commit-Queue: Alex Gough <ajgo@chromium.org>
Reviewed-by: Alex Gough <ajgo@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1059814}
NOKEYCHECK=True
GitOrigin-RevId: 84105d720627b59ec41fe1084bbc213ca749da94
diff --git a/src/ipcz/driver_object.cc b/src/ipcz/driver_object.cc
index 967b3b4..8dbec71 100644
--- a/src/ipcz/driver_object.cc
+++ b/src/ipcz/driver_object.cc
@@ -86,7 +86,7 @@
return dimensions;
}
-void DriverObject::Serialize(const DriverTransport& transport,
+bool DriverObject::Serialize(const DriverTransport& transport,
absl::Span<uint8_t> data,
absl::Span<IpczDriverHandle> handles) {
size_t num_bytes = data.size();
@@ -94,8 +94,11 @@
IpczResult result = driver_->Serialize(
handle_, transport.driver_object().handle(), IPCZ_NO_FLAGS, nullptr,
data.data(), &num_bytes, handles.data(), &num_handles);
- ABSL_ASSERT(result == IPCZ_RESULT_OK);
- release();
+ if (result == IPCZ_RESULT_OK) {
+ release();
+ return true;
+ }
+ return false;
}
// static
diff --git a/src/ipcz/driver_object.h b/src/ipcz/driver_object.h
index 44b0c68..eed6a9b 100644
--- a/src/ipcz/driver_object.h
+++ b/src/ipcz/driver_object.h
@@ -58,7 +58,7 @@
// transmissible by the driver without further serialization. Must only be
// called on valid objects which are known to be serializable and
// transmissible over `transport`.
- void Serialize(const DriverTransport& transport,
+ bool Serialize(const DriverTransport& transport,
absl::Span<uint8_t> data,
absl::Span<IpczDriverHandle> handles);
diff --git a/src/ipcz/driver_transport.cc b/src/ipcz/driver_transport.cc
index 0103284..a8cb7a1 100644
--- a/src/ipcz/driver_transport.cc
+++ b/src/ipcz/driver_transport.cc
@@ -102,7 +102,12 @@
IpczResult DriverTransport::Transmit(Message& message) {
ABSL_ASSERT(message.CanTransmitOn(*this));
- message.Serialize(*this);
+ if (!message.Serialize(*this)) {
+ // If serialization fails despite the object appearing to be serializable,
+ // we have to assume the transport is in a dysfunctional state and will be
+ // torn down by the driver soon. Discard the transmission.
+ return IPCZ_RESULT_FAILED_PRECONDITION;
+ }
const absl::Span<const uint8_t> data = message.data_view();
const absl::Span<const IpczDriverHandle> handles =
diff --git a/src/ipcz/driver_transport_test.cc b/src/ipcz/driver_transport_test.cc
index e64dab3..7ff58ad 100644
--- a/src/ipcz/driver_transport_test.cc
+++ b/src/ipcz/driver_transport_test.cc
@@ -166,5 +166,43 @@
EXPECT_CALL(driver(), Close(kTransport0, _, _));
}
+TEST_F(DriverTransportTest, SerializationFailure) {
+ // Verifies that if a serializable object fails to serialize, it's properly
+ // destroyed by the transport and no transmission occurs.
+ constexpr IpczDriverHandle kTransport0 = 5;
+ constexpr IpczDriverHandle kTransport1 = 42;
+ auto [a, b] = CreateTransportPair(kTransport0, kTransport1);
+
+ constexpr IpczDriverHandle kFakeObject = 12345678;
+ test::msg::MessageWithDriverObject message;
+ message.params().object =
+ message.AppendDriverObject(DriverObject(test::kMockDriver, kFakeObject));
+
+ EXPECT_CALL(driver(), Serialize(kFakeObject, kTransport0, _, _, _, _, _, _))
+ .WillRepeatedly([&](IpczDriverHandle, IpczDriverHandle, uint32_t,
+ const void*, void* data, size_t* num_bytes,
+ IpczDriverHandle* handles, size_t* num_handles) {
+ if (!data && !handles) {
+ // Return valid outputs when ipcz is sizing the object.
+ if (num_bytes && num_handles) {
+ *num_bytes = 1;
+ *num_handles = 1;
+ }
+ return IPCZ_RESULT_RESOURCE_EXHAUSTED;
+ }
+
+ // Return an error when serializing.
+ return IPCZ_RESULT_FAILED_PRECONDITION;
+ });
+
+ // The object handle should be closed upon transmission failure.
+ EXPECT_CALL(driver(), Close(kFakeObject, _, _)).Times(1);
+
+ a->Transmit(message);
+
+ EXPECT_CALL(driver(), Close(kTransport1, _, _));
+ EXPECT_CALL(driver(), Close(kTransport0, _, _));
+}
+
} // namespace
} // namespace ipcz
diff --git a/src/ipcz/message.cc b/src/ipcz/message.cc
index fea6a45..b736842 100644
--- a/src/ipcz/message.cc
+++ b/src/ipcz/message.cc
@@ -29,7 +29,7 @@
// transmissible handles emitted by the driver are appended to
// `transmissible_handles`, with relevant index and count also stashed in the
// DriverObjectData.
-IpczResult SerializeDriverObject(
+bool SerializeDriverObject(
DriverObject object,
const DriverTransport& transport,
Message& message,
@@ -38,7 +38,7 @@
if (!object.is_valid()) {
// This is not a valid driver handle and it cannot be serialized.
data.num_driver_handles = 0;
- return IPCZ_RESULT_INVALID_ARGUMENT;
+ return false;
}
uint32_t driver_data_array = 0;
@@ -60,10 +60,13 @@
dimensions.num_driver_handles);
auto handles_view = absl::MakeSpan(transmissible_handles);
- object.Serialize(
- transport, driver_data,
- handles_view.subspan(first_handle, dimensions.num_driver_handles));
- return IPCZ_RESULT_OK;
+ if (!object.Serialize(
+ transport, driver_data,
+ handles_view.subspan(first_handle, dimensions.num_driver_handles))) {
+ return false;
+ }
+
+ return true;
}
// Returns `true` if and only if it will be safe to use GetArrayView() to access
@@ -208,10 +211,10 @@
return true;
}
-void Message::Serialize(const DriverTransport& transport) {
+bool Message::Serialize(const DriverTransport& transport) {
ABSL_ASSERT(CanTransmitOn(transport));
if (driver_objects_.empty()) {
- return;
+ return true;
}
const uint32_t array_offset =
@@ -222,16 +225,19 @@
// handles attached. Since these objects are small, we inline some storage on
// the stack to avoid some heap allocation in the most common cases.
absl::InlinedVector<IpczDriverHandle, 2> transmissible_handles;
+ bool ok = true;
for (size_t i = 0; i < driver_objects().size(); ++i) {
internal::DriverObjectData data = {};
- const IpczResult result =
- SerializeDriverObject(std::move(driver_objects()[i]), transport, *this,
- data, transmissible_handles);
- ABSL_ASSERT(result == IPCZ_RESULT_OK);
+ ok &= SerializeDriverObject(std::move(driver_objects()[i]), transport,
+ *this, data, transmissible_handles);
GetArrayView<internal::DriverObjectData>(array_offset)[i] = data;
}
- transmissible_driver_handles_ = std::move(transmissible_handles);
+ if (ok) {
+ transmissible_driver_handles_ = std::move(transmissible_handles);
+ return true;
+ }
+ return false;
}
bool Message::DeserializeUnknownType(const DriverTransport::RawMessage& message,
diff --git a/src/ipcz/message.h b/src/ipcz/message.h
index 879afcd..7187528 100644
--- a/src/ipcz/message.h
+++ b/src/ipcz/message.h
@@ -322,7 +322,7 @@
// NOTE: It is invalid to call this on a message for which
// `CanTransmitOn(transport)` does not return true, and doing so results in
// unspecified behavior.
- void Serialize(const DriverTransport& transport);
+ bool Serialize(const DriverTransport& transport);
// Validates and deserializes a Message of an unrecognized type. DriverObjects
// are deserialized and much of the message structure is validated, but the
