From 5e68cb5f4b3b9e74ddd2b6bf240be7f3bc862378 Mon Sep 17 00:00:00 2001 From: JingMatrix Date: Wed, 4 Feb 2026 09:03:51 +0100 Subject: [PATCH] Resolve reference leak and warnings in binder interception (#122) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This merge addresses a critical strong reference leak in the ioctl hook that occurred during binder transaction interception. The leak was caused by a double increment of the reference count—once manually and once by a smart pointer's constructor—with only a single corresponding decrement. The fix ensures a balanced increment and decrement, preventing the leak and subsequent crashes. Additionally, this change: - Reverts a now-unnecessary compatibility layer for the Android 11 RefBase ABI. - Implements `getInterfaceDescriptor` in the `BinderStub` to silence framework warnings that appeared after the primary leak was fixed. --- app/src/main/cpp/CMakeLists.txt | 4 +- app/src/main/cpp/binder_interceptor.cpp | 21 ++++--- app/src/main/cpp/compat/refbase_compat.cpp | 61 ------------------- app/src/main/cpp/compat/refbase_compat.h | 11 ---- .../AOSP/include/utils/StrongPointer.h | 3 +- 5 files changed, 16 insertions(+), 84 deletions(-) delete mode 100644 app/src/main/cpp/compat/refbase_compat.cpp delete mode 100644 app/src/main/cpp/compat/refbase_compat.h diff --git a/app/src/main/cpp/CMakeLists.txt b/app/src/main/cpp/CMakeLists.txt index 209fb1c..30cd7f3 100644 --- a/app/src/main/cpp/CMakeLists.txt +++ b/app/src/main/cpp/CMakeLists.txt @@ -12,7 +12,7 @@ add_subdirectory(external/LSPlt/lsplt/src/main/jni) add_compile_definitions(BINDER_DISABLE_NATIVE_HANDLE) add_library(utils SHARED stub/stub_utils.cpp) -target_include_directories(utils PUBLIC external/AOSP/include compat) +target_include_directories(utils PUBLIC external/AOSP/include) add_library(binder SHARED stub/stub_binder.cpp) target_include_directories(binder PUBLIC external/AOSP/include) @@ -22,7 +22,7 @@ add_executable(libinject.so inject/main.cpp inject/utils.cpp) target_include_directories(libinject.so PUBLIC include) target_link_libraries(libinject.so PRIVATE lsplt_static) -add_library(${CMAKE_PROJECT_NAME} SHARED binder_interceptor.cpp compat/refbase_compat.cpp) +add_library(${CMAKE_PROJECT_NAME} SHARED binder_interceptor.cpp) target_include_directories(${CMAKE_PROJECT_NAME} PUBLIC external/linux-kernel/include include) target_link_libraries(${CMAKE_PROJECT_NAME} PRIVATE binder lsplt_static utils) diff --git a/app/src/main/cpp/binder_interceptor.cpp b/app/src/main/cpp/binder_interceptor.cpp index 4a337b9..bc2b2b5 100644 --- a/app/src/main/cpp/binder_interceptor.cpp +++ b/app/src/main/cpp/binder_interceptor.cpp @@ -276,6 +276,12 @@ static sp g_interceptor_instance = nullptr; // ============================================================================================= class BinderStub : public BBinder { +public: + const String16& getInterfaceDescriptor() const override { + static const String16 kDescriptor("org.matrix.TEESimulator.BinderStub"); + return kDescriptor; + } + protected: status_t onTransact(uint32_t code, const Parcel &data, Parcel *reply, uint32_t flags) override { if (code != intercept::kBackdoorCode) { @@ -376,18 +382,17 @@ void inspectAndRewriteTransaction(binder_transaction_data *txn_data) { // The raw pointer to the binder object itself is stored in the cookie BBinder *target_binder_ptr = reinterpret_cast(txn_data->cookie); - // This is safe ONLY because we successfully called attemptIncStrong(). - // The sp<> constructor will not increment the ref count again, it just adopts the one we have. - // When sp_target goes out of scope, it will call decStrong(), releasing our temporary reference. - sp sp_target = sp::fromExisting(target_binder_ptr); + // Create a weak pointer for the lookup and to store in our context map. + // This is safe because we are holding a strong reference. + wp wp_target = target_binder_ptr; - // Now we can safely use sp_target (which implicitly converts to a wp) for the lookup. - if (g_interceptor_instance->isBinderIntercepted(sp_target)) { + if (g_interceptor_instance->isBinderIntercepted(wp_target)) { info.transaction_code = txn_data->code; - info.target_binder = sp_target; // Assign the valid weak pointer + info.target_binder = wp_target; // Assign the valid weak pointer hijack = true; } - // No need to manually call decStrong(); the sp destructor handles it. + // Manually release the temporary strong reference we acquired at the start. + target_binder_ptr->decStrong(nullptr); } } diff --git a/app/src/main/cpp/compat/refbase_compat.cpp b/app/src/main/cpp/compat/refbase_compat.cpp deleted file mode 100644 index 4f7c18d..0000000 --- a/app/src/main/cpp/compat/refbase_compat.cpp +++ /dev/null @@ -1,61 +0,0 @@ -#include "refbase_compat.h" -#include "utils/RefBase.h" -#include -#include -#include // For memcpy -#include -#include -#include - -namespace android { - -// Helper function to get the Android API level at runtime. -// It caches the result for performance. -int32_t get_android_api_level() { - static std::atomic api_level = -1; - if (api_level.load(std::memory_order_relaxed) == -1) { - char sdk_version_str[PROP_VALUE_MAX]; - if (__system_property_get("ro.build.version.sdk", sdk_version_str) > 0) { - api_level.store(atoi(sdk_version_str), std::memory_order_relaxed); - } - } - return api_level.load(std::memory_order_relaxed); -} - -// Define the function pointer type for the const member function -// RefBase::incStrongRequireStrong. -using incStrongRequireStrong_t = void (RefBase::*)(const void *) const; - -// This is the implementation of our compatibility wrapper. -void incStrongFromExisting(const RefBase *ref, const void *id) { - // Only attempt to use the new function on Android 12 (API 31) or higher. - if (get_android_api_level() >= 31) { - static incStrongRequireStrong_t sIncStrongRequireStrong = nullptr; - static std::once_flag sFlag; - - // Thread-safe, one-time initialization. - std::call_once(sFlag, []() { - // Find the symbol in the already loaded libraries. - // The mangled symbol is _ZNK7android7RefBase22incStrongRequireStrongEPKv - void *sym = dlsym(RTLD_DEFAULT, - "_ZNK7android7RefBase22incStrongRequireStrongEPKv"); - if (sym) { - // Safely cast the void* symbol to our member function pointer. - memcpy(&sIncStrongRequireStrong, &sym, sizeof(void *)); - } - }); - - if (sIncStrongRequireStrong) { - // If the symbol was found, call it as member function. - (ref->*sIncStrongRequireStrong)(id); - return; // Success, we are done. - } - // If dlsym failed for any reason, we fall through to the old method. - } - - // Fallback for older Android versions or if dlsym failed. - // This calls the universally available incStrong method. - ref->incStrong(id); -} - -} // namespace android diff --git a/app/src/main/cpp/compat/refbase_compat.h b/app/src/main/cpp/compat/refbase_compat.h deleted file mode 100644 index 20aee7e..0000000 --- a/app/src/main/cpp/compat/refbase_compat.h +++ /dev/null @@ -1,11 +0,0 @@ -#pragma once - -namespace android { - -// Forward-declare the RefBase class. -class RefBase; - -// Declares our compatibility function. -void incStrongFromExisting(const RefBase *ref, const void *id); - -} // namespace android diff --git a/app/src/main/cpp/external/AOSP/include/utils/StrongPointer.h b/app/src/main/cpp/external/AOSP/include/utils/StrongPointer.h index c7a65a6..fb9b8e8 100644 --- a/app/src/main/cpp/external/AOSP/include/utils/StrongPointer.h +++ b/app/src/main/cpp/external/AOSP/include/utils/StrongPointer.h @@ -17,7 +17,6 @@ #ifndef ANDROID_STRONG_POINTER_H #define ANDROID_STRONG_POINTER_H -#include "refbase_compat.h" #include #include // for common_type. @@ -213,7 +212,7 @@ sp sp::make(Args&&... args) { template sp sp::fromExisting(T* other) { if (other) { - incStrongFromExisting(other, other); + other->incStrongRequireStrong(other); sp result; result.m_ptr = other; return result;