Revert "MojoIpcz: Tolerate handle duplication failure"
This reverts commit 94878b58bc86871783e5008de04490ccab6b0ede.
Reason for revert: New test consistently fails: https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20Tests%20(dbg)(1)/108516/overview
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: Ic3d44119443e04fe4f029d43d73493734ffde063
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3955977
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Darren Shen <shend@chromium.org>
Owners-Override: Darren Shen <shend@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1059769}
NOKEYCHECK=True
GitOrigin-RevId: fc4843ed9fd4b23bfe5c356aa89fdc59cf53fa14
diff --git a/src/ipcz/driver_object.cc b/src/ipcz/driver_object.cc
index 8dbec71..967b3b4 100644
--- a/src/ipcz/driver_object.cc
+++ b/src/ipcz/driver_object.cc
@@ -86,7 +86,7 @@
return dimensions;
}
-bool DriverObject::Serialize(const DriverTransport& transport,
+void DriverObject::Serialize(const DriverTransport& transport,
absl::Span<uint8_t> data,
absl::Span<IpczDriverHandle> handles) {
size_t num_bytes = data.size();
@@ -94,11 +94,8 @@
IpczResult result = driver_->Serialize(
handle_, transport.driver_object().handle(), IPCZ_NO_FLAGS, nullptr,
data.data(), &num_bytes, handles.data(), &num_handles);
- if (result == IPCZ_RESULT_OK) {
- release();
- return true;
- }
- return false;
+ ABSL_ASSERT(result == IPCZ_RESULT_OK);
+ release();
}
// static
diff --git a/src/ipcz/driver_object.h b/src/ipcz/driver_object.h
index eed6a9b..44b0c68 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`.
- bool Serialize(const DriverTransport& transport,
+ void 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 a8cb7a1..0103284 100644
--- a/src/ipcz/driver_transport.cc
+++ b/src/ipcz/driver_transport.cc
@@ -102,12 +102,7 @@
IpczResult DriverTransport::Transmit(Message& message) {
ABSL_ASSERT(message.CanTransmitOn(*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;
- }
+ message.Serialize(*this);
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 79bb836..e64dab3 100644
--- a/src/ipcz/driver_transport_test.cc
+++ b/src/ipcz/driver_transport_test.cc
@@ -166,41 +166,5 @@
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.
- *num_bytes = 1;
- *num_handles = 1;
- return IPCZ_RESULT_RESOURCE_EXHAUSTED;
- }
-
- // Return 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 b736842..fea6a45 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.
-bool SerializeDriverObject(
+IpczResult 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 false;
+ return IPCZ_RESULT_INVALID_ARGUMENT;
}
uint32_t driver_data_array = 0;
@@ -60,13 +60,10 @@
dimensions.num_driver_handles);
auto handles_view = absl::MakeSpan(transmissible_handles);
- if (!object.Serialize(
- transport, driver_data,
- handles_view.subspan(first_handle, dimensions.num_driver_handles))) {
- return false;
- }
-
- return true;
+ object.Serialize(
+ transport, driver_data,
+ handles_view.subspan(first_handle, dimensions.num_driver_handles));
+ return IPCZ_RESULT_OK;
}
// Returns `true` if and only if it will be safe to use GetArrayView() to access
@@ -211,10 +208,10 @@
return true;
}
-bool Message::Serialize(const DriverTransport& transport) {
+void Message::Serialize(const DriverTransport& transport) {
ABSL_ASSERT(CanTransmitOn(transport));
if (driver_objects_.empty()) {
- return true;
+ return;
}
const uint32_t array_offset =
@@ -225,19 +222,16 @@
// 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 = {};
- ok &= SerializeDriverObject(std::move(driver_objects()[i]), transport,
- *this, data, transmissible_handles);
+ const IpczResult result =
+ SerializeDriverObject(std::move(driver_objects()[i]), transport, *this,
+ data, transmissible_handles);
+ ABSL_ASSERT(result == IPCZ_RESULT_OK);
GetArrayView<internal::DriverObjectData>(array_offset)[i] = data;
}
- if (ok) {
- transmissible_driver_handles_ = std::move(transmissible_handles);
- return true;
- }
- return false;
+ transmissible_driver_handles_ = std::move(transmissible_handles);
}
bool Message::DeserializeUnknownType(const DriverTransport::RawMessage& message,
diff --git a/src/ipcz/message.h b/src/ipcz/message.h
index 7187528..879afcd 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.
- bool Serialize(const DriverTransport& transport);
+ void 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
