libbinder: thread count startThreadPool spawn
When startThreadPool is called, it spawns a thread which is
not counted as part of the lazy kernel-started threads.
This was discovered in fuzzers, and the test is updated.
Bug: 286237215
Test: binderLibTest
Change-Id: Ib0fa4484576f9d296b8f57f32ae536b17e5c6497
diff --git a/libs/binder/ProcessState.cpp b/libs/binder/ProcessState.cpp
index 3fa6867..02b0447 100644
--- a/libs/binder/ProcessState.cpp
+++ b/libs/binder/ProcessState.cpp
@@ -192,6 +192,7 @@
AutoMutex _l(mLock);
if (!mThreadPoolStarted) {
if (mMaxThreads == 0) {
+ // see also getThreadPoolMaxTotalThreadCount
ALOGW("Extra binder thread started, but 0 threads requested. Do not use "
"*startThreadPool when zero threads are requested.");
}
@@ -407,6 +408,11 @@
mKernelStartedThreads++;
pthread_mutex_unlock(&mThreadCountLock);
}
+ // TODO: if startThreadPool is called on another thread after the process
+ // starts up, the kernel might think that it already requested those
+ // binder threads, and additional won't be started. This is likely to
+ // cause deadlocks, and it will also cause getThreadPoolMaxTotalThreadCount
+ // to return too high of a value.
}
status_t ProcessState::setThreadPoolMaxThreadCount(size_t maxThreads) {
@@ -426,12 +432,32 @@
pthread_mutex_lock(&mThreadCountLock);
base::ScopeGuard detachGuard = [&]() { pthread_mutex_unlock(&mThreadCountLock); };
- // may actually be one more than this, if join is called
if (mThreadPoolStarted) {
- return mCurrentThreads < mKernelStartedThreads
- ? mMaxThreads
- : mMaxThreads + mCurrentThreads - mKernelStartedThreads;
+ LOG_ALWAYS_FATAL_IF(mKernelStartedThreads > mMaxThreads + 1,
+ "too many kernel-started threads: %zu > %zu + 1", mKernelStartedThreads,
+ mMaxThreads);
+
+ // calling startThreadPool starts a thread
+ size_t threads = 1;
+
+ // the kernel is configured to start up to mMaxThreads more threads
+ threads += mMaxThreads;
+
+ // Users may call IPCThreadState::joinThreadPool directly. We don't
+ // currently have a way to count this directly (it could be added by
+ // adding a separate private joinKernelThread method in IPCThreadState).
+ // So, if we are in a race between the kernel thread variable being
+ // incremented in this file and mCurrentThreads being incremented
+ // in IPCThreadState, temporarily forget about the extra join threads.
+ // This is okay, because most callers of this method only care about
+ // having 0, 1, or more threads.
+ if (mCurrentThreads > mKernelStartedThreads) {
+ threads += mCurrentThreads - mKernelStartedThreads;
+ }
+
+ return threads;
}
+
// must not be initialized or maybe has poll thread setup, we
// currently don't track this in libbinder
LOG_ALWAYS_FATAL_IF(mKernelStartedThreads != 0,
diff --git a/libs/binder/include/binder/IPCThreadState.h b/libs/binder/include/binder/IPCThreadState.h
index d261c21..9347ce4 100644
--- a/libs/binder/include/binder/IPCThreadState.h
+++ b/libs/binder/include/binder/IPCThreadState.h
@@ -147,7 +147,12 @@
void flushCommands();
bool flushIfNeeded();
- // For main functions - dangerous for libraries to use
+ // Adds the current thread into the binder threadpool.
+ //
+ // This is in addition to any threads which are started
+ // with startThreadPool. Libraries should not call this
+ // function, as they may be loaded into processes which
+ // try to configure the threadpool differently.
void joinThreadPool(bool isMain = true);
// Stop the local process.
diff --git a/libs/binder/include/binder/ProcessState.h b/libs/binder/include/binder/ProcessState.h
index 81391e9..9dc370b 100644
--- a/libs/binder/include/binder/ProcessState.h
+++ b/libs/binder/include/binder/ProcessState.h
@@ -52,7 +52,26 @@
sp<IBinder> getContextObject(const sp<IBinder>& caller);
- // For main functions - dangerous for libraries to use
+ // This should be called before startThreadPool at the beginning
+ // of a program, and libraries should never call it because programs
+ // should configure their own threadpools. The threadpool size can
+ // never be decreased.
+ //
+ // The 'maxThreads' value refers to the total number of threads
+ // that will be started by the kernel. This is in addition to any
+ // threads started by 'startThreadPool' or 'joinRpcThreadpool'.
+ status_t setThreadPoolMaxThreadCount(size_t maxThreads);
+
+ // Libraries should not call this, as processes should configure
+ // threadpools themselves. Should be called in the main function
+ // directly before any code executes or joins the threadpool.
+ //
+ // Starts one thread, PLUS those requested in setThreadPoolMaxThreadCount,
+ // PLUS those manually requested in joinThreadPool.
+ //
+ // For instance, if setThreadPoolMaxCount(3) is called and
+ // startThreadpPool (+1 thread) and joinThreadPool (+1 thread)
+ // are all called, then up to 5 threads can be started.
void startThreadPool();
[[nodiscard]] bool becomeContextManager();
@@ -63,8 +82,6 @@
// TODO: deprecate.
void spawnPooledThread(bool isMain);
- // For main functions - dangerous for libraries to use
- status_t setThreadPoolMaxThreadCount(size_t maxThreads);
status_t enableOnewaySpamDetection(bool enable);
// Set the name of the current thread to look like a threadpool
diff --git a/libs/binder/ndk/include_platform/android/binder_process.h b/libs/binder/ndk/include_platform/android/binder_process.h
index 3fbe90d..68528e1 100644
--- a/libs/binder/ndk/include_platform/android/binder_process.h
+++ b/libs/binder/ndk/include_platform/android/binder_process.h
@@ -24,7 +24,14 @@
__BEGIN_DECLS
/**
- * This creates a threadpool for incoming binder transactions if it has not already been created.
+ * This creates a threadpool for incoming binder transactions if it has not already been created,
+ * spawning one thread, and allowing the kernel to lazily start threads according to the count
+ * that is specified in ABinderProcess_setThreadPoolMaxThreadCount.
+ *
+ * For instance, if ABinderProcess_setThreadPoolMaxThreadCount(3) is called,
+ * ABinderProcess_startThreadPool() is called (+1 thread) then the main thread calls
+ * ABinderProcess_joinThreadPool() (+1 thread), up to *5* total threads will be started
+ * (2 directly, and 3 more if the kernel starts them lazily).
*
* When using this, it is expected that ABinderProcess_setupPolling and
* ABinderProcess_handlePolledCommands are not used.
@@ -36,7 +43,12 @@
/**
* This sets the maximum number of threads that can be started in the threadpool. By default, after
* startThreadPool is called, this is 15. If it is called additional times, it will only prevent
- * the kernel from starting new threads and will not delete already existing threads.
+ * the kernel from starting new threads and will not delete already existing threads. This should
+ * be called once before startThreadPool. The number of threads can never decrease.
+ *
+ * This count refers to the number of threads that will be created lazily by the kernel, in
+ * addition to the threads created by ABinderProcess_startThreadPool or
+ * ABinderProcess_joinThreadPool.
*
* Do not use this from a library. Apps setup their own threadpools, and otherwise, the main
* function should be responsible for configuring the threadpool for the entire application.
@@ -50,8 +62,9 @@
*/
bool ABinderProcess_isThreadPoolStarted(void);
/**
- * This adds the current thread to the threadpool. This may cause the threadpool to exceed the
- * maximum size.
+ * This adds the current thread to the threadpool. This thread will be in addition to the thread
+ * started by ABinderProcess_startThreadPool and the lazy kernel-started threads specified by
+ * ABinderProcess_setThreadPoolMaxThreadCount.
*
* Do not use this from a library. Apps setup their own threadpools, and otherwise, the main
* function should be responsible for configuring the threadpool for the entire application.
diff --git a/libs/binder/rust/src/state.rs b/libs/binder/rust/src/state.rs
index cc18741..b35511d 100644
--- a/libs/binder/rust/src/state.rs
+++ b/libs/binder/rust/src/state.rs
@@ -23,6 +23,9 @@
impl ProcessState {
/// Start the Binder IPC thread pool
+ ///
+ /// Starts 1 thread, plus allows the kernel to lazily start up to 'num_threads'
+ /// additional threads as specified by set_thread_pool_max_thread_count.
pub fn start_thread_pool() {
unsafe {
// Safety: Safe FFI
@@ -33,8 +36,7 @@
/// Set the maximum number of threads that can be started in the threadpool.
///
/// By default, after startThreadPool is called, this is 15. If it is called
- /// additional times, it will only prevent the kernel from starting new
- /// threads and will not delete already existing threads.
+ /// additional times, the thread pool size can only be increased.
pub fn set_thread_pool_max_thread_count(num_threads: u32) {
unsafe {
// Safety: Safe FFI
@@ -43,6 +45,9 @@
}
/// Block on the Binder IPC thread pool
+ ///
+ /// This adds additional threads in addition to what is created by
+ /// set_thread_pool_max_thread_count and start_thread_pool.
pub fn join_thread_pool() {
unsafe {
// Safety: Safe FFI
diff --git a/libs/binder/tests/binderLibTest.cpp b/libs/binder/tests/binderLibTest.cpp
index abc423b..e021af0 100644
--- a/libs/binder/tests/binderLibTest.cpp
+++ b/libs/binder/tests/binderLibTest.cpp
@@ -82,7 +82,7 @@
static constexpr int kSchedPolicy = SCHED_RR;
static constexpr int kSchedPriority = 7;
static constexpr int kSchedPriorityMore = 8;
-static constexpr int kKernelThreads = 15;
+static constexpr int kKernelThreads = 17; // anything different than the default
static String16 binderLibTestServiceName = String16("test.binderLib");
@@ -1357,17 +1357,20 @@
EXPECT_THAT(server->transact(BINDER_LIB_TEST_GET_MAX_THREAD_COUNT, data, &reply),
StatusEq(NO_ERROR));
int32_t replyi = reply.readInt32();
- // Expect 16 threads: kKernelThreads = 15 + Pool thread == 16
- EXPECT_TRUE(replyi == kKernelThreads || replyi == kKernelThreads + 1);
+ // see getThreadPoolMaxTotalThreadCount for why there is a race
+ EXPECT_TRUE(replyi == kKernelThreads + 1 || replyi == kKernelThreads + 2) << replyi;
+
EXPECT_THAT(server->transact(BINDER_LIB_TEST_PROCESS_LOCK, data, &reply), NO_ERROR);
/*
- * This will use all threads in the pool expect the main pool thread.
- * The service should run fine without locking, and the thread count should
- * not exceed 16 (15 Max + pool thread).
+ * This will use all threads in the pool but one. There are actually kKernelThreads+2
+ * available in the other process (startThreadPool, joinThreadPool, + the kernel-
+ * started threads from setThreadPoolMaxThreadCount
+ *
+ * Adding one more will cause it to deadlock.
*/
std::vector<std::thread> ts;
- for (size_t i = 0; i < kKernelThreads; i++) {
+ for (size_t i = 0; i < kKernelThreads + 1; i++) {
ts.push_back(std::thread([&] {
Parcel local_reply;
EXPECT_THAT(server->transact(BINDER_LIB_TEST_LOCK_UNLOCK, data, &local_reply),
@@ -1375,8 +1378,13 @@
}));
}
- data.writeInt32(500);
- // Give a chance for all threads to be used
+ // make sure all of the above calls will be queued in parallel. Otherwise, most of
+ // the time, the below call will pre-empt them (presumably because we have the
+ // scheduler timeslice already + scheduler hint).
+ sleep(1);
+
+ data.writeInt32(1000);
+ // Give a chance for all threads to be used (kKernelThreads + 1 thread in use)
EXPECT_THAT(server->transact(BINDER_LIB_TEST_UNLOCK_AFTER_MS, data, &reply), NO_ERROR);
for (auto &t : ts) {
@@ -1386,7 +1394,7 @@
EXPECT_THAT(server->transact(BINDER_LIB_TEST_GET_MAX_THREAD_COUNT, data, &reply),
StatusEq(NO_ERROR));
replyi = reply.readInt32();
- EXPECT_EQ(replyi, kKernelThreads + 1);
+ EXPECT_EQ(replyi, kKernelThreads + 2);
}
TEST_F(BinderLibTest, ThreadPoolStarted) {