diff options
44 files changed, 685 insertions, 166 deletions
diff --git a/cmds/dumpstate/Android.bp b/cmds/dumpstate/Android.bp index be96306f55..1397ae6df1 100644 --- a/cmds/dumpstate/Android.bp +++ b/cmds/dumpstate/Android.bp @@ -83,14 +83,16 @@ cc_defaults { "aconfig_lib_cc_static_link.defaults", "dumpstate_cflag_defaults", ], + // See README.md: "Dumpstate philosophy: exec not link" + // Do not add things here - keep dumpstate as simple as possible and exec where possible. shared_libs: [ "android.hardware.dumpstate@1.0", "android.hardware.dumpstate@1.1", "android.hardware.dumpstate-V1-ndk", "libziparchive", "libbase", - "libbinder", - "libbinder_ndk", + "libbinder", // BAD: dumpstate should not link code directly, should only exec binaries + "libbinder_ndk", // BAD: dumpstate should not link code directly, should only exec binaries "libcrypto", "libcutils", "libdebuggerd_client", @@ -98,11 +100,11 @@ cc_defaults { "libdumpstateutil", "libdumputils", "libhardware_legacy", - "libhidlbase", + "libhidlbase", // BAD: dumpstate should not link code directly, should only exec binaries "liblog", "libutils", - "libvintf", - "libbinderdebug", + "libvintf", // BAD: dumpstate should not link code directly, should only exec binaries + "libbinderdebug", // BAD: dumpstate should not link code directly, should only exec binaries "packagemanager_aidl-cpp", "server_configurable_flags", "device_policy_aconfig_flags_c_lib", diff --git a/cmds/dumpstate/README.md b/cmds/dumpstate/README.md index 26dabbbcef..3ab971a7e4 100644 --- a/cmds/dumpstate/README.md +++ b/cmds/dumpstate/README.md @@ -21,6 +21,18 @@ Example: mmm -j frameworks/native/cmds/dumpstate device/acme/secret_device/dumpstate/ hardware/interfaces/dumpstate ``` +## Dumpstate philosophy: exec not link + +Never link code directly into dumpstate. Dumpstate should execute many +binaries and collect the results. In general, code should fail hard fail fast, +but dumpstate is the last to solve many Android bugs. Oftentimes, failures +in core Android infrastructure or tools are issues that cause problems in +bugreport directly, so bugreport should not rely on these tools working. +We want dumpstate to have as minimal of code loaded in process so that +only that core subset needs to be bugfree for bugreport to work. Even if +many pieces of Android break, that should not prevent dumpstate from +working. + ## To build, deploy, and take a bugreport ``` diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index bb0ffe650b..c24bee12be 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -128,6 +128,9 @@ using android::os::dumpstate::PropertiesHelper; using android::os::dumpstate::TaskQueue; using android::os::dumpstate::WaitForTask; +// BAD - See README.md: "Dumpstate philosophy: exec not link" +// Do not add more complicated variables here, prefer to execute only. Don't link more code here. + // Keep in sync with // frameworks/base/services/core/java/com/android/server/am/ActivityManagerService.java static const int TRACE_DUMP_TIMEOUT_MS = 10000; // 10 seconds @@ -1266,6 +1269,8 @@ static void DumpKernelMemoryAllocations() { } } +// BAD - See README.md: "Dumpstate philosophy: exec not link" +// This should all be moved into a separate binary rather than have complex logic here. static Dumpstate::RunStatus RunDumpsysTextByPriority(const std::string& title, int priority, std::chrono::milliseconds timeout, std::chrono::milliseconds service_timeout) { @@ -1346,6 +1351,8 @@ static Dumpstate::RunStatus RunDumpsysTextNormalPriority(const std::string& titl service_timeout); } +// BAD - See README.md: "Dumpstate philosophy: exec not link" +// This should all be moved into a separate binary rather than have complex logic here. static Dumpstate::RunStatus RunDumpsysProto(const std::string& title, int priority, std::chrono::milliseconds timeout, std::chrono::milliseconds service_timeout) { @@ -1427,6 +1434,8 @@ static Dumpstate::RunStatus RunDumpsysNormal() { * Dumpstate can pick up later and output to the bugreport. Using STDOUT_FILENO * if it's not running in the parallel task. */ +// BAD - See README.md: "Dumpstate philosophy: exec not link" +// This should all be moved into a separate binary rather than have complex logic here. static void DumpHals(int out_fd = STDOUT_FILENO) { RunCommand("HARDWARE HALS", {"lshal", "--all", "--types=all"}, CommandOptions::WithTimeout(10).AsRootIfAvailable().Build(), @@ -1483,6 +1492,9 @@ static void DumpHals(int out_fd = STDOUT_FILENO) { } } +// BAD - See README.md: "Dumpstate philosophy: exec not link" +// This should all be moved into a separate binary rather than have complex logic here. +// // Dump all of the files that make up the vendor interface. // See the files listed in dumpFileList() for the latest list of files. static void DumpVintf() { @@ -1512,6 +1524,8 @@ static void DumpExternalFragmentationInfo() { printf("------ EXTERNAL FRAGMENTATION INFO ------\n"); std::ifstream ifs("/proc/buddyinfo"); auto unusable_index_regex = std::regex{"Node\\s+([0-9]+),\\s+zone\\s+(\\S+)\\s+(.*)"}; + // BAD - See README.md: "Dumpstate philosophy: exec not link" + // This should all be moved into a separate binary rather than have complex logic here. for (std::string line; std::getline(ifs, line);) { std::smatch match_results; if (std::regex_match(line, match_results, unusable_index_regex)) { @@ -2452,6 +2466,8 @@ static dumpstate_hal_aidl::IDumpstateDevice::DumpstateMode GetDumpstateHalModeAi return dumpstate_hal_aidl::IDumpstateDevice::DumpstateMode::DEFAULT; } +// BAD - See README.md: "Dumpstate philosophy: exec not link" +// This should all be moved into a separate binary rather than have complex logic here. static void DoDumpstateBoardHidl( const sp<dumpstate_hal_hidl_1_0::IDumpstateDevice> dumpstate_hal_1_0, const std::vector<::ndk::ScopedFileDescriptor>& dumpstate_fds, diff --git a/cmds/lshal/libprocpartition/include/procpartition/procpartition.h b/cmds/lshal/libprocpartition/include/procpartition/procpartition.h index ca1e690694..9c0fc18697 100644 --- a/cmds/lshal/libprocpartition/include/procpartition/procpartition.h +++ b/cmds/lshal/libprocpartition/include/procpartition/procpartition.h @@ -27,6 +27,8 @@ namespace procpartition { enum class Partition { UNKNOWN = 0, SYSTEM, + SYSTEM_EXT, + PRODUCT, VENDOR, ODM }; diff --git a/cmds/lshal/libprocpartition/procpartition.cpp b/cmds/lshal/libprocpartition/procpartition.cpp index 9645f3a3f3..35fad577a4 100644 --- a/cmds/lshal/libprocpartition/procpartition.cpp +++ b/cmds/lshal/libprocpartition/procpartition.cpp @@ -24,6 +24,8 @@ namespace procpartition { std::ostream& operator<<(std::ostream& os, Partition p) { switch (p) { case Partition::SYSTEM: return os << "system"; + case Partition::SYSTEM_EXT: return os << "system_ext"; + case Partition::PRODUCT: return os << "product"; case Partition::VENDOR: return os << "vendor"; case Partition::ODM: return os << "odm"; case Partition::UNKNOWN: // fallthrough @@ -57,6 +59,12 @@ Partition parsePartition(const std::string& s) { if (s == "system") { return Partition::SYSTEM; } + if (s == "system_ext") { + return Partition::SYSTEM_EXT; + } + if (s == "product") { + return Partition::PRODUCT; + } if (s == "vendor") { return Partition::VENDOR; } diff --git a/cmds/servicemanager/ServiceManager.cpp b/cmds/servicemanager/ServiceManager.cpp index 38a125bb54..59c4d53bc0 100644 --- a/cmds/servicemanager/ServiceManager.cpp +++ b/cmds/servicemanager/ServiceManager.cpp @@ -410,7 +410,16 @@ Status ServiceManager::getService2(const std::string& name, os::Service* outServ return Status::ok(); } -Status ServiceManager::checkService(const std::string& name, os::Service* outService) { +Status ServiceManager::checkService(const std::string& name, sp<IBinder>* outBinder) { + SM_PERFETTO_TRACE_FUNC(PERFETTO_TE_PROTO_FIELDS( + PERFETTO_TE_PROTO_FIELD_CSTR(kProtoServiceName, name.c_str()))); + + *outBinder = tryGetBinder(name, false).service; + // returns ok regardless of result for legacy reasons + return Status::ok(); +} + +Status ServiceManager::checkService2(const std::string& name, os::Service* outService) { SM_PERFETTO_TRACE_FUNC(PERFETTO_TE_PROTO_FIELDS( PERFETTO_TE_PROTO_FIELD_CSTR(kProtoServiceName, name.c_str()))); diff --git a/cmds/servicemanager/ServiceManager.h b/cmds/servicemanager/ServiceManager.h index 964abee6af..5c4d891218 100644 --- a/cmds/servicemanager/ServiceManager.h +++ b/cmds/servicemanager/ServiceManager.h @@ -46,7 +46,8 @@ public: // getService will try to start any services it cannot find binder::Status getService(const std::string& name, sp<IBinder>* outBinder) override; binder::Status getService2(const std::string& name, os::Service* outService) override; - binder::Status checkService(const std::string& name, os::Service* outService) override; + binder::Status checkService(const std::string& name, sp<IBinder>* outBinder) override; + binder::Status checkService2(const std::string& name, os::Service* outService) override; binder::Status addService(const std::string& name, const sp<IBinder>& binder, bool allowIsolated, int32_t dumpPriority) override; binder::Status listServices(int32_t dumpPriority, std::vector<std::string>* outList) override; diff --git a/cmds/servicemanager/test_sm.cpp b/cmds/servicemanager/test_sm.cpp index e620770e18..7ad84faca0 100644 --- a/cmds/servicemanager/test_sm.cpp +++ b/cmds/servicemanager/test_sm.cpp @@ -204,6 +204,11 @@ TEST(GetService, HappyHappy) { sp<IBinder> outBinder; EXPECT_TRUE(sm->getService("foo", &outBinder).isOk()); EXPECT_EQ(service, outBinder); + + EXPECT_TRUE(sm->checkService2("foo", &out).isOk()); + EXPECT_EQ(service, out.get<Service::Tag::serviceWithMetadata>().service); + EXPECT_TRUE(sm->checkService("foo", &outBinder).isOk()); + EXPECT_EQ(service, outBinder); } TEST(GetService, NonExistant) { diff --git a/include/private/OWNERS b/include/private/OWNERS index 37da96d488..db3ae48698 100644 --- a/include/private/OWNERS +++ b/include/private/OWNERS @@ -1,3 +1,4 @@ # ADPF per-file thermal_private.h = file:platform/frameworks/base:/ADPF_OWNERS per-file performance_hint_private.h = file:platform/frameworks/base:/ADPF_OWNERS +per-file system_health_private.h = file:platform/frameworks/base:/ADPF_OWNERS diff --git a/libs/binder/BackendUnifiedServiceManager.cpp b/libs/binder/BackendUnifiedServiceManager.cpp index 34d5a09948..7c0319aead 100644 --- a/libs/binder/BackendUnifiedServiceManager.cpp +++ b/libs/binder/BackendUnifiedServiceManager.cpp @@ -15,6 +15,7 @@ */ #include "BackendUnifiedServiceManager.h" +#include <android-base/strings.h> #include <android/os/IAccessor.h> #include <android/os/IServiceManager.h> #include <binder/RpcSession.h> @@ -47,6 +48,9 @@ using AidlServiceManager = android::os::IServiceManager; using android::os::IAccessor; using binder::Status; +static const char* kUnsupportedOpNoServiceManager = + "Unsupported operation without a kernel binder servicemanager process"; + static const char* kStaticCachableList[] = { // go/keep-sorted start "accessibility", @@ -211,7 +215,9 @@ Status BackendUnifiedServiceManager::getService(const ::std::string& name, sp<IBinder>* _aidl_return) { os::Service service; Status status = getService2(name, &service); - *_aidl_return = service.get<os::Service::Tag::serviceWithMetadata>().service; + if (status.isOk()) { + *_aidl_return = service.get<os::Service::Tag::serviceWithMetadata>().service; + } return status; } @@ -220,7 +226,10 @@ Status BackendUnifiedServiceManager::getService2(const ::std::string& name, os:: return Status::ok(); } os::Service service; - Status status = mTheRealServiceManager->getService2(name, &service); + Status status = Status::ok(); + if (mTheRealServiceManager) { + status = mTheRealServiceManager->getService2(name, &service); + } if (status.isOk()) { status = toBinderService(name, service, _out); @@ -231,13 +240,26 @@ Status BackendUnifiedServiceManager::getService2(const ::std::string& name, os:: return status; } -Status BackendUnifiedServiceManager::checkService(const ::std::string& name, os::Service* _out) { +Status BackendUnifiedServiceManager::checkService(const ::std::string& name, + sp<IBinder>* _aidl_return) { + os::Service service; + Status status = checkService2(name, &service); + if (status.isOk()) { + *_aidl_return = service.get<os::Service::Tag::serviceWithMetadata>().service; + } + return status; +} + +Status BackendUnifiedServiceManager::checkService2(const ::std::string& name, os::Service* _out) { os::Service service; if (returnIfCached(name, _out)) { return Status::ok(); } - Status status = mTheRealServiceManager->checkService(name, &service); + Status status = Status::ok(); + if (mTheRealServiceManager) { + status = mTheRealServiceManager->checkService2(name, &service); + } if (status.isOk()) { status = toBinderService(name, service, _out); if (status.isOk()) { @@ -315,66 +337,156 @@ Status BackendUnifiedServiceManager::toBinderService(const ::std::string& name, Status BackendUnifiedServiceManager::addService(const ::std::string& name, const sp<IBinder>& service, bool allowIsolated, int32_t dumpPriority) { - Status status = mTheRealServiceManager->addService(name, service, allowIsolated, dumpPriority); - // mEnableAddServiceCache is true by default. - if (kUseCacheInAddService && mEnableAddServiceCache && status.isOk()) { - return updateCache(name, service, - dumpPriority & android::os::IServiceManager::FLAG_IS_LAZY_SERVICE); + if (mTheRealServiceManager) { + Status status = + mTheRealServiceManager->addService(name, service, allowIsolated, dumpPriority); + // mEnableAddServiceCache is true by default. + if (kUseCacheInAddService && mEnableAddServiceCache && status.isOk()) { + return updateCache(name, service, + dumpPriority & android::os::IServiceManager::FLAG_IS_LAZY_SERVICE); + } + return status; } - return status; + return Status::fromExceptionCode(Status::EX_UNSUPPORTED_OPERATION, + kUnsupportedOpNoServiceManager); } Status BackendUnifiedServiceManager::listServices(int32_t dumpPriority, ::std::vector<::std::string>* _aidl_return) { - return mTheRealServiceManager->listServices(dumpPriority, _aidl_return); + Status status = Status::ok(); + if (mTheRealServiceManager) { + status = mTheRealServiceManager->listServices(dumpPriority, _aidl_return); + } + if (!status.isOk()) return status; + + appendInjectedAccessorServices(_aidl_return); + + return status; } Status BackendUnifiedServiceManager::registerForNotifications( const ::std::string& name, const sp<os::IServiceCallback>& callback) { - return mTheRealServiceManager->registerForNotifications(name, callback); + if (mTheRealServiceManager) { + return mTheRealServiceManager->registerForNotifications(name, callback); + } + return Status::fromExceptionCode(Status::EX_UNSUPPORTED_OPERATION, + kUnsupportedOpNoServiceManager); } Status BackendUnifiedServiceManager::unregisterForNotifications( const ::std::string& name, const sp<os::IServiceCallback>& callback) { - return mTheRealServiceManager->unregisterForNotifications(name, callback); + if (mTheRealServiceManager) { + return mTheRealServiceManager->unregisterForNotifications(name, callback); + } + return Status::fromExceptionCode(Status::EX_UNSUPPORTED_OPERATION, + kUnsupportedOpNoServiceManager); } Status BackendUnifiedServiceManager::isDeclared(const ::std::string& name, bool* _aidl_return) { - return mTheRealServiceManager->isDeclared(name, _aidl_return); + Status status = Status::ok(); + if (mTheRealServiceManager) { + status = mTheRealServiceManager->isDeclared(name, _aidl_return); + } + if (!status.isOk()) return status; + + if (!*_aidl_return) { + forEachInjectedAccessorService([&](const std::string& instance) { + if (name == instance) { + *_aidl_return = true; + } + }); + } + + return status; } Status BackendUnifiedServiceManager::getDeclaredInstances( const ::std::string& iface, ::std::vector<::std::string>* _aidl_return) { - return mTheRealServiceManager->getDeclaredInstances(iface, _aidl_return); + Status status = Status::ok(); + if (mTheRealServiceManager) { + status = mTheRealServiceManager->getDeclaredInstances(iface, _aidl_return); + } + if (!status.isOk()) return status; + + forEachInjectedAccessorService([&](const std::string& instance) { + // Declared instances have the format + // <interface>/instance like foo.bar.ISomething/instance + // If it does not have that format, consider the instance to be "" + std::string_view name(instance); + if (base::ConsumePrefix(&name, iface + "/")) { + _aidl_return->emplace_back(name); + } else if (iface == instance) { + _aidl_return->push_back(""); + } + }); + + return status; } Status BackendUnifiedServiceManager::updatableViaApex( const ::std::string& name, ::std::optional<::std::string>* _aidl_return) { - return mTheRealServiceManager->updatableViaApex(name, _aidl_return); + if (mTheRealServiceManager) { + return mTheRealServiceManager->updatableViaApex(name, _aidl_return); + } + return Status::fromExceptionCode(Status::EX_UNSUPPORTED_OPERATION, + kUnsupportedOpNoServiceManager); } Status BackendUnifiedServiceManager::getUpdatableNames(const ::std::string& apexName, ::std::vector<::std::string>* _aidl_return) { - return mTheRealServiceManager->getUpdatableNames(apexName, _aidl_return); + if (mTheRealServiceManager) { + return mTheRealServiceManager->getUpdatableNames(apexName, _aidl_return); + } + return Status::fromExceptionCode(Status::EX_UNSUPPORTED_OPERATION, + kUnsupportedOpNoServiceManager); } Status BackendUnifiedServiceManager::getConnectionInfo( const ::std::string& name, ::std::optional<os::ConnectionInfo>* _aidl_return) { - return mTheRealServiceManager->getConnectionInfo(name, _aidl_return); + if (mTheRealServiceManager) { + return mTheRealServiceManager->getConnectionInfo(name, _aidl_return); + } + return Status::fromExceptionCode(Status::EX_UNSUPPORTED_OPERATION, + kUnsupportedOpNoServiceManager); } Status BackendUnifiedServiceManager::registerClientCallback( const ::std::string& name, const sp<IBinder>& service, const sp<os::IClientCallback>& callback) { - return mTheRealServiceManager->registerClientCallback(name, service, callback); + if (mTheRealServiceManager) { + return mTheRealServiceManager->registerClientCallback(name, service, callback); + } + return Status::fromExceptionCode(Status::EX_UNSUPPORTED_OPERATION, + kUnsupportedOpNoServiceManager); } Status BackendUnifiedServiceManager::tryUnregisterService(const ::std::string& name, const sp<IBinder>& service) { - return mTheRealServiceManager->tryUnregisterService(name, service); + if (mTheRealServiceManager) { + return mTheRealServiceManager->tryUnregisterService(name, service); + } + return Status::fromExceptionCode(Status::EX_UNSUPPORTED_OPERATION, + kUnsupportedOpNoServiceManager); } Status BackendUnifiedServiceManager::getServiceDebugInfo( ::std::vector<os::ServiceDebugInfo>* _aidl_return) { - return mTheRealServiceManager->getServiceDebugInfo(_aidl_return); + if (mTheRealServiceManager) { + return mTheRealServiceManager->getServiceDebugInfo(_aidl_return); + } + return Status::fromExceptionCode(Status::EX_UNSUPPORTED_OPERATION, + kUnsupportedOpNoServiceManager); } [[clang::no_destroy]] static std::once_flag gUSmOnce; [[clang::no_destroy]] static sp<BackendUnifiedServiceManager> gUnifiedServiceManager; +static bool hasOutOfProcessServiceManager() { +#ifndef BINDER_WITH_KERNEL_IPC + return false; +#else +#if defined(__BIONIC__) && !defined(__ANDROID_VNDK__) + return android::base::GetBoolProperty("servicemanager.installed", true); +#else + return true; +#endif +#endif // BINDER_WITH_KERNEL_IPC +} + sp<BackendUnifiedServiceManager> getBackendUnifiedServiceManager() { std::call_once(gUSmOnce, []() { #if defined(__BIONIC__) && !defined(__ANDROID_VNDK__) - /* wait for service manager */ { + /* wait for service manager */ + if (hasOutOfProcessServiceManager()) { using std::literals::chrono_literals::operator""s; using android::base::WaitForProperty; while (!WaitForProperty("servicemanager.ready", "true", 1s)) { @@ -384,7 +496,7 @@ sp<BackendUnifiedServiceManager> getBackendUnifiedServiceManager() { #endif sp<AidlServiceManager> sm = nullptr; - while (sm == nullptr) { + while (hasOutOfProcessServiceManager() && sm == nullptr) { sm = interface_cast<AidlServiceManager>( ProcessState::self()->getContextObject(nullptr)); if (sm == nullptr) { diff --git a/libs/binder/BackendUnifiedServiceManager.h b/libs/binder/BackendUnifiedServiceManager.h index 6a0d06a079..c14f28063f 100644 --- a/libs/binder/BackendUnifiedServiceManager.h +++ b/libs/binder/BackendUnifiedServiceManager.h @@ -122,7 +122,8 @@ public: binder::Status getService(const ::std::string& name, sp<IBinder>* _aidl_return) override; binder::Status getService2(const ::std::string& name, os::Service* out) override; - binder::Status checkService(const ::std::string& name, os::Service* out) override; + binder::Status checkService(const ::std::string& name, sp<IBinder>* _aidl_return) override; + binder::Status checkService2(const ::std::string& name, os::Service* out) override; binder::Status addService(const ::std::string& name, const sp<IBinder>& service, bool allowIsolated, int32_t dumpPriority) override; binder::Status listServices(int32_t dumpPriority, @@ -167,5 +168,9 @@ private: sp<BackendUnifiedServiceManager> getBackendUnifiedServiceManager(); android::binder::Status getInjectedAccessor(const std::string& name, android::os::Service* service); +void appendInjectedAccessorServices(std::vector<std::string>* list); +// Do not call any other service manager APIs that might take the accessor +// mutex because this will be holding it! +void forEachInjectedAccessorService(const std::function<void(const std::string&)>& f); } // namespace android diff --git a/libs/binder/Binder.cpp b/libs/binder/Binder.cpp index 53bd08d420..0a22588a90 100644 --- a/libs/binder/Binder.cpp +++ b/libs/binder/Binder.cpp @@ -288,7 +288,7 @@ public: // for below objects RpcMutex mLock; std::set<sp<RpcServerLink>> mRpcServerLinks; - BpBinder::ObjectManager mObjects; + BpBinder::ObjectManager mObjectMgr; unique_fd mRecordingFd; }; @@ -468,7 +468,7 @@ void* BBinder::attachObject(const void* objectID, void* object, void* cleanupCoo LOG_ALWAYS_FATAL_IF(!e, "no memory"); RpcMutexUniqueLock _l(e->mLock); - return e->mObjects.attach(objectID, object, cleanupCookie, func); + return e->mObjectMgr.attach(objectID, object, cleanupCookie, func); } void* BBinder::findObject(const void* objectID) const @@ -477,7 +477,7 @@ void* BBinder::findObject(const void* objectID) const if (!e) return nullptr; RpcMutexUniqueLock _l(e->mLock); - return e->mObjects.find(objectID); + return e->mObjectMgr.find(objectID); } void* BBinder::detachObject(const void* objectID) { @@ -485,7 +485,7 @@ void* BBinder::detachObject(const void* objectID) { if (!e) return nullptr; RpcMutexUniqueLock _l(e->mLock); - return e->mObjects.detach(objectID); + return e->mObjectMgr.detach(objectID); } void BBinder::withLock(const std::function<void()>& doWithLock) { @@ -501,7 +501,7 @@ sp<IBinder> BBinder::lookupOrCreateWeak(const void* objectID, object_make_func m Extras* e = getOrCreateExtras(); LOG_ALWAYS_FATAL_IF(!e, "no memory"); RpcMutexUniqueLock _l(e->mLock); - return e->mObjects.lookupOrCreateWeak(objectID, make, makeArgs); + return e->mObjectMgr.lookupOrCreateWeak(objectID, make, makeArgs); } BBinder* BBinder::localBinder() diff --git a/libs/binder/BpBinder.cpp b/libs/binder/BpBinder.cpp index 3758b6521c..444f06174e 100644 --- a/libs/binder/BpBinder.cpp +++ b/libs/binder/BpBinder.cpp @@ -78,7 +78,16 @@ BpBinder::ObjectManager::ObjectManager() BpBinder::ObjectManager::~ObjectManager() { - kill(); + const size_t N = mObjects.size(); + ALOGV("Killing %zu objects in manager %p", N, this); + for (auto i : mObjects) { + const entry_t& e = i.second; + if (e.func != nullptr) { + e.func(i.first, e.object, e.cleanupCookie); + } + } + + mObjects.clear(); } void* BpBinder::ObjectManager::attach(const void* objectID, void* object, void* cleanupCookie, @@ -144,20 +153,6 @@ sp<IBinder> BpBinder::ObjectManager::lookupOrCreateWeak(const void* objectID, ob return newObj; } -void BpBinder::ObjectManager::kill() -{ - const size_t N = mObjects.size(); - ALOGV("Killing %zu objects in manager %p", N, this); - for (auto i : mObjects) { - const entry_t& e = i.second; - if (e.func != nullptr) { - e.func(i.first, e.object, e.cleanupCookie); - } - } - - mObjects.clear(); -} - // --------------------------------------------------------------------------- sp<BpBinder> BpBinder::create(int32_t handle, std::function<void()>* postTask) { @@ -697,19 +692,19 @@ void BpBinder::reportOneDeath(const Obituary& obit) void* BpBinder::attachObject(const void* objectID, void* object, void* cleanupCookie, object_cleanup_func func) { RpcMutexUniqueLock _l(mLock); - ALOGV("Attaching object %p to binder %p (manager=%p)", object, this, &mObjects); - return mObjects.attach(objectID, object, cleanupCookie, func); + ALOGV("Attaching object %p to binder %p (manager=%p)", object, this, &mObjectMgr); + return mObjectMgr.attach(objectID, object, cleanupCookie, func); } void* BpBinder::findObject(const void* objectID) const { RpcMutexUniqueLock _l(mLock); - return mObjects.find(objectID); + return mObjectMgr.find(objectID); } void* BpBinder::detachObject(const void* objectID) { RpcMutexUniqueLock _l(mLock); - return mObjects.detach(objectID); + return mObjectMgr.detach(objectID); } void BpBinder::withLock(const std::function<void()>& doWithLock) { @@ -720,7 +715,7 @@ void BpBinder::withLock(const std::function<void()>& doWithLock) { sp<IBinder> BpBinder::lookupOrCreateWeak(const void* objectID, object_make_func make, const void* makeArgs) { RpcMutexUniqueLock _l(mLock); - return mObjects.lookupOrCreateWeak(objectID, make, makeArgs); + return mObjectMgr.lookupOrCreateWeak(objectID, make, makeArgs); } BpBinder* BpBinder::remoteBinder() diff --git a/libs/binder/IServiceManager.cpp b/libs/binder/IServiceManager.cpp index 53435c357b..719e445794 100644 --- a/libs/binder/IServiceManager.cpp +++ b/libs/binder/IServiceManager.cpp @@ -304,6 +304,25 @@ android::binder::Status getInjectedAccessor(const std::string& name, return android::binder::Status::ok(); } +void appendInjectedAccessorServices(std::vector<std::string>* list) { + LOG_ALWAYS_FATAL_IF(list == nullptr, + "Attempted to get list of services from Accessors with nullptr"); + std::lock_guard<std::mutex> lock(gAccessorProvidersMutex); + for (const auto& entry : gAccessorProviders) { + list->insert(list->end(), entry.mProvider->instances().begin(), + entry.mProvider->instances().end()); + } +} + +void forEachInjectedAccessorService(const std::function<void(const std::string&)>& f) { + std::lock_guard<std::mutex> lock(gAccessorProvidersMutex); + for (const auto& entry : gAccessorProviders) { + for (const auto& instance : entry.mProvider->instances()) { + f(instance); + } + } +} + sp<IServiceManager> defaultServiceManager() { std::call_once(gSmOnce, []() { @@ -605,7 +624,7 @@ sp<IBinder> CppBackendShim::getService(const String16& name) const { sp<IBinder> CppBackendShim::checkService(const String16& name) const { Service ret; - if (!mUnifiedServiceManager->checkService(String8(name).c_str(), &ret).isOk()) { + if (!mUnifiedServiceManager->checkService2(String8(name).c_str(), &ret).isOk()) { return nullptr; } return ret.get<Service::Tag::serviceWithMetadata>().service; diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp index 2d65cf53c8..a97d111b97 100644 --- a/libs/binder/Parcel.cpp +++ b/libs/binder/Parcel.cpp @@ -1314,10 +1314,6 @@ status_t Parcel::writeUniqueFileDescriptorVector(const std::vector<unique_fd>& v status_t Parcel::writeUniqueFileDescriptorVector(const std::optional<std::vector<unique_fd>>& val) { return writeData(val); } -status_t Parcel::writeUniqueFileDescriptorVector( - const std::unique_ptr<std::vector<unique_fd>>& val) { - return writeData(val); -} status_t Parcel::writeStrongBinderVector(const std::vector<sp<IBinder>>& val) { return writeData(val); } status_t Parcel::writeStrongBinderVector(const std::optional<std::vector<sp<IBinder>>>& val) { return writeData(val); } @@ -1373,10 +1369,6 @@ status_t Parcel::readUtf8VectorFromUtf16Vector(std::vector<std::string>* val) co status_t Parcel::readUniqueFileDescriptorVector(std::optional<std::vector<unique_fd>>* val) const { return readData(val); } -status_t Parcel::readUniqueFileDescriptorVector( - std::unique_ptr<std::vector<unique_fd>>* val) const { - return readData(val); -} status_t Parcel::readUniqueFileDescriptorVector(std::vector<unique_fd>* val) const { return readData(val); } @@ -3287,14 +3279,6 @@ void Parcel::scanForFds() const { } #ifdef BINDER_WITH_KERNEL_IPC -size_t Parcel::getBlobAshmemSize() const -{ - // This used to return the size of all blobs that were written to ashmem, now we're returning - // the ashmem currently referenced by this Parcel, which should be equivalent. - // TODO(b/202029388): Remove method once ABI can be changed. - return getOpenAshmemSize(); -} - size_t Parcel::getOpenAshmemSize() const { auto* kernelFields = maybeKernelFields(); diff --git a/libs/binder/TEST_MAPPING b/libs/binder/TEST_MAPPING index 9e5e79f89b..4332f8ae79 100644 --- a/libs/binder/TEST_MAPPING +++ b/libs/binder/TEST_MAPPING @@ -37,6 +37,9 @@ "name": "binderStabilityTest" }, { + "name": "binderStabilityIntegrationTest" + }, + { "name": "binderRpcWireProtocolTest" }, { diff --git a/libs/binder/aidl/android/os/IServiceManager.aidl b/libs/binder/aidl/android/os/IServiceManager.aidl index 69edef86aa..6539238ce7 100644 --- a/libs/binder/aidl/android/os/IServiceManager.aidl +++ b/libs/binder/aidl/android/os/IServiceManager.aidl @@ -83,11 +83,20 @@ interface IServiceManager { /** * Retrieve an existing service called @a name from the service + * manager. Non-blocking. Returns null if the service does not exist. + * + * @deprecated TODO(b/355394904): Use checkService2 instead. This does not + * return metadata that is included in ServiceWithMetadata + */ + @UnsupportedAppUsage + @nullable IBinder checkService(@utf8InCpp String name); + + /** + * Retrieve an existing service called @a name from the service * manager. Non-blocking. Returns null if the service does not * exist. */ - @UnsupportedAppUsage - Service checkService(@utf8InCpp String name); + Service checkService2(@utf8InCpp String name); /** * Place a new @a service called @a name into the service diff --git a/libs/binder/include/binder/BpBinder.h b/libs/binder/include/binder/BpBinder.h index 7518044ce6..935bd8dbc6 100644 --- a/libs/binder/include/binder/BpBinder.h +++ b/libs/binder/include/binder/BpBinder.h @@ -104,6 +104,7 @@ public: // Stop the current recording. LIBBINDER_EXPORTED status_t stopRecordingBinder(); + // Note: This class is not thread safe so protect uses of it when necessary class ObjectManager { public: ObjectManager(); @@ -116,8 +117,6 @@ public: sp<IBinder> lookupOrCreateWeak(const void* objectID, IBinder::object_make_func make, const void* makeArgs); - void kill(); - private: ObjectManager(const ObjectManager&); ObjectManager& operator=(const ObjectManager&); @@ -224,7 +223,7 @@ private: volatile int32_t mObitsSent; Vector<Obituary>* mObituaries; std::unique_ptr<FrozenStateChange> mFrozen; - ObjectManager mObjects; + ObjectManager mObjectMgr; mutable String16 mDescriptorCache; int32_t mTrackedUid; diff --git a/libs/binder/include/binder/IInterface.h b/libs/binder/include/binder/IInterface.h index cdee17c2ee..bb45ad2ad5 100644 --- a/libs/binder/include/binder/IInterface.h +++ b/libs/binder/include/binder/IInterface.h @@ -243,7 +243,6 @@ constexpr const char* const kManualInterfaces[] = { "android.media.IMediaHTTPService", "android.media.IMediaLogService", "android.media.IMediaMetadataRetriever", - "android.media.IMediaMetricsService", "android.media.IMediaPlayer", "android.media.IMediaPlayerClient", "android.media.IMediaPlayerService", diff --git a/libs/binder/include/binder/IServiceManager.h b/libs/binder/include/binder/IServiceManager.h index 7d79baa34d..d248f22e89 100644 --- a/libs/binder/include/binder/IServiceManager.h +++ b/libs/binder/include/binder/IServiceManager.h @@ -80,6 +80,14 @@ public: /** * Register a service. + * + * Note: + * This status_t return value may be an exception code from an underlying + * Status type that doesn't have a representive error code in + * utils/Errors.h. + * One example of this is a return value of -7 + * (Status::Exception::EX_UNSUPPORTED_OPERATION) when the service manager + * process is not installed on the device when addService is called. */ // NOLINTNEXTLINE(google-default-arguments) virtual status_t addService(const String16& name, const sp<IBinder>& service, diff --git a/libs/binder/include/binder/Parcel.h b/libs/binder/include/binder/Parcel.h index 15a0da765e..9cd2ae9c25 100644 --- a/libs/binder/include/binder/Parcel.h +++ b/libs/binder/include/binder/Parcel.h @@ -383,11 +383,13 @@ public: LIBBINDER_EXPORTED status_t writeUniqueFileDescriptorVector(const std::optional<std::vector<binder::unique_fd>>& val); LIBBINDER_EXPORTED status_t - writeUniqueFileDescriptorVector(const std::unique_ptr<std::vector<binder::unique_fd>>& val) - __attribute__((deprecated("use std::optional version instead"))); - LIBBINDER_EXPORTED status_t writeUniqueFileDescriptorVector(const std::vector<binder::unique_fd>& val); + // WARNING: deprecated and incompatible with AIDL. You should use Parcelable + // definitions outside of Parcel to represent shared memory, such as + // IMemory or with ParcelFileDescriptor. We should remove this, or move it to be + // external to Parcel, it's not a very encapsulated API. + // // Writes a blob to the parcel. // If the blob is small, then it is stored in-place, otherwise it is // transferred by way of an anonymous shared memory region. Prefer sending @@ -401,8 +403,6 @@ public: // as long as it keeps a dup of the blob file descriptor handy for later. LIBBINDER_EXPORTED status_t writeDupImmutableBlobFileDescriptor(int fd); - LIBBINDER_EXPORTED status_t writeObject(const flat_binder_object& val, bool nullMetaData); - // Like Parcel.java's writeNoException(). Just writes a zero int32. // Currently the native implementation doesn't do any of the StrictMode // stack gathering and serialization that the Java implementation does. @@ -626,11 +626,13 @@ public: LIBBINDER_EXPORTED status_t readUniqueFileDescriptorVector(std::optional<std::vector<binder::unique_fd>>* val) const; LIBBINDER_EXPORTED status_t - readUniqueFileDescriptorVector(std::unique_ptr<std::vector<binder::unique_fd>>* val) const - __attribute__((deprecated("use std::optional version instead"))); - LIBBINDER_EXPORTED status_t readUniqueFileDescriptorVector(std::vector<binder::unique_fd>* val) const; + // WARNING: deprecated and incompatible with AIDL. You should use Parcelable + // definitions outside of Parcel to represent shared memory, such as + // IMemory or with ParcelFileDescriptor. We should remove this, or move it to be + // external to Parcel, it's not a very encapsulated API. + // // Reads a blob from the parcel. // The caller should call release() on the blob after reading its contents. LIBBINDER_EXPORTED status_t readBlob(size_t len, ReadableBlob* outBlob) const; @@ -679,6 +681,7 @@ private: // Set the capacity to `desired`, truncating the Parcel if necessary. status_t continueWrite(size_t desired); status_t truncateRpcObjects(size_t newObjectsSize); + status_t writeObject(const flat_binder_object& val, bool nullMetaData); status_t writePointer(uintptr_t val); status_t readPointer(uintptr_t *pArg) const; uintptr_t readPointer() const; @@ -1479,14 +1482,15 @@ public: * Note: for historical reasons, this does not include ashmem memory which * is referenced by this Parcel, but which this parcel doesn't own (e.g. * writeFileDescriptor is called without 'takeOwnership' true). + * + * WARNING: you should not use this, but rather, unparcel, and inspect + * each FD independently. This counts ashmem size, but there may be + * other resources used for non-ashmem FDs, such as other types of + * shared memory, files, etc.. */ LIBBINDER_EXPORTED size_t getOpenAshmemSize() const; private: - // TODO(b/202029388): Remove 'getBlobAshmemSize' once no prebuilts reference - // this - LIBBINDER_EXPORTED size_t getBlobAshmemSize() const; - // Needed so that we can save object metadata to the disk friend class android::binder::debug::RecordedTransaction; }; diff --git a/libs/binder/include/binder/SafeInterface.h b/libs/binder/include/binder/SafeInterface.h index 0b4f196b8f..bcbd14f9d4 100644 --- a/libs/binder/include/binder/SafeInterface.h +++ b/libs/binder/include/binder/SafeInterface.h @@ -34,6 +34,13 @@ namespace android { namespace SafeInterface { +/** + * WARNING: Prefer to use AIDL-generated interfaces. Using SafeInterface to generate interfaces + * does not support tracing, and many other AIDL features out of the box. The general direction + * we should go is to migrate safe interface users to AIDL and then remove this so that there + * is only one thing to learn/use/test/integrate, not this as well. + */ + // ParcelHandler is responsible for writing/reading various types to/from a Parcel in a generic way class LIBBINDER_EXPORTED ParcelHandler { public: diff --git a/libs/binder/include/binder/Stability.h b/libs/binder/include/binder/Stability.h index cafb8aa04b..bfe0a5af9c 100644 --- a/libs/binder/include/binder/Stability.h +++ b/libs/binder/include/binder/Stability.h @@ -20,6 +20,8 @@ #include <binder/IBinder.h> #include <string> +class BinderStabilityIntegrationTest_ExpectedStabilityForItsPartition_Test; + namespace android { class BpBinder; @@ -127,6 +129,8 @@ private: // through Parcel) friend ::android::ProcessState; + friend ::BinderStabilityIntegrationTest_ExpectedStabilityForItsPartition_Test; + static void tryMarkCompilationUnit(IBinder* binder); // Currently, we use int16_t for Level so that it can fit in BBinder. @@ -156,11 +160,11 @@ private: uint32_t flags); // get stability information as encoded on the wire - static int16_t getRepr(IBinder* binder); + LIBBINDER_EXPORTED static int16_t getRepr(IBinder* binder); // whether a transaction on binder is allowed, if the transaction // is done from a context with a specific stability level - static bool check(int16_t provided, Level required); + LIBBINDER_EXPORTED static bool check(int16_t provided, Level required); static bool isDeclaredLevel(int32_t level); static std::string levelString(int32_t level); diff --git a/libs/binder/ndk/tests/binderVendorDoubleLoadTest.cpp b/libs/binder/ndk/tests/binderVendorDoubleLoadTest.cpp index 66be94f5b5..fb92e05de5 100644 --- a/libs/binder/ndk/tests/binderVendorDoubleLoadTest.cpp +++ b/libs/binder/ndk/tests/binderVendorDoubleLoadTest.cpp @@ -30,6 +30,8 @@ #include <gtest/gtest.h> #include <sys/prctl.h> +static_assert(FLAG_PRIVATE_LOCAL != 0, "Build system configuration breaks stability"); + using namespace android; using ::android::binder::Status; using ::android::internal::Stability; diff --git a/libs/binder/rust/src/parcel.rs b/libs/binder/rust/src/parcel.rs index 485b0bdb0d..2d40ced2fd 100644 --- a/libs/binder/rust/src/parcel.rs +++ b/libs/binder/rust/src/parcel.rs @@ -184,7 +184,7 @@ unsafe impl AsNative<sys::AParcel> for Parcel { /// Safety: The `BorrowedParcel` constructors guarantee that a `BorrowedParcel` /// object will always contain a valid pointer to an `AParcel`. -unsafe impl<'a> AsNative<sys::AParcel> for BorrowedParcel<'a> { +unsafe impl AsNative<sys::AParcel> for BorrowedParcel<'_> { fn as_native(&self) -> *const sys::AParcel { self.ptr.as_ptr() } @@ -195,7 +195,7 @@ unsafe impl<'a> AsNative<sys::AParcel> for BorrowedParcel<'a> { } // Data serialization methods -impl<'a> BorrowedParcel<'a> { +impl BorrowedParcel<'_> { /// Data written to parcelable is zero'd before being deleted or reallocated. #[cfg(not(android_ndk))] pub fn mark_sensitive(&mut self) { @@ -334,7 +334,7 @@ impl<'a> BorrowedParcel<'a> { /// A segment of a writable parcel, used for [`BorrowedParcel::sized_write`]. pub struct WritableSubParcel<'a>(BorrowedParcel<'a>); -impl<'a> WritableSubParcel<'a> { +impl WritableSubParcel<'_> { /// Write a type that implements [`Serialize`] to the sub-parcel. pub fn write<S: Serialize + ?Sized>(&mut self, parcelable: &S) -> Result<()> { parcelable.serialize(&mut self.0) @@ -440,7 +440,7 @@ impl Parcel { } // Data deserialization methods -impl<'a> BorrowedParcel<'a> { +impl BorrowedParcel<'_> { /// Attempt to read a type that implements [`Deserialize`] from this parcel. pub fn read<D: Deserialize>(&self) -> Result<D> { D::deserialize(self) @@ -565,7 +565,7 @@ pub struct ReadableSubParcel<'a> { end_position: i32, } -impl<'a> ReadableSubParcel<'a> { +impl ReadableSubParcel<'_> { /// Read a type that implements [`Deserialize`] from the sub-parcel. pub fn read<D: Deserialize>(&self) -> Result<D> { D::deserialize(&self.parcel) @@ -649,7 +649,7 @@ impl Parcel { } // Internal APIs -impl<'a> BorrowedParcel<'a> { +impl BorrowedParcel<'_> { pub(crate) fn write_binder(&mut self, binder: Option<&SpIBinder>) -> Result<()> { // Safety: `BorrowedParcel` always contains a valid pointer to an // `AParcel`. `AsNative` for `Option<SpIBinder`> will either return @@ -702,7 +702,7 @@ impl fmt::Debug for Parcel { } } -impl<'a> fmt::Debug for BorrowedParcel<'a> { +impl fmt::Debug for BorrowedParcel<'_> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("BorrowedParcel").finish() } diff --git a/libs/binder/rust/src/system_only.rs b/libs/binder/rust/src/system_only.rs index 1a58d6b44d..50aa336641 100644 --- a/libs/binder/rust/src/system_only.rs +++ b/libs/binder/rust/src/system_only.rs @@ -23,22 +23,17 @@ use std::ffi::{c_void, CStr, CString}; use std::os::raw::c_char; use libc::{sockaddr, sockaddr_un, sockaddr_vm, socklen_t}; -use std::sync::Arc; -use std::{fmt, mem, ptr}; +use std::boxed::Box; +use std::{mem, ptr}; /// Rust wrapper around ABinderRpc_Accessor objects for RPC binder service management. /// /// Dropping the `Accessor` will drop the underlying object and the binder it owns. +#[derive(Debug)] pub struct Accessor { accessor: *mut sys::ABinderRpc_Accessor, } -impl fmt::Debug for Accessor { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "ABinderRpc_Accessor({:p})", self.accessor) - } -} - /// Socket connection info required for libbinder to connect to a service. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum ConnectionInfo { @@ -70,7 +65,7 @@ impl Accessor { where F: Fn(&str) -> Option<ConnectionInfo> + Send + Sync + 'static, { - let callback: *mut c_void = Arc::into_raw(Arc::new(callback)) as *mut c_void; + let callback: *mut c_void = Box::into_raw(Box::new(callback)) as *mut c_void; let inst = CString::new(instance).unwrap(); // Safety: The function pointer is a valid connection_info callback. @@ -154,7 +149,7 @@ impl Accessor { /// the string within isize::MAX from the pointer. The memory must not be mutated for /// the duration of this function call and must be valid for reads from the pointer /// to the null terminator. - /// - The `cookie` parameter must be the cookie for an `Arc<F>` and + /// - The `cookie` parameter must be the cookie for a `Box<F>` and /// the caller must hold a ref-count to it. unsafe extern "C" fn connection_info<F>( instance: *const c_char, @@ -167,7 +162,7 @@ impl Accessor { log::error!("Cookie({cookie:p}) or instance({instance:p}) is null!"); return ptr::null_mut(); } - // Safety: The caller promises that `cookie` is for an Arc<F>. + // Safety: The caller promises that `cookie` is for a Box<F>. let callback = unsafe { (cookie as *const F).as_ref().unwrap() }; // Safety: The caller in libbinder_ndk will have already verified this is a valid @@ -212,19 +207,19 @@ impl Accessor { } } - /// Callback that decrements the ref-count. + /// Callback that drops the `Box<F>`. /// This is invoked from C++ when a binder is unlinked. /// /// # Safety /// - /// - The `cookie` parameter must be the cookie for an `Arc<F>` and + /// - The `cookie` parameter must be the cookie for a `Box<F>` and /// the owner must give up a ref-count to it. unsafe extern "C" fn cookie_decr_refcount<F>(cookie: *mut c_void) where F: Fn(&str) -> Option<ConnectionInfo> + Send + Sync + 'static, { - // Safety: The caller promises that `cookie` is for an Arc<F>. - unsafe { Arc::decrement_strong_count(cookie as *const F) }; + // Safety: The caller promises that `cookie` is for a Box<F>. + unsafe { std::mem::drop(Box::from_raw(cookie as *mut F)) }; } } @@ -301,7 +296,7 @@ impl AccessorProvider { where F: Fn(&str) -> Option<Accessor> + Send + Sync + 'static, { - let callback: *mut c_void = Arc::into_raw(Arc::new(provider)) as *mut c_void; + let callback: *mut c_void = Box::into_raw(Box::new(provider)) as *mut c_void; let c_str_instances: Vec<CString> = instances.iter().map(|s| CString::new(s.as_bytes()).unwrap()).collect(); let mut c_instances: Vec<*const c_char> = @@ -351,7 +346,7 @@ impl AccessorProvider { log::error!("Cookie({cookie:p}) or instance({instance:p}) is null!"); return ptr::null_mut(); } - // Safety: The caller promises that `cookie` is for an Arc<F>. + // Safety: The caller promises that `cookie` is for a Box<F>. let callback = unsafe { (cookie as *const F).as_ref().unwrap() }; let inst = { @@ -382,14 +377,14 @@ impl AccessorProvider { /// /// # Safety /// - /// - The `cookie` parameter must be the cookie for an `Arc<F>` and + /// - The `cookie` parameter must be the cookie for a `Box<F>` and /// the owner must give up a ref-count to it. unsafe extern "C" fn accessor_cookie_decr_refcount<F>(cookie: *mut c_void) where F: Fn(&str) -> Option<Accessor> + Send + Sync + 'static, { - // Safety: The caller promises that `cookie` is for an Arc<F>. - unsafe { Arc::decrement_strong_count(cookie as *const F) }; + // Safety: The caller promises that `cookie` is for a Box<F>. + unsafe { std::mem::drop(Box::from_raw(cookie as *mut F)) }; } } diff --git a/libs/binder/servicedispatcher.cpp b/libs/binder/servicedispatcher.cpp index be990657d5..78fe2a8714 100644 --- a/libs/binder/servicedispatcher.cpp +++ b/libs/binder/servicedispatcher.cpp @@ -127,7 +127,12 @@ public: // We can't send BpBinder for regular binder over RPC. return android::binder::Status::fromStatusT(android::INVALID_OPERATION); } - android::binder::Status checkService(const std::string&, android::os::Service*) override { + android::binder::Status checkService(const std::string&, + android::sp<android::IBinder>*) override { + // We can't send BpBinder for regular binder over RPC. + return android::binder::Status::fromStatusT(android::INVALID_OPERATION); + } + android::binder::Status checkService2(const std::string&, android::os::Service*) override { // We can't send BpBinder for regular binder over RPC. return android::binder::Status::fromStatusT(android::INVALID_OPERATION); } diff --git a/libs/binder/tests/Android.bp b/libs/binder/tests/Android.bp index c21d7c61cd..f412dfb6f4 100644 --- a/libs/binder/tests/Android.bp +++ b/libs/binder/tests/Android.bp @@ -803,6 +803,28 @@ cc_test { } cc_test { + name: "binderStabilityIntegrationTest", + defaults: ["binder_test_defaults"], + srcs: [ + "binderStabilityIntegrationTest.cpp", + ], + + shared_libs: [ + "libbinder", + "libutils", + ], + static_libs: [ + "libprocpartition", + ], + + test_suites: [ + "general-tests", + "vts", + ], + require_root: true, +} + +cc_test { name: "binderAllocationLimits", defaults: ["binder_test_defaults"], srcs: ["binderAllocationLimits.cpp"], diff --git a/libs/binder/tests/binderCacheUnitTest.cpp b/libs/binder/tests/binderCacheUnitTest.cpp index 19395c2dcd..121e5ae5e9 100644 --- a/libs/binder/tests/binderCacheUnitTest.cpp +++ b/libs/binder/tests/binderCacheUnitTest.cpp @@ -74,7 +74,7 @@ class MockAidlServiceManager : public os::IServiceManagerDefault { public: MockAidlServiceManager() : innerSm() {} - binder::Status checkService(const ::std::string& name, os::Service* _out) override { + binder::Status checkService2(const ::std::string& name, os::Service* _out) override { os::ServiceWithMetadata serviceWithMetadata = os::ServiceWithMetadata(); serviceWithMetadata.service = innerSm.getService(String16(name.c_str())); serviceWithMetadata.isLazyService = false; @@ -98,7 +98,7 @@ class MockAidlServiceManager2 : public os::IServiceManagerDefault { public: MockAidlServiceManager2() : innerSm() {} - binder::Status checkService(const ::std::string& name, os::Service* _out) override { + binder::Status checkService2(const ::std::string& name, os::Service* _out) override { os::ServiceWithMetadata serviceWithMetadata = os::ServiceWithMetadata(); serviceWithMetadata.service = innerSm.getService(String16(name.c_str())); serviceWithMetadata.isLazyService = true; diff --git a/libs/binder/tests/binderLibTest.cpp b/libs/binder/tests/binderLibTest.cpp index d6cb508d7f..391a57010a 100644 --- a/libs/binder/tests/binderLibTest.cpp +++ b/libs/binder/tests/binderLibTest.cpp @@ -44,6 +44,7 @@ #include <processgroup/processgroup.h> #include <utils/Flattenable.h> #include <utils/SystemClock.h> +#include "binder/IServiceManagerUnitTestHelper.h" #include <linux/sched.h> #include <sys/epoll.h> @@ -558,14 +559,14 @@ TEST_F(BinderLibTest, AddManagerToManager) { EXPECT_EQ(NO_ERROR, sm->addService(String16("binderLibTest-manager"), binder)); } +class LocalRegistrationCallbackImpl : public virtual IServiceManager::LocalRegistrationCallback { + void onServiceRegistration(const String16&, const sp<IBinder>&) override {} + virtual ~LocalRegistrationCallbackImpl() {} +}; + TEST_F(BinderLibTest, RegisterForNotificationsFailure) { auto sm = defaultServiceManager(); - using LocalRegistrationCallback = IServiceManager::LocalRegistrationCallback; - class LocalRegistrationCallbackImpl : public virtual LocalRegistrationCallback { - void onServiceRegistration(const String16&, const sp<IBinder>&) override {} - virtual ~LocalRegistrationCallbackImpl() {} - }; - sp<LocalRegistrationCallback> cb = sp<LocalRegistrationCallbackImpl>::make(); + sp<IServiceManager::LocalRegistrationCallback> cb = sp<LocalRegistrationCallbackImpl>::make(); EXPECT_EQ(BAD_VALUE, sm->registerForNotifications(String16("ValidName"), nullptr)); EXPECT_EQ(UNKNOWN_ERROR, sm->registerForNotifications(String16("InvalidName!$"), cb)); @@ -573,12 +574,7 @@ TEST_F(BinderLibTest, RegisterForNotificationsFailure) { TEST_F(BinderLibTest, UnregisterForNotificationsFailure) { auto sm = defaultServiceManager(); - using LocalRegistrationCallback = IServiceManager::LocalRegistrationCallback; - class LocalRegistrationCallbackImpl : public virtual LocalRegistrationCallback { - void onServiceRegistration(const String16&, const sp<IBinder>&) override {} - virtual ~LocalRegistrationCallbackImpl() {} - }; - sp<LocalRegistrationCallback> cb = sp<LocalRegistrationCallbackImpl>::make(); + sp<IServiceManager::LocalRegistrationCallback> cb = sp<LocalRegistrationCallbackImpl>::make(); EXPECT_EQ(OK, sm->registerForNotifications(String16("ValidName"), cb)); @@ -1667,6 +1663,43 @@ TEST(ServiceNotifications, Unregister) { EXPECT_EQ(sm->unregisterForNotifications(String16("RogerRafa"), cb), OK); } +// Make sure all IServiceManager APIs will function without an AIDL service +// manager registered on the device. +TEST(ServiceManagerNoAidlServer, SanityCheck) { + String16 kServiceName("no_services_exist"); + // This is what clients will see when there is no servicemanager process + // that registers itself as context object 0. + // Can't use setDefaultServiceManager() here because these test cases run in + // the same process and will abort when called twice or before/after + // defaultServiceManager(). + sp<IServiceManager> sm = getServiceManagerShimFromAidlServiceManagerForTests(nullptr); + auto status = sm->addService(kServiceName, sp<BBinder>::make()); + // CppBackendShim returns Status::exceptionCode as the status_t + EXPECT_EQ(status, Status::Exception::EX_UNSUPPORTED_OPERATION) << statusToString(status); + auto service = sm->checkService(String16("no_services_exist")); + EXPECT_TRUE(service == nullptr); + auto list = sm->listServices(android::IServiceManager::DUMP_FLAG_PRIORITY_ALL); + EXPECT_TRUE(list.isEmpty()); + bool declared = sm->isDeclared(kServiceName); + EXPECT_FALSE(declared); + list = sm->getDeclaredInstances(kServiceName); + EXPECT_TRUE(list.isEmpty()); + auto updatable = sm->updatableViaApex(kServiceName); + EXPECT_EQ(updatable, std::nullopt); + list = sm->getUpdatableNames(kServiceName); + EXPECT_TRUE(list.isEmpty()); + auto conInfo = sm->getConnectionInfo(kServiceName); + EXPECT_EQ(conInfo, std::nullopt); + auto cb = sp<LocalRegistrationCallbackImpl>::make(); + status = sm->registerForNotifications(kServiceName, cb); + EXPECT_EQ(status, UNKNOWN_ERROR) << statusToString(status); + status = sm->unregisterForNotifications(kServiceName, cb); + EXPECT_EQ(status, BAD_VALUE) << statusToString(status); + auto dbgInfos = sm->getServiceDebugInfo(); + EXPECT_TRUE(dbgInfos.empty()); + sm->enableAddServiceCache(true); +} + TEST_F(BinderLibTest, ThreadPoolAvailableThreads) { Parcel data, reply; sp<IBinder> server = addServer(); diff --git a/libs/binder/tests/binderRpcTest.cpp b/libs/binder/tests/binderRpcTest.cpp index da5a8e3881..9f656ec96c 100644 --- a/libs/binder/tests/binderRpcTest.cpp +++ b/libs/binder/tests/binderRpcTest.cpp @@ -1328,6 +1328,109 @@ TEST_P(BinderRpcAccessor, InjectNoSockaddrProvided) { EXPECT_EQ(status, OK); } +class BinderRpcAccessorNoConnection : public ::testing::Test {}; + +TEST_F(BinderRpcAccessorNoConnection, listServices) { + const String16 kInstanceName("super.cool.service/better_than_default"); + const String16 kInstanceName2("super.cool.service/better_than_default2"); + + auto receipt = + addAccessorProvider({String8(kInstanceName).c_str(), String8(kInstanceName2).c_str()}, + [&](const String16&) -> sp<IBinder> { return nullptr; }); + EXPECT_FALSE(receipt.expired()); + Vector<String16> list = + defaultServiceManager()->listServices(IServiceManager::DUMP_FLAG_PRIORITY_ALL); + bool name1 = false; + bool name2 = false; + for (auto name : list) { + if (name == kInstanceName) name1 = true; + if (name == kInstanceName2) name2 = true; + } + EXPECT_TRUE(name1); + EXPECT_TRUE(name2); + status_t status = removeAccessorProvider(receipt); + EXPECT_EQ(status, OK); +} + +TEST_F(BinderRpcAccessorNoConnection, isDeclared) { + const String16 kInstanceName("super.cool.service/default"); + const String16 kInstanceName2("still_counts_as_declared"); + + auto receipt = + addAccessorProvider({String8(kInstanceName).c_str(), String8(kInstanceName2).c_str()}, + [&](const String16&) -> sp<IBinder> { return nullptr; }); + EXPECT_FALSE(receipt.expired()); + EXPECT_TRUE(defaultServiceManager()->isDeclared(kInstanceName)); + EXPECT_TRUE(defaultServiceManager()->isDeclared(kInstanceName2)); + EXPECT_FALSE(defaultServiceManager()->isDeclared(String16("doesnt_exist"))); + status_t status = removeAccessorProvider(receipt); + EXPECT_EQ(status, OK); +} + +TEST_F(BinderRpcAccessorNoConnection, getDeclaredInstances) { + const String16 kInstanceName("super.cool.service.IFoo/default"); + const String16 kInstanceName2("super.cool.service.IFoo/extra/default"); + const String16 kInstanceName3("super.cool.service.IFoo"); + + auto receipt = + addAccessorProvider({String8(kInstanceName).c_str(), String8(kInstanceName2).c_str(), + String8(kInstanceName3).c_str()}, + [&](const String16&) -> sp<IBinder> { return nullptr; }); + EXPECT_FALSE(receipt.expired()); + Vector<String16> list = + defaultServiceManager()->getDeclaredInstances(String16("super.cool.service.IFoo")); + // We would prefer ASSERT_EQ here, but we must call removeAccessorProvider + EXPECT_EQ(list.size(), 3u); + if (list.size() == 3) { + bool name1 = false; + bool name2 = false; + bool name3 = false; + for (auto name : list) { + if (name == String16("default")) name1 = true; + if (name == String16("extra/default")) name2 = true; + if (name == String16()) name3 = true; + } + EXPECT_TRUE(name1) << String8(list[0]); + EXPECT_TRUE(name2) << String8(list[1]); + EXPECT_TRUE(name3) << String8(list[2]); + } + + status_t status = removeAccessorProvider(receipt); + EXPECT_EQ(status, OK); +} + +TEST_F(BinderRpcAccessorNoConnection, getDeclaredWrongInstances) { + const String16 kInstanceName("super.cool.service.IFoo"); + + auto receipt = addAccessorProvider({String8(kInstanceName).c_str()}, + [&](const String16&) -> sp<IBinder> { return nullptr; }); + EXPECT_FALSE(receipt.expired()); + Vector<String16> list = defaultServiceManager()->getDeclaredInstances(String16("unknown")); + EXPECT_TRUE(list.empty()); + + status_t status = removeAccessorProvider(receipt); + EXPECT_EQ(status, OK); +} + +TEST_F(BinderRpcAccessorNoConnection, getDeclaredInstancesSlash) { + // This is treated as if there were no '/' and the declared instance is "" + const String16 kInstanceName("super.cool.service.IFoo/"); + + auto receipt = addAccessorProvider({String8(kInstanceName).c_str()}, + [&](const String16&) -> sp<IBinder> { return nullptr; }); + EXPECT_FALSE(receipt.expired()); + Vector<String16> list = + defaultServiceManager()->getDeclaredInstances(String16("super.cool.service.IFoo")); + bool name1 = false; + for (auto name : list) { + if (name == String16("")) name1 = true; + } + EXPECT_TRUE(name1); + + status_t status = removeAccessorProvider(receipt); + EXPECT_EQ(status, OK); +} + constexpr const char* kARpcInstance = "some.instance.name.IFoo/default"; const char* kARpcSupportedServices[] = { kARpcInstance, diff --git a/libs/binder/tests/binderRpcUniversalTests.cpp b/libs/binder/tests/binderRpcUniversalTests.cpp index c6fd4870a0..4b9dcf85d2 100644 --- a/libs/binder/tests/binderRpcUniversalTests.cpp +++ b/libs/binder/tests/binderRpcUniversalTests.cpp @@ -498,9 +498,9 @@ TEST_P(BinderRpc, Callbacks) { // same thread, everything should have happened in a nested call. Otherwise, // the callback will be processed on another thread. if (callIsOneway || callbackIsOneway || delayed) { - using std::literals::chrono_literals::operator""s; + using std::literals::chrono_literals::operator""ms; RpcMutexUniqueLock _l(cb->mMutex); - cb->mCv.wait_for(_l, 1s, [&] { return !cb->mValues.empty(); }); + cb->mCv.wait_for(_l, 1500ms, [&] { return !cb->mValues.empty(); }); } EXPECT_EQ(cb->mValues.size(), 1UL) diff --git a/libs/binder/tests/binderSafeInterfaceTest.cpp b/libs/binder/tests/binderSafeInterfaceTest.cpp index 849dc7c4d5..45b2103637 100644 --- a/libs/binder/tests/binderSafeInterfaceTest.cpp +++ b/libs/binder/tests/binderSafeInterfaceTest.cpp @@ -789,7 +789,7 @@ TEST_F(SafeInterfaceTest, TestCallMeBack) { std::optional<int32_t> waitForCallback() { std::unique_lock<decltype(mMutex)> lock(mMutex); bool success = - mCondition.wait_for(lock, 100ms, [&]() { return static_cast<bool>(mValue); }); + mCondition.wait_for(lock, 1000ms, [&]() { return static_cast<bool>(mValue); }); return success ? mValue : std::nullopt; } @@ -858,7 +858,13 @@ TEST_F(SafeInterfaceTest, TestIncrementTwo) { ASSERT_EQ(b + 1, bPlusOne); } -extern "C" int main(int argc, char **argv) { +} // namespace tests +} // namespace android + +int main(int argc, char** argv) { + using namespace android; + using namespace android::tests; + testing::InitGoogleTest(&argc, argv); if (fork() == 0) { @@ -875,6 +881,3 @@ extern "C" int main(int argc, char **argv) { return RUN_ALL_TESTS(); } - -} // namespace tests -} // namespace android diff --git a/libs/binder/tests/binderStabilityIntegrationTest.cpp b/libs/binder/tests/binderStabilityIntegrationTest.cpp new file mode 100644 index 0000000000..a3fc9cc2c8 --- /dev/null +++ b/libs/binder/tests/binderStabilityIntegrationTest.cpp @@ -0,0 +1,89 @@ +/* + * 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. + */ + +#include <binder/Binder.h> +#include <binder/IServiceManager.h> +#include <binder/Stability.h> +#include <gtest/gtest.h> +#include <procpartition/procpartition.h> + +using namespace android; +using android::internal::Stability; // for testing only! +using android::procpartition::getPartition; +using android::procpartition::Partition; + +class BinderStabilityIntegrationTest : public testing::Test, + public ::testing::WithParamInterface<String16> { +public: + virtual ~BinderStabilityIntegrationTest() {} +}; + +TEST_P(BinderStabilityIntegrationTest, ExpectedStabilityForItsPartition) { + const String16& serviceName = GetParam(); + + sp<IBinder> binder = defaultServiceManager()->checkService(serviceName); + if (!binder) GTEST_SKIP() << "Could not get service, may have gone away."; + + pid_t pid; + status_t res = binder->getDebugPid(&pid); + if (res != OK) { + GTEST_SKIP() << "Could not talk to service to get PID, res: " << statusToString(res); + } + + Partition partition = getPartition(pid); + + Stability::Level level = Stability::Level::UNDECLARED; + switch (partition) { + case Partition::SYSTEM: + case Partition::SYSTEM_EXT: + level = Stability::Level::SYSTEM; + break; + case Partition::VENDOR: + case Partition::ODM: + level = Stability::Level::VENDOR; + break; + case Partition::UNKNOWN: + GTEST_SKIP() << "Not sure of partition of process."; + return; + default: + ADD_FAILURE() << "Unrecognized partition for service: " << partition; + return; + } + + ASSERT_TRUE(Stability::check(Stability::getRepr(binder.get()), level)) + << "Binder hosted on partition " << partition + << " should have corresponding stability set."; +} + +std::string PrintTestParam( + const testing::TestParamInfo<BinderStabilityIntegrationTest::ParamType>& info) { + std::string name = String8(info.param).c_str(); + for (size_t i = 0; i < name.size(); i++) { + bool alnum = false; + alnum |= (name[i] >= 'a' && name[i] <= 'z'); + alnum |= (name[i] >= 'A' && name[i] <= 'Z'); + alnum |= (name[i] >= '0' && name[i] <= '9'); + alnum |= (name[i] == '_'); + if (!alnum) name[i] = '_'; + } + + // index for uniqueness + return std::to_string(info.index) + "__" + name; +} + +INSTANTIATE_TEST_CASE_P(RegisteredServices, BinderStabilityIntegrationTest, + ::testing::ValuesIn(defaultServiceManager()->listServices()), + PrintTestParam); diff --git a/libs/binder/tests/parcel_fuzzer/binder.cpp b/libs/binder/tests/parcel_fuzzer/binder.cpp index e378b864f7..20b6b44c62 100644 --- a/libs/binder/tests/parcel_fuzzer/binder.cpp +++ b/libs/binder/tests/parcel_fuzzer/binder.cpp @@ -318,8 +318,6 @@ std::vector<ParcelRead<::android::Parcel>> BINDER_PARCEL_READ_FUNCTIONS { PARCEL_READ_NO_STATUS(int, readParcelFileDescriptor), PARCEL_READ_WITH_STATUS(unique_fd, readUniqueFileDescriptor), - PARCEL_READ_WITH_STATUS(std::unique_ptr<std::vector<unique_fd>>, - readUniqueFileDescriptorVector), PARCEL_READ_WITH_STATUS(std::optional<std::vector<unique_fd>>, readUniqueFileDescriptorVector), PARCEL_READ_WITH_STATUS(std::vector<unique_fd>, readUniqueFileDescriptorVector), diff --git a/libs/binderdebug/stats.cpp b/libs/binderdebug/stats.cpp index 9c26afaa97..972fbd5295 100644 --- a/libs/binderdebug/stats.cpp +++ b/libs/binderdebug/stats.cpp @@ -22,9 +22,9 @@ #include <inttypes.h> -namespace android { +int main() { + using namespace android; -extern "C" int main() { // ignore args - we only print csv // we should use a csv library here for escaping, because @@ -58,5 +58,3 @@ extern "C" int main() { } return 0; } - -} // namespace android diff --git a/libs/binderdebug/tests/binderdebug_test.cpp b/libs/binderdebug/tests/binderdebug_test.cpp index ea799c06a2..ad2b581abc 100644 --- a/libs/binderdebug/tests/binderdebug_test.cpp +++ b/libs/binderdebug/tests/binderdebug_test.cpp @@ -60,8 +60,15 @@ TEST(BinderDebugTests, BinderThreads) { EXPECT_GE(pidInfo.threadCount, 1); } -extern "C" { +} // namespace test +} // namespace binderdebug +} // namespace android + int main(int argc, char** argv) { + using namespace android; + using namespace android::binderdebug; + using namespace android::binderdebug::test; + ::testing::InitGoogleTest(&argc, argv); // Create a child/client process to call into the main process so we can ensure @@ -84,7 +91,3 @@ int main(int argc, char** argv) { return RUN_ALL_TESTS(); } -} // extern "C" -} // namespace test -} // namespace binderdebug -} // namespace android diff --git a/libs/graphicsenv/GraphicsEnv.cpp b/libs/graphicsenv/GraphicsEnv.cpp index a8d5fe7371..4874dbde9c 100644 --- a/libs/graphicsenv/GraphicsEnv.cpp +++ b/libs/graphicsenv/GraphicsEnv.cpp @@ -596,7 +596,7 @@ bool GraphicsEnv::shouldUseAngle() { // If path is set to nonempty and shouldUseNativeDriver is true, ANGLE will be used regardless. void GraphicsEnv::setAngleInfo(const std::string& path, const bool shouldUseNativeDriver, const std::string& packageName, - const std::vector<std::string> eglFeatures) { + const std::vector<std::string>& eglFeatures) { if (mShouldUseAngle) { // ANGLE is already set up for this application process, even if the application // needs to switch from apk to system or vice versa, the application process must @@ -606,11 +606,11 @@ void GraphicsEnv::setAngleInfo(const std::string& path, const bool shouldUseNati return; } - mAngleEglFeatures = std::move(eglFeatures); + mAngleEglFeatures = eglFeatures; ALOGV("setting ANGLE path to '%s'", path.c_str()); - mAnglePath = std::move(path); + mAnglePath = path; ALOGV("setting app package name to '%s'", packageName.c_str()); - mPackageName = std::move(packageName); + mPackageName = packageName; if (mAnglePath == "system") { mShouldUseSystemAngle = true; } diff --git a/libs/graphicsenv/include/graphicsenv/GraphicsEnv.h b/libs/graphicsenv/include/graphicsenv/GraphicsEnv.h index b0ab0b9d22..452e48bb75 100644 --- a/libs/graphicsenv/include/graphicsenv/GraphicsEnv.h +++ b/libs/graphicsenv/include/graphicsenv/GraphicsEnv.h @@ -114,7 +114,7 @@ public: // If shouldUseNativeDriver is true, it means native GLES drivers must be used for the process. // If path is set to nonempty and shouldUseNativeDriver is true, ANGLE will be used regardless. void setAngleInfo(const std::string& path, const bool shouldUseNativeDriver, - const std::string& packageName, const std::vector<std::string> eglFeatures); + const std::string& packageName, const std::vector<std::string>& eglFeatures); // Get the ANGLE driver namespace. android_namespace_t* getAngleNamespace(); // Get the app package name. diff --git a/libs/input/rust/keyboard_classifier.rs b/libs/input/rust/keyboard_classifier.rs index 3c789b41e2..1b89a5cf2b 100644 --- a/libs/input/rust/keyboard_classifier.rs +++ b/libs/input/rust/keyboard_classifier.rs @@ -66,11 +66,11 @@ impl KeyboardClassifier { /// Get keyboard type for a tracked keyboard in KeyboardClassifier pub fn get_keyboard_type(&self, device_id: DeviceId) -> KeyboardType { - return if let Some(keyboard) = self.device_map.get(&device_id) { + if let Some(keyboard) = self.device_map.get(&device_id) { keyboard.keyboard_type } else { KeyboardType::None - }; + } } /// Tells if keyboard type classification is finalized. Once finalized the classification can't @@ -79,11 +79,11 @@ impl KeyboardClassifier { /// Finalized devices are either "alphabetic" keyboards or keyboards in blocklist or /// allowlist that are explicitly categorized and won't change with future key events pub fn is_finalized(&self, device_id: DeviceId) -> bool { - return if let Some(keyboard) = self.device_map.get(&device_id) { + if let Some(keyboard) = self.device_map.get(&device_id) { keyboard.is_finalized } else { false - }; + } } /// Process a key event and change keyboard type if required. diff --git a/libs/nativewindow/rust/src/lib.rs b/libs/nativewindow/rust/src/lib.rs index d760285918..aeb26031a5 100644 --- a/libs/nativewindow/rust/src/lib.rs +++ b/libs/nativewindow/rust/src/lib.rs @@ -541,7 +541,7 @@ pub struct HardwareBufferGuard<'a> { pub address: NonNull<c_void>, } -impl<'a> Drop for HardwareBufferGuard<'a> { +impl Drop for HardwareBufferGuard<'_> { fn drop(&mut self) { self.buffer .unlock() diff --git a/services/inputflinger/dispatcher/InputDispatcher.cpp b/services/inputflinger/dispatcher/InputDispatcher.cpp index 5db21fdfa5..56ab0dc6ec 100644 --- a/services/inputflinger/dispatcher/InputDispatcher.cpp +++ b/services/inputflinger/dispatcher/InputDispatcher.cpp @@ -4899,6 +4899,19 @@ InputEventInjectionResult InputDispatcher::injectInputEvent(const InputEvent* ev } } + if (!(policyFlags & POLICY_FLAG_PASS_TO_USER)) { + // Set the flag anyway if we already have an ongoing motion gesture. That + // would allow us to complete the processing of the current stroke. + const auto touchStateIt = mTouchStatesByDisplay.find(displayId); + if (touchStateIt != mTouchStatesByDisplay.end()) { + const TouchState& touchState = touchStateIt->second; + if (touchState.hasTouchingPointers(resolvedDeviceId) || + touchState.hasHoveringPointers(resolvedDeviceId)) { + policyFlags |= POLICY_FLAG_PASS_TO_USER; + } + } + } + const nsecs_t* sampleEventTimes = motionEvent.getSampleEventTimes(); const size_t pointerCount = motionEvent.getPointerCount(); const std::vector<PointerProperties> diff --git a/services/inputflinger/tests/InputDispatcher_test.cpp b/services/inputflinger/tests/InputDispatcher_test.cpp index 7b5c47b1ac..41131fc49b 100644 --- a/services/inputflinger/tests/InputDispatcher_test.cpp +++ b/services/inputflinger/tests/InputDispatcher_test.cpp @@ -1571,6 +1571,60 @@ TEST_F(InputDispatcherTest, HoverEventInconsistentPolicy) { window->consumeMotionEvent(WithMotionAction(ACTION_HOVER_EXIT)); } +// Still send inject motion events to window which already be touched. +TEST_F(InputDispatcherTest, AlwaysDispatchInjectMotionEventWhenAlreadyDownForWindow) { + std::shared_ptr<FakeApplicationHandle> application1 = std::make_shared<FakeApplicationHandle>(); + sp<FakeWindowHandle> window1 = + sp<FakeWindowHandle>::make(application1, mDispatcher, "window1", + ui::LogicalDisplayId::DEFAULT); + window1->setFrame(Rect(0, 0, 100, 100)); + window1->setWatchOutsideTouch(false); + + std::shared_ptr<FakeApplicationHandle> application2 = std::make_shared<FakeApplicationHandle>(); + sp<FakeWindowHandle> window2 = + sp<FakeWindowHandle>::make(application2, mDispatcher, "window2", + ui::LogicalDisplayId::DEFAULT); + window2->setFrame(Rect(50, 50, 100, 100)); + window2->setWatchOutsideTouch(true); + mDispatcher->onWindowInfosChanged({{*window2->getInfo(), *window1->getInfo()}, {}, 0, 0}); + + std::chrono::milliseconds injectionTimeout = INJECT_EVENT_TIMEOUT; + InputEventInjectionSync injectionMode = InputEventInjectionSync::WAIT_FOR_RESULT; + std::optional<gui::Uid> targetUid = {}; + uint32_t policyFlags = DEFAULT_POLICY_FLAGS; + + const MotionEvent eventDown1 = MotionEventBuilder(ACTION_DOWN, AINPUT_SOURCE_TOUCHSCREEN) + .pointer(PointerBuilder(0, ToolType::FINGER).x(60).y(60)).deviceId(-1) + .build(); + injectMotionEvent(*mDispatcher, eventDown1, injectionTimeout, injectionMode, targetUid, + policyFlags); + window2->consumeMotionEvent(WithMotionAction(ACTION_DOWN)); + + const MotionEvent eventUp1 = MotionEventBuilder(ACTION_UP, AINPUT_SOURCE_TOUCHSCREEN) + .pointer(PointerBuilder(0, ToolType::FINGER).x(60).y(60)).deviceId(-1) + .downTime(eventDown1.getDownTime()).build(); + // Inject UP event, without the POLICY_FLAG_PASS_TO_USER (to simulate policy behaviour + // when screen is off). + injectMotionEvent(*mDispatcher, eventUp1, injectionTimeout, injectionMode, targetUid, + /*policyFlags=*/0); + window2->consumeMotionEvent(WithMotionAction(ACTION_UP)); + const MotionEvent eventDown2 = MotionEventBuilder(ACTION_DOWN, AINPUT_SOURCE_TOUCHSCREEN) + .pointer(PointerBuilder(0, ToolType::FINGER).x(40).y(40)).deviceId(-1) + .build(); + injectMotionEvent(*mDispatcher, eventDown2, injectionTimeout, injectionMode, targetUid, + policyFlags); + window1->consumeMotionEvent(WithMotionAction(ACTION_DOWN)); + window2->consumeMotionEvent(WithMotionAction(ACTION_OUTSIDE)); + + const MotionEvent eventUp2 = MotionEventBuilder(ACTION_UP, AINPUT_SOURCE_TOUCHSCREEN) + .pointer(PointerBuilder(0, ToolType::FINGER).x(60).y(60)).deviceId(-1) + .downTime(eventDown2.getDownTime()).build(); + injectMotionEvent(*mDispatcher, eventUp2, injectionTimeout, injectionMode, targetUid, + /*policyFlags=*/0); + window1->consumeMotionEvent(WithMotionAction(ACTION_UP)); + window2->assertNoEvents(); +} + /** * Two windows: a window on the left and a window on the right. * Mouse is hovered from the right window into the left window. diff --git a/services/surfaceflinger/Client.cpp b/services/surfaceflinger/Client.cpp index abeb2a92eb..77bf1457c3 100644 --- a/services/surfaceflinger/Client.cpp +++ b/services/surfaceflinger/Client.cpp @@ -53,7 +53,6 @@ binder::Status Client::createSurface(const std::string& name, int32_t flags, const sp<IBinder>& parent, const gui::LayerMetadata& metadata, gui::CreateSurfaceResult* outResult) { // We rely on createLayer to check permissions. - sp<IBinder> handle; LayerCreationArgs args(mFlinger.get(), sp<Client>::fromExisting(this), name.c_str(), static_cast<uint32_t>(flags), std::move(metadata)); args.parentHandle = parent; @@ -101,7 +100,6 @@ binder::Status Client::getLayerFrameStats(const sp<IBinder>& handle, gui::FrameS binder::Status Client::mirrorSurface(const sp<IBinder>& mirrorFromHandle, gui::CreateSurfaceResult* outResult) { - sp<IBinder> handle; LayerCreationArgs args(mFlinger.get(), sp<Client>::fromExisting(this), "MirrorRoot", 0 /* flags */, gui::LayerMetadata()); status_t status = mFlinger->mirrorLayer(args, mirrorFromHandle, *outResult); @@ -109,7 +107,6 @@ binder::Status Client::mirrorSurface(const sp<IBinder>& mirrorFromHandle, } binder::Status Client::mirrorDisplay(int64_t displayId, gui::CreateSurfaceResult* outResult) { - sp<IBinder> handle; LayerCreationArgs args(mFlinger.get(), sp<Client>::fromExisting(this), "MirrorRoot-" + std::to_string(displayId), 0 /* flags */, gui::LayerMetadata()); |