Merge "RPC Binder: increase transaction size" into main am: 412d19a083
Original change: https://android-review.googlesource.com/c/platform/frameworks/native/+/3466199
Change-Id: Id4aa9522b4513326d155278325ab0ded5a7a03a1
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>
diff --git a/libs/binder/Binder.cpp b/libs/binder/Binder.cpp
index 0a22588..bc7ae37 100644
--- a/libs/binder/Binder.cpp
+++ b/libs/binder/Binder.cpp
@@ -38,6 +38,7 @@
#endif
#include "BuildFlags.h"
+#include "Constants.h"
#include "OS.h"
#include "RpcState.h"
@@ -70,8 +71,6 @@
constexpr bool kEnableRecording = false;
#endif
-// Log any reply transactions for which the data exceeds this size
-#define LOG_REPLIES_OVER_SIZE (300 * 1024)
// ---------------------------------------------------------------------------
IBinder::IBinder()
@@ -412,7 +411,7 @@
// In case this is being transacted on in the same process.
if (reply != nullptr) {
reply->setDataPosition(0);
- if (reply->dataSize() > LOG_REPLIES_OVER_SIZE) {
+ if (reply->dataSize() > binder::kLogTransactionsOverBytes) {
ALOGW("Large reply transaction of %zu bytes, interface descriptor %s, code %d",
reply->dataSize(), String8(getInterfaceDescriptor()).c_str(), code);
}
diff --git a/libs/binder/BpBinder.cpp b/libs/binder/BpBinder.cpp
index 444f061..c13e0f9 100644
--- a/libs/binder/BpBinder.cpp
+++ b/libs/binder/BpBinder.cpp
@@ -28,6 +28,7 @@
#include <stdio.h>
#include "BuildFlags.h"
+#include "Constants.h"
#include "file.h"
//#undef ALOGV
@@ -63,9 +64,6 @@
static constexpr uint32_t kBinderProxyCountWarnInterval = 5000;
-// Log any transactions for which the data exceeds this size
-#define LOG_TRANSACTIONS_OVER_SIZE (300 * 1024)
-
enum {
LIMIT_REACHED_MASK = 0x80000000, // A flag denoting that the limit has been reached
WARNING_REACHED_MASK = 0x40000000, // A flag denoting that the warning has been reached
@@ -403,9 +401,11 @@
status = IPCThreadState::self()->transact(binderHandle(), code, data, reply, flags);
}
- if (data.dataSize() > LOG_TRANSACTIONS_OVER_SIZE) {
+
+ if (data.dataSize() > binder::kLogTransactionsOverBytes) {
RpcMutexUniqueLock _l(mLock);
- ALOGW("Large outgoing transaction of %zu bytes, interface descriptor %s, code %d",
+ ALOGW("Large outgoing transaction of %zu bytes, interface descriptor %s, code %d was "
+ "sent",
data.dataSize(), String8(mDescriptorCache).c_str(), code);
}
diff --git a/libs/binder/Constants.h b/libs/binder/Constants.h
new file mode 100644
index 0000000..b75493c
--- /dev/null
+++ b/libs/binder/Constants.h
@@ -0,0 +1,39 @@
+/*
+ * Copyright (C) 2025 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+namespace android::binder {
+
+/**
+ * See also BINDER_VM_SIZE. In kernel binder, the sum of all transactions must be allocated in this
+ * space. Large transactions are very error prone. In general, we should work to reduce this limit.
+ * The same limit is used in RPC binder for consistency.
+ */
+constexpr size_t kLogTransactionsOverBytes = 300 * 1024;
+
+/**
+ * See b/392575419 - this limit is chosen for a specific usecase, because RPC binder does not have
+ * support for shared memory in the Android Baklava timeframe. This was 100 KB during and before
+ * Android V.
+ *
+ * Keeping this low helps preserve overall system performance. Transactions of this size are far too
+ * expensive to make multiple copies over binder or sockets, and they should be avoided if at all
+ * possible and transition to shared memory.
+ */
+constexpr size_t kRpcTransactionLimitBytes = 600 * 1024;
+
+} // namespace android::binder
diff --git a/libs/binder/RpcState.cpp b/libs/binder/RpcState.cpp
index fe6e1a3..03d974d 100644
--- a/libs/binder/RpcState.cpp
+++ b/libs/binder/RpcState.cpp
@@ -23,6 +23,7 @@
#include <binder/IPCThreadState.h>
#include <binder/RpcServer.h>
+#include "Constants.h"
#include "Debug.h"
#include "RpcWireFormat.h"
#include "Utils.h"
@@ -337,6 +338,8 @@
}
RpcState::CommandData::CommandData(size_t size) : mSize(size) {
+ if (size == 0) return;
+
// The maximum size for regular binder is 1MB for all concurrent
// transactions. A very small proportion of transactions are even
// larger than a page, but we need to avoid allocating too much
@@ -348,11 +351,11 @@
// transaction (in some cases, additional fixed size amounts are added),
// though for rough consistency, we should avoid cases where this data type
// is used for multiple dynamic allocations for a single transaction.
- constexpr size_t kMaxTransactionAllocation = 100 * 1000;
- if (size == 0) return;
- if (size > kMaxTransactionAllocation) {
- ALOGW("Transaction requested too much data allocation %zu", size);
+ if (size > binder::kRpcTransactionLimitBytes) {
+ ALOGE("Transaction requested too much data allocation: %zu bytes, failing.", size);
return;
+ } else if (size > binder::kLogTransactionsOverBytes) {
+ ALOGW("Transaction too large: inefficient and in danger of breaking: %zu bytes.", size);
}
mData.reset(new (std::nothrow) uint8_t[size]);
}
diff --git a/libs/binder/tests/IBinderRpcTest.aidl b/libs/binder/tests/IBinderRpcTest.aidl
index 1164767..dcd6461 100644
--- a/libs/binder/tests/IBinderRpcTest.aidl
+++ b/libs/binder/tests/IBinderRpcTest.aidl
@@ -34,6 +34,8 @@
void holdBinder(@nullable IBinder binder);
@nullable IBinder getHeldBinder();
+ byte[] repeatBytes(in byte[] bytes);
+
// Idea is client creates its own instance of IBinderRpcTest and calls this,
// and the server calls 'binder' with (calls - 1) passing itself as 'binder',
// going back and forth until calls = 0
diff --git a/libs/binder/tests/binderRpcTest.cpp b/libs/binder/tests/binderRpcTest.cpp
index 9f656ec..e88e3f3 100644
--- a/libs/binder/tests/binderRpcTest.cpp
+++ b/libs/binder/tests/binderRpcTest.cpp
@@ -711,6 +711,35 @@
proc.proc->sessions.erase(proc.proc->sessions.begin() + 1);
}
+// TODO(b/392717039): can we move this to universal tests?
+TEST_P(BinderRpc, SendTooLargeVector) {
+ if (GetParam().singleThreaded) {
+ GTEST_SKIP() << "Requires multi-threaded server to test one of the sessions crashing.";
+ }
+
+ auto proc = createRpcTestSocketServerProcess({.numSessions = 2});
+
+ // need a working transaction
+ EXPECT_EQ(OK, proc.rootBinder->pingBinder());
+
+ // see libbinder internal Constants.h
+ const size_t kTooLargeSize = 650 * 1024;
+ const std::vector<uint8_t> kTestValue(kTooLargeSize / sizeof(uint8_t), 42);
+
+ // TODO(b/392717039): Telling a server to allocate too much data currently causes the session to
+ // close since RpcServer treats any transaction error as a failure. We likely want to change
+ // this behavior to be a soft failure, since it isn't hard to keep track of this state.
+ sp<IBinderRpcTest> rootIface2 = interface_cast<IBinderRpcTest>(proc.proc->sessions.at(1).root);
+ std::vector<uint8_t> result;
+ status_t res = rootIface2->repeatBytes(kTestValue, &result).transactionError();
+
+ // TODO(b/392717039): consistent error results always
+ EXPECT_TRUE(res == -ECONNRESET || res == DEAD_OBJECT) << statusToString(res);
+
+ // died, so remove it for checks in destructor of proc
+ proc.proc->sessions.erase(proc.proc->sessions.begin() + 1);
+}
+
TEST_P(BinderRpc, SessionWithIncomingThreadpoolDoesntLeak) {
if (clientOrServerSingleThreaded()) {
GTEST_SKIP() << "This test requires multiple threads";
diff --git a/libs/binder/tests/binderRpcTestCommon.h b/libs/binder/tests/binderRpcTestCommon.h
index dc22647..6e00246 100644
--- a/libs/binder/tests/binderRpcTestCommon.h
+++ b/libs/binder/tests/binderRpcTestCommon.h
@@ -348,6 +348,10 @@
*out = binder;
return Status::ok();
}
+ Status repeatBytes(const std::vector<uint8_t>& bytes, std::vector<uint8_t>* out) override {
+ *out = bytes;
+ return Status::ok();
+ }
static sp<IBinder> mHeldBinder;
Status holdBinder(const sp<IBinder>& binder) override {
mHeldBinder = binder;
diff --git a/libs/binder/tests/binderRpcUniversalTests.cpp b/libs/binder/tests/binderRpcUniversalTests.cpp
index 4b9dcf8..d227e6e 100644
--- a/libs/binder/tests/binderRpcUniversalTests.cpp
+++ b/libs/binder/tests/binderRpcUniversalTests.cpp
@@ -209,6 +209,18 @@
EXPECT_EQ(0, MyBinderRpcSession::gNum);
}
+TEST_P(BinderRpc, SendLargeVector) {
+ auto proc = createRpcTestSocketServerProcess({});
+
+ // see libbinder internal Constants.h
+ const size_t kLargeSize = 550 * 1024;
+ const std::vector<uint8_t> kTestValue(kLargeSize / sizeof(uint8_t), 42);
+
+ std::vector<uint8_t> result;
+ EXPECT_OK(proc.rootIface->repeatBytes(kTestValue, &result));
+ EXPECT_EQ(result, kTestValue);
+}
+
TEST_P(BinderRpc, RepeatTheirBinder) {
auto proc = createRpcTestSocketServerProcess({});
diff --git a/libs/binder/trusty/binderRpcTest/manifest.json b/libs/binder/trusty/binderRpcTest/manifest.json
index 6e20b8a..da0f2ed 100644
--- a/libs/binder/trusty/binderRpcTest/manifest.json
+++ b/libs/binder/trusty/binderRpcTest/manifest.json
@@ -1,6 +1,6 @@
{
"uuid": "9dbe9fb8-60fd-4bdd-af86-03e95d7ad78b",
"app_name": "binderRpcTest",
- "min_heap": 262144,
+ "min_heap": 4194304,
"min_stack": 20480
}
diff --git a/libs/binder/trusty/binderRpcTest/service/manifest.json b/libs/binder/trusty/binderRpcTest/service/manifest.json
index d2a1fc0..55ff49c 100644
--- a/libs/binder/trusty/binderRpcTest/service/manifest.json
+++ b/libs/binder/trusty/binderRpcTest/service/manifest.json
@@ -1,7 +1,7 @@
{
"uuid": "87e424e5-69d7-4bbd-8b7c-7e24812cbc94",
"app_name": "binderRpcTestService",
- "min_heap": 65536,
+ "min_heap": 4194304,
"min_stack": 20480,
"mgmt_flags": {
"restart_on_exit": true,
diff --git a/libs/binder/trusty/rust/binder_rpc_test/binder_rpc_test_session/lib.rs b/libs/binder/trusty/rust/binder_rpc_test/binder_rpc_test_session/lib.rs
index 22cba44..caf3117 100644
--- a/libs/binder/trusty/rust/binder_rpc_test/binder_rpc_test_session/lib.rs
+++ b/libs/binder/trusty/rust/binder_rpc_test/binder_rpc_test_session/lib.rs
@@ -82,6 +82,9 @@
fn repeatBinder(&self, _binder: Option<&SpIBinder>) -> Result<Option<SpIBinder>, Status> {
todo!()
}
+ fn repeatBytes(&self, _bytes: &[u8]) -> Result<Vec<u8>, Status> {
+ todo!()
+ }
fn holdBinder(&self, _binder: Option<&SpIBinder>) -> Result<(), Status> {
todo!()
}
diff --git a/libs/binder/trusty/rust/binder_rpc_test/service/main.rs b/libs/binder/trusty/rust/binder_rpc_test/service/main.rs
index c4a758a..6f454be 100644
--- a/libs/binder/trusty/rust/binder_rpc_test/service/main.rs
+++ b/libs/binder/trusty/rust/binder_rpc_test/service/main.rs
@@ -96,6 +96,9 @@
None => Err(Status::from(StatusCode::BAD_VALUE)),
}
}
+ fn repeatBytes(&self, _bytes: &[u8]) -> Result<Vec<u8>, Status> {
+ todo!()
+ }
fn holdBinder(&self, binder: Option<&SpIBinder>) -> Result<(), Status> {
*HOLD_BINDER.lock().unwrap() = binder.cloned();
Ok(())