diff options
| -rw-r--r-- | cmds/installd/InstalldNativeService.cpp | 8 | ||||
| -rw-r--r-- | libs/binder/ProcessState.cpp | 34 | ||||
| -rw-r--r-- | libs/binder/include/binder/IPCThreadState.h | 7 | ||||
| -rw-r--r-- | libs/binder/include/binder/ProcessState.h | 23 | ||||
| -rw-r--r-- | libs/binder/ndk/include_platform/android/binder_process.h | 21 | ||||
| -rw-r--r-- | libs/binder/rust/src/state.rs | 9 | ||||
| -rw-r--r-- | libs/binder/rust/tests/parcel_fuzzer/random_parcel/fuzz_service_test/Android.bp | 5 | ||||
| -rw-r--r-- | libs/binder/tests/Android.bp | 9 | ||||
| -rw-r--r-- | libs/binder/tests/binderLibTest.cpp | 28 | ||||
| -rw-r--r-- | libs/binder/tests/binderParcelUnitTest.cpp | 161 | ||||
| -rw-r--r-- | services/inputflinger/dispatcher/include/InputDispatcherPolicyInterface.h | 3 |
11 files changed, 272 insertions, 36 deletions
diff --git a/cmds/installd/InstalldNativeService.cpp b/cmds/installd/InstalldNativeService.cpp index 99f7669a57..e84428ee41 100644 --- a/cmds/installd/InstalldNativeService.cpp +++ b/cmds/installd/InstalldNativeService.cpp @@ -3574,22 +3574,22 @@ binder::Status InstalldNativeService::tryMountDataMirror( std::lock_guard<std::recursive_mutex> lock(mMountsLock); std::string mirrorVolCePath(StringPrintf("%s/%s", kDataMirrorCePath, uuid_)); - if (fs_prepare_dir(mirrorVolCePath.c_str(), 0711, AID_SYSTEM, AID_SYSTEM) != 0) { + if (fs_prepare_dir(mirrorVolCePath.c_str(), 0511, AID_SYSTEM, AID_SYSTEM) != 0) { return error("Failed to create CE data mirror"); } std::string mirrorVolDePath(StringPrintf("%s/%s", kDataMirrorDePath, uuid_)); - if (fs_prepare_dir(mirrorVolDePath.c_str(), 0711, AID_SYSTEM, AID_SYSTEM) != 0) { + if (fs_prepare_dir(mirrorVolDePath.c_str(), 0511, AID_SYSTEM, AID_SYSTEM) != 0) { return error("Failed to create DE data mirror"); } std::string mirrorVolMiscCePath(StringPrintf("%s/%s", kMiscMirrorCePath, uuid_)); - if (fs_prepare_dir(mirrorVolMiscCePath.c_str(), 0711, AID_SYSTEM, AID_SYSTEM) != 0) { + if (fs_prepare_dir(mirrorVolMiscCePath.c_str(), 0511, AID_SYSTEM, AID_SYSTEM) != 0) { return error("Failed to create CE misc mirror"); } std::string mirrorVolMiscDePath(StringPrintf("%s/%s", kMiscMirrorDePath, uuid_)); - if (fs_prepare_dir(mirrorVolMiscDePath.c_str(), 0711, AID_SYSTEM, AID_SYSTEM) != 0) { + if (fs_prepare_dir(mirrorVolMiscDePath.c_str(), 0511, AID_SYSTEM, AID_SYSTEM) != 0) { return error("Failed to create DE misc mirror"); } diff --git a/libs/binder/ProcessState.cpp b/libs/binder/ProcessState.cpp index 3fa686782a..02b0447304 100644 --- a/libs/binder/ProcessState.cpp +++ b/libs/binder/ProcessState.cpp @@ -192,6 +192,7 @@ void ProcessState::startThreadPool() 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 @@ void ProcessState::spawnPooledThread(bool isMain) 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 @@ size_t ProcessState::getThreadPoolMaxTotalThreadCount() const { 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 d261c2143c..9347ce47a5 100644 --- a/libs/binder/include/binder/IPCThreadState.h +++ b/libs/binder/include/binder/IPCThreadState.h @@ -147,7 +147,12 @@ public: 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 81391e96e7..9dc370b412 100644 --- a/libs/binder/include/binder/ProcessState.h +++ b/libs/binder/include/binder/ProcessState.h @@ -52,7 +52,26 @@ public: 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 @@ public: // 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 3fbe90d70a..68528e1004 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 @@ void ABinderProcess_startThreadPool(void); /** * 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_setThreadPoolMaxThreadCount(uint32_t numThreads); */ 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 cc18741a4e..b35511d74e 100644 --- a/libs/binder/rust/src/state.rs +++ b/libs/binder/rust/src/state.rs @@ -23,6 +23,9 @@ pub struct ProcessState; 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 @@ impl ProcessState { /// 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 @@ impl ProcessState { } /// 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/rust/tests/parcel_fuzzer/random_parcel/fuzz_service_test/Android.bp b/libs/binder/rust/tests/parcel_fuzzer/random_parcel/fuzz_service_test/Android.bp index 89126ca2a7..2537ce0bcb 100644 --- a/libs/binder/rust/tests/parcel_fuzzer/random_parcel/fuzz_service_test/Android.bp +++ b/libs/binder/rust/tests/parcel_fuzzer/random_parcel/fuzz_service_test/Android.bp @@ -19,11 +19,6 @@ rust_fuzz { srcs: [ "service_fuzzer.rs", ], - shared_libs: [ - "libbinder", - "libbinder_ndk", - "libutils", - ], rustlibs: [ "libbinder_rs", "libbinder_random_parcel_rs", diff --git a/libs/binder/tests/Android.bp b/libs/binder/tests/Android.bp index 24fd2a6048..41856f9b79 100644 --- a/libs/binder/tests/Android.bp +++ b/libs/binder/tests/Android.bp @@ -804,5 +804,12 @@ cc_defaults { cc_defaults { name: "fuzzer_disable_leaks", - //TODO(b/286112918) : Readd leak detection options + fuzz_config: { + asan_options: [ + "detect_leaks=0", + ], + hwasan_options: [ + "detect_leaks=0", + ], + }, } diff --git a/libs/binder/tests/binderLibTest.cpp b/libs/binder/tests/binderLibTest.cpp index abc423b669..e021af0264 100644 --- a/libs/binder/tests/binderLibTest.cpp +++ b/libs/binder/tests/binderLibTest.cpp @@ -82,7 +82,7 @@ static char binderserverarg[] = "--binderserver"; 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 @@ TEST_F(BinderLibTest, ThreadPoolAvailableThreads) { 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 @@ TEST_F(BinderLibTest, ThreadPoolAvailableThreads) { })); } - 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 @@ TEST_F(BinderLibTest, ThreadPoolAvailableThreads) { 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) { diff --git a/libs/binder/tests/binderParcelUnitTest.cpp b/libs/binder/tests/binderParcelUnitTest.cpp index 359c783de5..0a0dae0f59 100644 --- a/libs/binder/tests/binderParcelUnitTest.cpp +++ b/libs/binder/tests/binderParcelUnitTest.cpp @@ -29,6 +29,7 @@ using android::sp; using android::status_t; using android::String16; using android::String8; +using android::base::unique_fd; using android::binder::Status; TEST(Parcel, NonNullTerminatedString8) { @@ -112,6 +113,166 @@ TEST(Parcel, DebugReadAllFds) { EXPECT_EQ(ret[1], STDIN_FILENO); } +TEST(Parcel, AppendFromEmpty) { + Parcel p1; + Parcel p2; + p2.writeInt32(2); + + ASSERT_EQ(OK, p1.appendFrom(&p2, 0, p2.dataSize())); + + p1.setDataPosition(0); + ASSERT_EQ(2, p1.readInt32()); + + p2.setDataPosition(0); + ASSERT_EQ(2, p2.readInt32()); +} + +TEST(Parcel, AppendPlainData) { + Parcel p1; + p1.writeInt32(1); + Parcel p2; + p2.writeInt32(2); + + ASSERT_EQ(OK, p1.appendFrom(&p2, 0, p2.dataSize())); + + p1.setDataPosition(0); + ASSERT_EQ(1, p1.readInt32()); + ASSERT_EQ(2, p1.readInt32()); + + p2.setDataPosition(0); + ASSERT_EQ(2, p2.readInt32()); +} + +TEST(Parcel, AppendPlainDataPartial) { + Parcel p1; + p1.writeInt32(1); + Parcel p2; + p2.writeInt32(2); + p2.writeInt32(3); + p2.writeInt32(4); + + // only copy 8 bytes (two int32's worth) + ASSERT_EQ(OK, p1.appendFrom(&p2, 0, 8)); + + p1.setDataPosition(0); + ASSERT_EQ(1, p1.readInt32()); + ASSERT_EQ(2, p1.readInt32()); + ASSERT_EQ(3, p1.readInt32()); + ASSERT_EQ(0, p1.readInt32()); // not 4, end of Parcel + + p2.setDataPosition(0); + ASSERT_EQ(2, p2.readInt32()); +} + +TEST(Parcel, AppendWithBinder) { + sp<IBinder> b1 = sp<BBinder>::make(); + sp<IBinder> b2 = sp<BBinder>::make(); + + Parcel p1; + p1.writeInt32(1); + p1.writeStrongBinder(b1); + Parcel p2; + p2.writeInt32(2); + p2.writeStrongBinder(b2); + + ASSERT_EQ(OK, p1.appendFrom(&p2, 0, p2.dataSize())); + + p1.setDataPosition(0); + ASSERT_EQ(1, p1.readInt32()); + ASSERT_EQ(b1, p1.readStrongBinder()); + ASSERT_EQ(2, p1.readInt32()); + ASSERT_EQ(b2, p1.readStrongBinder()); + ASSERT_EQ(2, p1.objectsCount()); + + p2.setDataPosition(0); + ASSERT_EQ(2, p2.readInt32()); + ASSERT_EQ(b2, p2.readStrongBinder()); +} + +TEST(Parcel, AppendWithBinderPartial) { + sp<IBinder> b1 = sp<BBinder>::make(); + sp<IBinder> b2 = sp<BBinder>::make(); + + Parcel p1; + p1.writeInt32(1); + p1.writeStrongBinder(b1); + Parcel p2; + p2.writeInt32(2); + p2.writeStrongBinder(b2); + + ASSERT_EQ(OK, p1.appendFrom(&p2, 0, 8)); // BAD: 4 bytes into strong binder + + p1.setDataPosition(0); + ASSERT_EQ(1, p1.readInt32()); + ASSERT_EQ(b1, p1.readStrongBinder()); + ASSERT_EQ(2, p1.readInt32()); + ASSERT_EQ(1935813253, p1.readInt32()); // whatever garbage that is there (ABI) + ASSERT_EQ(1, p1.objectsCount()); + + p2.setDataPosition(0); + ASSERT_EQ(2, p2.readInt32()); + ASSERT_EQ(b2, p2.readStrongBinder()); +} + +TEST(Parcel, AppendWithFd) { + unique_fd fd1 = unique_fd(dup(0)); + unique_fd fd2 = unique_fd(dup(0)); + + Parcel p1; + p1.writeInt32(1); + p1.writeDupFileDescriptor(0); // with ownership + p1.writeFileDescriptor(fd1.get()); // without ownership + Parcel p2; + p2.writeInt32(2); + p2.writeDupFileDescriptor(0); // with ownership + p2.writeFileDescriptor(fd2.get()); // without ownership + + ASSERT_EQ(OK, p1.appendFrom(&p2, 0, p2.dataSize())); + + p1.setDataPosition(0); + ASSERT_EQ(1, p1.readInt32()); + ASSERT_NE(-1, p1.readFileDescriptor()); + ASSERT_NE(-1, p1.readFileDescriptor()); + ASSERT_EQ(2, p1.readInt32()); + ASSERT_NE(-1, p1.readFileDescriptor()); + ASSERT_NE(-1, p1.readFileDescriptor()); + ASSERT_EQ(4, p1.objectsCount()); + + p2.setDataPosition(0); + ASSERT_EQ(2, p2.readInt32()); + ASSERT_NE(-1, p1.readFileDescriptor()); + ASSERT_NE(-1, p1.readFileDescriptor()); +} + +TEST(Parcel, AppendWithFdPartial) { + unique_fd fd1 = unique_fd(dup(0)); + unique_fd fd2 = unique_fd(dup(0)); + + Parcel p1; + p1.writeInt32(1); + p1.writeDupFileDescriptor(0); // with ownership + p1.writeFileDescriptor(fd1.get()); // without ownership + Parcel p2; + p2.writeInt32(2); + p2.writeDupFileDescriptor(0); // with ownership + p2.writeFileDescriptor(fd2.get()); // without ownership + + ASSERT_EQ(OK, p1.appendFrom(&p2, 0, 8)); // BAD: 4 bytes into binder + + p1.setDataPosition(0); + ASSERT_EQ(1, p1.readInt32()); + ASSERT_NE(-1, p1.readFileDescriptor()); + ASSERT_NE(-1, p1.readFileDescriptor()); + ASSERT_EQ(2, p1.readInt32()); + ASSERT_EQ(1717840517, p1.readInt32()); // whatever garbage that is there (ABI) + ASSERT_EQ(2, p1.objectsCount()); + + p2.setDataPosition(0); + ASSERT_EQ(2, p2.readInt32()); + ASSERT_NE(-1, p1.readFileDescriptor()); + ASSERT_NE(-1, p1.readFileDescriptor()); +} + // Tests a second operation results in a parcel at the same location as it // started. void parcelOpSameLength(const std::function<void(Parcel*)>& a, const std::function<void(Parcel*)>& b) { diff --git a/services/inputflinger/dispatcher/include/InputDispatcherPolicyInterface.h b/services/inputflinger/dispatcher/include/InputDispatcherPolicyInterface.h index 575b3d7059..fff1b03339 100644 --- a/services/inputflinger/dispatcher/include/InputDispatcherPolicyInterface.h +++ b/services/inputflinger/dispatcher/include/InputDispatcherPolicyInterface.h @@ -26,11 +26,10 @@ namespace android { - /* * Input dispatcher policy interface. * - * The input reader policy is used by the input reader to interact with the Window Manager + * The input dispatcher policy is used by the input dispatcher to interact with the Window Manager * and other system components. * * The actual implementation is partially supported by callbacks into the DVM |