MojoIpcz: Disable shared memory parcel data
Introduces a new option for ipcz CreateNode() to disable use of shared
memory fragment allocation on behalf of outgoing parcel data buffers.
Shared memory parcel allocations will be disabled by default by MojoIpcz
for its initial launch.
Bug: 1380476
Change-Id: I775318db15935cdeedc058a2e5b412d87ee07e1d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3997676
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Alex Gough <ajgo@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1066319}
NOKEYCHECK=True
GitOrigin-RevId: f3b79963f9cb1aa5f61a6e65ef3a9c45a6f5ff58
diff --git a/include/ipcz/ipcz.h b/include/ipcz/ipcz.h
index cffbc11..3e5c070 100644
--- a/include/ipcz/ipcz.h
+++ b/include/ipcz/ipcz.h
@@ -501,6 +501,17 @@
} // extern "C"
#endif
+// Options given to CreateNode() to configure the new node's behavior.
+struct IPCZ_ALIGN(8) IpczCreateNodeOptions {
+ // The exact size of this structure in bytes. Must be set accurately before
+ // passing the structure to CreateNode().
+ size_t size;
+
+ // If set to true, this node will not attempt to allocate parcel data storage
+ // within shared memory.
+ bool disable_shared_memory_parcel_data;
+};
+
// See CreateNode() and the IPCZ_CREATE_NODE_* flag descriptions below.
typedef uint32_t IpczCreateNodeFlags;
@@ -900,11 +911,11 @@
// from operating correctly. For example, the is returned if ipcz was
// built against a std::atomic implementation which does not provide
// lock-free 32-bit and 64-bit atomics.
- IpczResult(IPCZ_API* CreateNode)(const struct IpczDriver* driver, // in
- IpczDriverHandle driver_node, // in
- IpczCreateNodeFlags flags, // in
- const void* options, // in
- IpczHandle* node); // out
+ IpczResult(IPCZ_API* CreateNode)(const struct IpczDriver* driver, // in
+ IpczDriverHandle driver_node, // in
+ IpczCreateNodeFlags flags, // in
+ const IpczCreateNodeOptions* options, // in
+ IpczHandle* node); // out
// Connects `node` to another node in the system using an application-provided
// driver transport handle in `driver_transport` for communication. If this
diff --git a/src/api.cc b/src/api.cc
index 74cf5b1..061d849 100644
--- a/src/api.cc
+++ b/src/api.cc
@@ -31,12 +31,16 @@
IpczResult CreateNode(const IpczDriver* driver,
IpczDriverHandle driver_node,
IpczCreateNodeFlags flags,
- const void* options,
+ const IpczCreateNodeOptions* options,
IpczHandle* node) {
if (!node || !driver || driver->size < sizeof(IpczDriver)) {
return IPCZ_RESULT_INVALID_ARGUMENT;
}
+ if (options && options->size < sizeof(IpczCreateNodeOptions)) {
+ return IPCZ_RESULT_INVALID_ARGUMENT;
+ }
+
if (!driver->Close || !driver->Serialize || !driver->Deserialize ||
!driver->CreateTransports || !driver->ActivateTransport ||
!driver->DeactivateTransport || !driver->Transmit ||
@@ -61,7 +65,7 @@
auto node_ptr = ipcz::MakeRefCounted<ipcz::Node>(
(flags & IPCZ_CREATE_NODE_AS_BROKER) != 0 ? ipcz::Node::Type::kBroker
: ipcz::Node::Type::kNormal,
- *driver, driver_node);
+ *driver, driver_node, options);
*node = ipcz::Node::ReleaseAsHandle(std::move(node_ptr));
return IPCZ_RESULT_OK;
}
diff --git a/src/ipcz/node.cc b/src/ipcz/node.cc
index f2fc29e..9d266da 100644
--- a/src/ipcz/node.cc
+++ b/src/ipcz/node.cc
@@ -24,6 +24,25 @@
namespace ipcz {
+namespace {
+
+// Returns a copy of the structure pointed to by `options` if non-null;
+// otherwise returns a default set of options. This function will also adapt
+// to newer or older versions of the input options on input, coercing them into
+// the current implementation's version if needed.
+IpczCreateNodeOptions CopyOrUseDefaultOptions(
+ const IpczCreateNodeOptions* options) {
+ IpczCreateNodeOptions copied_options = {0};
+ if (options) {
+ memcpy(&copied_options, options,
+ std::min(options->size, sizeof(copied_options)));
+ }
+ copied_options.size = sizeof(copied_options);
+ return copied_options;
+}
+
+} // namespace
+
// A pending introduction tracks progress of one or more outstanding
// introduction requests for a single node in the system.
class Node::PendingIntroduction {
@@ -70,8 +89,14 @@
std::vector<EstablishLinkCallback> callbacks_;
};
-Node::Node(Type type, const IpczDriver& driver, IpczDriverHandle driver_node)
- : type_(type), driver_(driver), driver_node_(driver_node) {
+Node::Node(Type type,
+ const IpczDriver& driver,
+ IpczDriverHandle driver_node,
+ const IpczCreateNodeOptions* options)
+ : type_(type),
+ driver_(driver),
+ driver_node_(driver_node),
+ options_(CopyOrUseDefaultOptions(options)) {
if (type_ == Type::kBroker) {
// Only brokers assign their own names.
assigned_name_ = GenerateRandomName();
diff --git a/src/ipcz/node.h b/src/ipcz/node.h
index ee9a223..c343789 100644
--- a/src/ipcz/node.h
+++ b/src/ipcz/node.h
@@ -54,11 +54,15 @@
// Note that `driver` must outlive the Node. `driver_node` is an arbitrary
// driver-specific handle that may be used for additional context when
// interfacing with the driver regarding this node.
- Node(Type type, const IpczDriver& driver, IpczDriverHandle driver_node);
+ Node(Type type,
+ const IpczDriver& driver,
+ IpczDriverHandle driver_node,
+ const IpczCreateNodeOptions* options = nullptr);
Type type() const { return type_; }
const IpczDriver& driver() const { return driver_; }
IpczDriverHandle driver_node() const { return driver_node_; }
+ const IpczCreateNodeOptions& options() const { return options_; }
// APIObject:
IpczResult Close() override;
@@ -193,6 +197,7 @@
const Type type_;
const IpczDriver& driver_;
const IpczDriverHandle driver_node_;
+ const IpczCreateNodeOptions options_;
absl::Mutex mutex_;
diff --git a/src/ipcz/node_link_memory.cc b/src/ipcz/node_link_memory.cc
index 0cfadfa..2e67c91 100644
--- a/src/ipcz/node_link_memory.cc
+++ b/src/ipcz/node_link_memory.cc
@@ -168,6 +168,8 @@
NodeLinkMemory::NodeLinkMemory(Ref<Node> node,
DriverMemoryMapping primary_buffer_memory)
: node_(std::move(node)),
+ allow_parcel_data_allocation_(
+ !node_->options().disable_shared_memory_parcel_data),
primary_buffer_memory_(primary_buffer_memory.bytes()),
primary_buffer_(
*reinterpret_cast<PrimaryBuffer*>(primary_buffer_memory_.data())) {
diff --git a/src/ipcz/node_link_memory.h b/src/ipcz/node_link_memory.h
index 1eb43cb..cf7b8ac 100644
--- a/src/ipcz/node_link_memory.h
+++ b/src/ipcz/node_link_memory.h
@@ -60,6 +60,12 @@
static Ref<NodeLinkMemory> Create(Ref<Node> node,
DriverMemoryMapping primary_buffer_memory);
+ // Returns true if and only if new parcels are allowed to try allocating their
+ // data buffer through this NodeLinkMemory.
+ bool allow_parcel_data_allocation() const {
+ return allow_parcel_data_allocation_;
+ }
+
// Returns a new BufferId which should still be unused by any buffer in this
// NodeLinkMemory's BufferPool, or that of its peer NodeLinkMemory. When
// allocating new a buffer to add to the BufferPool, its BufferId should be
@@ -162,6 +168,7 @@
const Fragment& fragment);
const Ref<Node> node_;
+ const bool allow_parcel_data_allocation_;
// The underlying BufferPool. Note that this object is itself thread-safe, so
// access to it is not synchronized by NodeLinkMemory.
diff --git a/src/ipcz/node_link_memory_test.cc b/src/ipcz/node_link_memory_test.cc
index 44ba853..2a1cd3b 100644
--- a/src/ipcz/node_link_memory_test.cc
+++ b/src/ipcz/node_link_memory_test.cc
@@ -15,6 +15,7 @@
#include "ipcz/node_link.h"
#include "ipcz/node_link_memory.h"
#include "ipcz/node_name.h"
+#include "ipcz/parcel.h"
#include "reference_drivers/sync_reference_driver.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "util/ref_counted.h"
@@ -26,29 +27,45 @@
constexpr NodeName kTestBrokerName(1, 2);
constexpr NodeName kTestNonBrokerName(2, 3);
+constexpr NodeName kOtherTestNonBrokerName(3, 5);
class NodeLinkMemoryTest : public testing::Test {
public:
+ const Ref<Node>& node_a() const { return node_a_; }
+
NodeLinkMemory& memory_a() { return link_a_->memory(); }
NodeLinkMemory& memory_b() { return link_b_->memory(); }
- void SetUp() override {
+ // Connects a broker to a non-broker and returns their respective NodeLinks.
+ static std::pair<Ref<NodeLink>, Ref<NodeLink>> ConnectNodes(
+ Ref<Node> broker,
+ Ref<Node> non_broker) {
+ std::pair<Ref<NodeLink>, Ref<NodeLink>> links;
auto transports = DriverTransport::CreatePair(kTestDriver);
DriverMemoryWithMapping buffer =
NodeLinkMemory::AllocateMemory(kTestDriver);
- link_a_ = NodeLink::CreateInactive(
- node_a_, LinkSide::kA, kTestBrokerName, kTestNonBrokerName,
- Node::Type::kNormal, 0, transports.first,
- NodeLinkMemory::Create(node_a_, std::move(buffer.mapping)));
- link_b_ = NodeLink::CreateInactive(
- node_b_, LinkSide::kB, kTestNonBrokerName, kTestBrokerName,
- Node::Type::kBroker, 0, transports.second,
- NodeLinkMemory::Create(node_b_, buffer.memory.Map()));
- node_a_->AddConnection(kTestNonBrokerName, {.link = link_a_});
- node_b_->AddConnection(kTestBrokerName,
- {.link = link_b_, .broker = link_a_});
- link_a_->Activate();
- link_b_->Activate();
+ links.first = NodeLink::CreateInactive(
+ broker, LinkSide::kA, broker->GetAssignedName(),
+ non_broker->GetAssignedName(), Node::Type::kNormal, 0, transports.first,
+ NodeLinkMemory::Create(broker, std::move(buffer.mapping)));
+ links.second = NodeLink::CreateInactive(
+ non_broker, LinkSide::kB, non_broker->GetAssignedName(),
+ broker->GetAssignedName(), Node::Type::kBroker, 0, transports.second,
+ NodeLinkMemory::Create(non_broker, buffer.memory.Map()));
+ broker->AddConnection(non_broker->GetAssignedName(), {.link = links.first});
+ non_broker->AddConnection(broker->GetAssignedName(),
+ {.link = links.second, .broker = links.first});
+ links.first->Activate();
+ links.second->Activate();
+ return links;
+ }
+
+ void SetUp() override {
+ node_a_->SetAssignedName(kTestBrokerName);
+ node_b_->SetAssignedName(kTestNonBrokerName);
+ auto links = ConnectNodes(node_a_, node_b_);
+ link_a_ = std::move(links.first);
+ link_b_ = std::move(links.second);
}
void TearDown() override {
@@ -225,5 +242,38 @@
EXPECT_TRUE(memory_b().FreeFragment(fragment));
}
+TEST_F(NodeLinkMemoryTest, ParcelDataAllocation) {
+ // NodeLinkMemory can in general be used by Parcel instances to allocate data
+ // buffers; but this can also be disabled when configuring a new node.
+
+ const IpczCreateNodeOptions options = {
+ .size = sizeof(options),
+ .disable_shared_memory_parcel_data = true,
+ };
+ const Ref<Node> node_c{MakeRefCounted<Node>(
+ Node::Type::kNormal, kTestDriver, IPCZ_INVALID_DRIVER_HANDLE, &options)};
+ node_c->SetAssignedName(kOtherTestNonBrokerName);
+ auto links = ConnectNodes(node_a(), node_c);
+
+ // We use a small enough size that this is guaranteed to allocate within
+ // NodeLinkMemory unless parcel allocation is disabled.
+ constexpr size_t kParcelSize = 32;
+ Parcel parcel_from_a_to_c;
+ parcel_from_a_to_c.AllocateData(kParcelSize, /*allow_partial=*/false,
+ &links.first->memory());
+ EXPECT_FALSE(parcel_from_a_to_c.data_fragment().is_null());
+ EXPECT_GE(parcel_from_a_to_c.data_fragment().size(), kParcelSize);
+
+ // But when allocated from the `node_c` side, we should never get a memory
+ // fragment for parcel data.
+ Parcel parcel_from_c_to_a;
+ parcel_from_c_to_a.AllocateData(kParcelSize, /*allow_partial=*/false,
+ &links.second->memory());
+ EXPECT_TRUE(parcel_from_c_to_a.data_fragment().is_null());
+ EXPECT_EQ(parcel_from_c_to_a.data_size(), kParcelSize);
+
+ node_c->Close();
+}
+
} // namespace
} // namespace ipcz
diff --git a/src/ipcz/parcel.cc b/src/ipcz/parcel.cc
index fa8ed8e..dab0fd8 100644
--- a/src/ipcz/parcel.cc
+++ b/src/ipcz/parcel.cc
@@ -75,7 +75,7 @@
ABSL_ASSERT(data_view_.empty());
Fragment fragment;
- if (memory && num_bytes > 0) {
+ if (num_bytes > 0 && memory && memory->allow_parcel_data_allocation()) {
const size_t requested_fragment_size = num_bytes + sizeof(FragmentHeader);
if (allow_partial) {
fragment = memory->AllocateFragmentBestEffort(requested_fragment_size);
