diff options
-rw-r--r-- | libs/binder/Binder.cpp | 5 | ||||
-rw-r--r-- | libs/binder/BpBinder.cpp | 10 | ||||
-rw-r--r-- | libs/binder/Constants.h | 39 | ||||
-rw-r--r-- | libs/binder/RpcState.cpp | 11 | ||||
-rw-r--r-- | libs/binder/tests/IBinderRpcTest.aidl | 2 | ||||
-rw-r--r-- | libs/binder/tests/binderRpcTest.cpp | 29 | ||||
-rw-r--r-- | libs/binder/tests/binderRpcTestCommon.h | 4 | ||||
-rw-r--r-- | libs/binder/tests/binderRpcUniversalTests.cpp | 12 | ||||
-rw-r--r-- | libs/binder/trusty/binderRpcTest/manifest.json | 2 | ||||
-rw-r--r-- | libs/binder/trusty/binderRpcTest/service/manifest.json | 2 | ||||
-rw-r--r-- | libs/binder/trusty/rust/binder_rpc_test/binder_rpc_test_session/lib.rs | 3 | ||||
-rw-r--r-- | libs/binder/trusty/rust/binder_rpc_test/service/main.rs | 3 |
12 files changed, 108 insertions, 14 deletions
diff --git a/libs/binder/Binder.cpp b/libs/binder/Binder.cpp index 0a22588a90..bc7ae37ff0 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 = true; 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 @@ status_t BBinder::transact( // 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 444f06174e..c13e0f9e9c 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 @@ std::atomic<uint32_t> BpBinder::sBinderProxyCountWarned(0); 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_t BpBinder::transact( 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 0000000000..b75493cb8a --- /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 fe6e1a3318..03d974d186 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 @@ std::string RpcState::BinderNode::toString() const { } 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 @@ RpcState::CommandData::CommandData(size_t size) : mSize(size) { // 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 116476765a..dcd6461b53 100644 --- a/libs/binder/tests/IBinderRpcTest.aidl +++ b/libs/binder/tests/IBinderRpcTest.aidl @@ -34,6 +34,8 @@ interface IBinderRpcTest { 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 9f656ec96c..e88e3f3530 100644 --- a/libs/binder/tests/binderRpcTest.cpp +++ b/libs/binder/tests/binderRpcTest.cpp @@ -711,6 +711,35 @@ TEST_P(BinderRpc, OnewayCallExhaustion) { 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 dc22647b85..6e0024628a 100644 --- a/libs/binder/tests/binderRpcTestCommon.h +++ b/libs/binder/tests/binderRpcTestCommon.h @@ -348,6 +348,10 @@ public: *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 4b9dcf85d2..d227e6eeec 100644 --- a/libs/binder/tests/binderRpcUniversalTests.cpp +++ b/libs/binder/tests/binderRpcUniversalTests.cpp @@ -209,6 +209,18 @@ TEST_P(BinderRpc, RepeatBinder) { 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 6e20b8a7ad..da0f2ed2d8 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 d2a1fc0a48..55ff49c0b8 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 22cba44975..caf3117195 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 @@ impl IBinderRpcTest for MyBinderRpcSession { 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 c4a758a214..6f454be2b6 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 @@ impl IBinderRpcTest for TestService { 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(()) |