ipcz: Better shm allocation failure handling
Various shared memory allocation operations may fail during node
interconnect, and we kind of pretend like this doesn't happen, which
can lead to crashes.
Fix it by actually checking the success of allocation + mapping, and
fail gracefully as needed.
Fixed: 1442422
Change-Id: I09d57ecd21c36768aabcaed8966148e387093590
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4521881
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Alex Gough <ajgo@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1142721}
NOKEYCHECK=True
GitOrigin-RevId: 4137cb2a579cd27a45863d8563bcc05de02f8aca
diff --git a/src/ipcz/node_connector.cc b/src/ipcz/node_connector.cc
index f30d239..012dafd 100644
--- a/src/ipcz/node_connector.cc
+++ b/src/ipcz/node_connector.cc
@@ -31,6 +31,7 @@
public:
NodeConnectorForBrokerToNonBroker(Ref<Node> node,
Ref<DriverTransport> transport,
+ DriverMemoryWithMapping memory,
IpczConnectNodeFlags flags,
std::vector<Ref<Portal>> waiting_portals,
ConnectCallback callback)
@@ -39,9 +40,8 @@
flags,
std::move(waiting_portals),
std::move(callback)),
- link_memory_allocation_(
- NodeLinkMemory::AllocateMemory(node_->driver())) {
- ABSL_ASSERT(link_memory_allocation_.mapping.is_valid());
+ link_memory_allocation_(std::move(memory)) {
+ ABSL_HARDENING_ASSERT(link_memory_allocation_.mapping.is_valid());
}
~NodeConnectorForBrokerToNonBroker() override = default;
@@ -291,7 +291,9 @@
uint64_t referral_id,
uint32_t num_initial_portals,
Ref<NodeLink> referrer,
- Ref<DriverTransport> transport)
+ Ref<DriverTransport> transport,
+ DriverMemoryWithMapping link_memory,
+ DriverMemoryWithMapping client_link_memory)
: NodeConnector(std::move(node),
std::move(transport),
IPCZ_NO_FLAGS,
@@ -299,9 +301,11 @@
/*callback=*/nullptr),
referral_id_(referral_id),
num_initial_portals_(num_initial_portals),
- referrer_(std::move(referrer)) {
- ABSL_ASSERT(link_memory_.mapping.is_valid());
- ABSL_ASSERT(client_link_memory_.mapping.is_valid());
+ referrer_(std::move(referrer)),
+ link_memory_(std::move(link_memory)),
+ client_link_memory_(std::move(client_link_memory)) {
+ ABSL_HARDENING_ASSERT(link_memory_.mapping.is_valid());
+ ABSL_HARDENING_ASSERT(client_link_memory_.mapping.is_valid());
}
~NodeConnectorForBrokerReferral() override = default;
@@ -392,16 +396,15 @@
const Ref<NodeLink> referrer_;
const NodeName broker_name_{node_->GetAssignedName()};
const NodeName referred_node_name_{node_->GenerateRandomName()};
- DriverMemoryWithMapping link_memory_{
- NodeLinkMemory::AllocateMemory(node_->driver())};
- DriverMemoryWithMapping client_link_memory_{
- NodeLinkMemory::AllocateMemory(node_->driver())};
+ DriverMemoryWithMapping link_memory_;
+ DriverMemoryWithMapping client_link_memory_;
};
class NodeConnectorForBrokerToBroker : public NodeConnector {
public:
NodeConnectorForBrokerToBroker(Ref<Node> node,
Ref<DriverTransport> transport,
+ DriverMemoryWithMapping memory,
IpczConnectNodeFlags flags,
std::vector<Ref<Portal>> waiting_portals,
ConnectCallback callback)
@@ -410,9 +413,8 @@
flags,
std::move(waiting_portals),
std::move(callback)),
- link_memory_allocation_(
- NodeLinkMemory::AllocateMemory(node_->driver())) {
- ABSL_ASSERT(link_memory_allocation_.mapping.is_valid());
+ link_memory_allocation_(std::move(memory)) {
+ ABSL_HARDENING_ASSERT(link_memory_allocation_.mapping.is_valid());
}
~NodeConnectorForBrokerToBroker() override = default;
@@ -480,16 +482,22 @@
const bool share_broker = (flags & IPCZ_CONNECT_NODE_SHARE_BROKER) != 0;
const bool inherit_broker = (flags & IPCZ_CONNECT_NODE_INHERIT_BROKER) != 0;
if (from_broker) {
+ DriverMemoryWithMapping memory =
+ NodeLinkMemory::AllocateMemory(node->driver());
+ if (!memory.mapping.is_valid()) {
+ return {nullptr, IPCZ_RESULT_RESOURCE_EXHAUSTED};
+ }
+
if (to_broker) {
return {MakeRefCounted<NodeConnectorForBrokerToBroker>(
- std::move(node), std::move(transport), flags, initial_portals,
- std::move(callback)),
+ std::move(node), std::move(transport), std::move(memory),
+ flags, initial_portals, std::move(callback)),
IPCZ_RESULT_OK};
}
return {MakeRefCounted<NodeConnectorForBrokerToNonBroker>(
- std::move(node), std::move(transport), flags, initial_portals,
- std::move(callback)),
+ std::move(node), std::move(transport), std::move(memory), flags,
+ initial_portals, std::move(callback)),
IPCZ_RESULT_OK};
}
@@ -565,11 +573,14 @@
uint64_t referral_id,
uint32_t num_initial_portals,
Ref<NodeLink> referrer,
- Ref<DriverTransport> transport_to_referred_node) {
+ Ref<DriverTransport> transport_to_referred_node,
+ DriverMemoryWithMapping link_memory,
+ DriverMemoryWithMapping client_link_memory) {
ABSL_ASSERT(node->type() == Node::Type::kBroker);
auto connector = MakeRefCounted<NodeConnectorForBrokerReferral>(
std::move(node), referral_id, num_initial_portals, std::move(referrer),
- std::move(transport_to_referred_node));
+ std::move(transport_to_referred_node), std::move(link_memory),
+ std::move(client_link_memory));
// The connector effectively owns itself and lives only until its transport is
// disconnected or it receives a greeting from the referred node.
diff --git a/src/ipcz/node_connector.h b/src/ipcz/node_connector.h
index 9d1d9e0..4e9beb9 100644
--- a/src/ipcz/node_connector.h
+++ b/src/ipcz/node_connector.h
@@ -51,13 +51,17 @@
// non-broker node referral from `referrer`, referring a new non-broker node
// on the remote end of `transport_to_referred_node`. This performs a
// handshake with the referred node before introducing it and the referrer to
- // each other.
+ // each other. `link_memory` and `client_link_memory` must be valid and will
+ // be passed respectively to the referred node (for its link to the broker)
+ // and the referring node (for its link to the referred node).
static bool HandleNonBrokerReferral(
Ref<Node> node,
uint64_t referral_id,
uint32_t num_initial_portals,
Ref<NodeLink> referrer,
- Ref<DriverTransport> transport_to_referred_node);
+ Ref<DriverTransport> transport_to_referred_node,
+ DriverMemoryWithMapping link_memory,
+ DriverMemoryWithMapping client_link_memory);
virtual bool Connect() = 0;
diff --git a/src/ipcz/node_link.cc b/src/ipcz/node_link.cc
index a1ede6f..4d451b8 100644
--- a/src/ipcz/node_link.cc
+++ b/src/ipcz/node_link.cc
@@ -374,10 +374,25 @@
return false;
}
+ DriverMemoryWithMapping link_memory =
+ NodeLinkMemory::AllocateMemory(node()->driver());
+ DriverMemoryWithMapping client_link_memory =
+ NodeLinkMemory::AllocateMemory(node()->driver());
+ if (!link_memory.mapping.is_valid() ||
+ !client_link_memory.mapping.is_valid()) {
+ // Not a validation failure, but we can't accept the referral because we
+ // can't allocate link memory for one side or the other.
+ msg::NonBrokerReferralRejected rejected;
+ rejected.params().referral_id = refer.params().referral_id;
+ Transmit(rejected);
+ return true;
+ }
+
return NodeConnector::HandleNonBrokerReferral(
node(), refer.params().referral_id, refer.params().num_initial_portals,
WrapRefCounted(this),
- MakeRefCounted<DriverTransport>(std::move(transport)));
+ MakeRefCounted<DriverTransport>(std::move(transport)),
+ std::move(link_memory), std::move(client_link_memory));
}
bool NodeLink::OnNonBrokerReferralAccepted(
