From 9ebf233e8d1c6a6be6e75c71c5be0a3531008b94 Mon Sep 17 00:00:00 2001 From: Luke Huang Date: Sat, 20 Jun 2020 13:01:33 +0800 Subject: Backport: Update the documentation of ANDROID_RESOLV_NO_CACHE_STORE Make it up-to-date. Test: N/A Bug: 150371903 Change-Id: Ia1402a18d6d466ffbb0357127d7d45cf6c722550 (cherry picked from commit f1cf6a632da354cff3d8aed54913e1ee2909908e) --- include/android/multinetwork.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/android/multinetwork.h b/include/android/multinetwork.h index d31d1f122f..f7294a9ea5 100644 --- a/include/android/multinetwork.h +++ b/include/android/multinetwork.h @@ -123,8 +123,8 @@ enum ResNsendFlags : uint32_t { ANDROID_RESOLV_NO_RETRY = 1 << 0, /** - * Do not cache the result of the lookup. The lookup may return a result that is already - * in the cache, unless the ANDROID_RESOLV_NO_CACHE_LOOKUP flag is also specified. + * Don't lookup this request in the cache, and don't cache the result of the lookup. + * This flag implies {@link #ANDROID_RESOLV_NO_CACHE_LOOKUP}. */ ANDROID_RESOLV_NO_CACHE_STORE = 1 << 1, -- cgit v1.2.3 From 09052cc6655ab2756c4c484b958cdaf29325e652 Mon Sep 17 00:00:00 2001 From: Arthur Ishiguro Date: Tue, 22 Sep 2020 13:05:15 -0700 Subject: Prevent mEventCache UAF in SensorEventConnection Since there is no check to see if SensorEventConnection has been destroyed, the mEventCache pointer can still be used even after it was freed. Bug: 168211968 Test: Run test code that attempts to enable a sensor after destroying the SensorEventConnection, and verify no system_server crash occurs. Change-Id: Ia9275b7cc574df371cdb2e1b80c6699df193b580 Merged-In: Ia9275b7cc574df371cdb2e1b80c6699df193b580 --- services/sensorservice/SensorEventConnection.cpp | 28 +++++++++++++++--------- services/sensorservice/SensorEventConnection.h | 5 +++-- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/services/sensorservice/SensorEventConnection.cpp b/services/sensorservice/SensorEventConnection.cpp index 0a05dd1b18..15ca7ac3cb 100644 --- a/services/sensorservice/SensorEventConnection.cpp +++ b/services/sensorservice/SensorEventConnection.cpp @@ -14,6 +14,7 @@ * limitations under the License. */ +#include #include #include @@ -44,20 +45,13 @@ SensorService::SensorEventConnection::SensorEventConnection( SensorService::SensorEventConnection::~SensorEventConnection() { ALOGD_IF(DEBUG_CONNECTIONS, "~SensorEventConnection(%p)", this); destroy(); -} - -void SensorService::SensorEventConnection::destroy() { - Mutex::Autolock _l(mDestroyLock); - - // destroy once only - if (mDestroyed) { - return; - } - mService->cleanupConnection(this); if (mEventCache != NULL) { delete mEventCache; } +} + +void SensorService::SensorEventConnection::destroy() { mDestroyed = true; } @@ -525,6 +519,11 @@ status_t SensorService::SensorEventConnection::enableDisable( int handle, bool enabled, nsecs_t samplingPeriodNs, nsecs_t maxBatchReportLatencyNs, int reservedFlags) { + if (mDestroyed) { + android_errorWriteLog(0x534e4554, "168211968"); + return DEAD_OBJECT; + } + status_t err; if (enabled) { err = mService->enable(this, handle, samplingPeriodNs, maxBatchReportLatencyNs, @@ -539,10 +538,19 @@ status_t SensorService::SensorEventConnection::enableDisable( status_t SensorService::SensorEventConnection::setEventRate( int handle, nsecs_t samplingPeriodNs) { + if (mDestroyed) { + android_errorWriteLog(0x534e4554, "168211968"); + return DEAD_OBJECT; + } + return mService->setEventRate(this, handle, samplingPeriodNs, mOpPackageName); } status_t SensorService::SensorEventConnection::flush() { + if (mDestroyed) { + return DEAD_OBJECT; + } + return mService->flushSensor(this, mOpPackageName); } diff --git a/services/sensorservice/SensorEventConnection.h b/services/sensorservice/SensorEventConnection.h index 6f282cdc60..bd1b3bbef7 100644 --- a/services/sensorservice/SensorEventConnection.h +++ b/services/sensorservice/SensorEventConnection.h @@ -17,6 +17,7 @@ #ifndef ANDROID_SENSOR_EVENT_CONNECTION_H #define ANDROID_SENSOR_EVENT_CONNECTION_H +#include #include #include @@ -165,8 +166,8 @@ private: int mTotalAcksNeeded, mTotalAcksReceived; #endif - mutable Mutex mDestroyLock; - bool mDestroyed; + // Used to track if this object was inappropriately used after destroy(). + std::atomic_bool mDestroyed; }; } // namepsace android -- cgit v1.2.3 From 3b27b3b3bb36274171a9abb31f3e3f6b53036cf0 Mon Sep 17 00:00:00 2001 From: Arthur Ishiguro Date: Tue, 22 Sep 2020 13:05:15 -0700 Subject: Prevent mEventCache UAF in SensorEventConnection Since there is no check to see if SensorEventConnection has been destroyed, the mEventCache pointer can still be used even after it was freed. Bug: 168211968 Test: Run test code that attempts to enable a sensor after destroying the SensorEventConnection, and verify no system_server crash occurs. Change-Id: Ia9275b7cc574df371cdb2e1b80c6699df193b580 Merged-In: Ia9275b7cc574df371cdb2e1b80c6699df193b580 --- services/sensorservice/SensorEventConnection.cpp | 28 +++++++++++++++--------- services/sensorservice/SensorEventConnection.h | 6 +++-- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/services/sensorservice/SensorEventConnection.cpp b/services/sensorservice/SensorEventConnection.cpp index 956844f426..a9e8321dcf 100644 --- a/services/sensorservice/SensorEventConnection.cpp +++ b/services/sensorservice/SensorEventConnection.cpp @@ -14,6 +14,7 @@ * limitations under the License. */ +#include #include #include @@ -44,20 +45,13 @@ SensorService::SensorEventConnection::SensorEventConnection( SensorService::SensorEventConnection::~SensorEventConnection() { ALOGD_IF(DEBUG_CONNECTIONS, "~SensorEventConnection(%p)", this); destroy(); -} - -void SensorService::SensorEventConnection::destroy() { - Mutex::Autolock _l(mDestroyLock); - - // destroy once only - if (mDestroyed) { - return; - } - mService->cleanupConnection(this); if (mEventCache != NULL) { delete mEventCache; } +} + +void SensorService::SensorEventConnection::destroy() { mDestroyed = true; } @@ -555,6 +549,11 @@ status_t SensorService::SensorEventConnection::enableDisable( int handle, bool enabled, nsecs_t samplingPeriodNs, nsecs_t maxBatchReportLatencyNs, int reservedFlags) { + if (mDestroyed) { + android_errorWriteLog(0x534e4554, "168211968"); + return DEAD_OBJECT; + } + status_t err; if (enabled) { err = mService->enable(this, handle, samplingPeriodNs, maxBatchReportLatencyNs, @@ -569,10 +568,19 @@ status_t SensorService::SensorEventConnection::enableDisable( status_t SensorService::SensorEventConnection::setEventRate( int handle, nsecs_t samplingPeriodNs) { + if (mDestroyed) { + android_errorWriteLog(0x534e4554, "168211968"); + return DEAD_OBJECT; + } + return mService->setEventRate(this, handle, samplingPeriodNs, mOpPackageName); } status_t SensorService::SensorEventConnection::flush() { + if (mDestroyed) { + return DEAD_OBJECT; + } + return mService->flushSensor(this, mOpPackageName); } diff --git a/services/sensorservice/SensorEventConnection.h b/services/sensorservice/SensorEventConnection.h index 032721ea39..84b4d86879 100644 --- a/services/sensorservice/SensorEventConnection.h +++ b/services/sensorservice/SensorEventConnection.h @@ -17,6 +17,7 @@ #ifndef ANDROID_SENSOR_EVENT_CONNECTION_H #define ANDROID_SENSOR_EVENT_CONNECTION_H +#include #include #include @@ -168,8 +169,9 @@ private: int mTotalAcksNeeded, mTotalAcksReceived; #endif - mutable Mutex mDestroyLock; - bool mDestroyed; + // Used to track if this object was inappropriately used after destroy(). + std::atomic_bool mDestroyed; + bool mHasSensorAccess; }; -- cgit v1.2.3 From bbf3b377482aeac64c23556bcb2a129d944629ed Mon Sep 17 00:00:00 2001 From: Arthur Ishiguro Date: Tue, 22 Sep 2020 13:05:15 -0700 Subject: Prevent mEventCache UAF in SensorEventConnection Since there is no check to see if SensorEventConnection has been destroyed, the mEventCache pointer can still be used even after it was freed. Bug: 168211968 Test: Run test code that attempts to enable a sensor after destroying the SensorEventConnection, and verify no system_server crash occurs. Change-Id: Ia9275b7cc574df371cdb2e1b80c6699df193b580 Merged-In: Ia9275b7cc574df371cdb2e1b80c6699df193b580 --- services/sensorservice/SensorEventConnection.cpp | 28 +++++++++++++++--------- services/sensorservice/SensorEventConnection.h | 6 +++-- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/services/sensorservice/SensorEventConnection.cpp b/services/sensorservice/SensorEventConnection.cpp index 0e409409f2..1811716af9 100644 --- a/services/sensorservice/SensorEventConnection.cpp +++ b/services/sensorservice/SensorEventConnection.cpp @@ -14,6 +14,7 @@ * limitations under the License. */ +#include #include #include @@ -45,20 +46,13 @@ SensorService::SensorEventConnection::SensorEventConnection( SensorService::SensorEventConnection::~SensorEventConnection() { ALOGD_IF(DEBUG_CONNECTIONS, "~SensorEventConnection(%p)", this); destroy(); -} - -void SensorService::SensorEventConnection::destroy() { - Mutex::Autolock _l(mDestroyLock); - - // destroy once only - if (mDestroyed) { - return; - } - mService->cleanupConnection(this); if (mEventCache != nullptr) { delete[] mEventCache; } +} + +void SensorService::SensorEventConnection::destroy() { mDestroyed = true; } @@ -603,6 +597,11 @@ status_t SensorService::SensorEventConnection::enableDisable( int handle, bool enabled, nsecs_t samplingPeriodNs, nsecs_t maxBatchReportLatencyNs, int reservedFlags) { + if (mDestroyed) { + android_errorWriteLog(0x534e4554, "168211968"); + return DEAD_OBJECT; + } + status_t err; if (enabled) { err = mService->enable(this, handle, samplingPeriodNs, maxBatchReportLatencyNs, @@ -617,10 +616,19 @@ status_t SensorService::SensorEventConnection::enableDisable( status_t SensorService::SensorEventConnection::setEventRate( int handle, nsecs_t samplingPeriodNs) { + if (mDestroyed) { + android_errorWriteLog(0x534e4554, "168211968"); + return DEAD_OBJECT; + } + return mService->setEventRate(this, handle, samplingPeriodNs, mOpPackageName); } status_t SensorService::SensorEventConnection::flush() { + if (mDestroyed) { + return DEAD_OBJECT; + } + return mService->flushSensor(this, mOpPackageName); } diff --git a/services/sensorservice/SensorEventConnection.h b/services/sensorservice/SensorEventConnection.h index fd881cbc0b..d3528551bb 100644 --- a/services/sensorservice/SensorEventConnection.h +++ b/services/sensorservice/SensorEventConnection.h @@ -17,6 +17,7 @@ #ifndef ANDROID_SENSOR_EVENT_CONNECTION_H #define ANDROID_SENSOR_EVENT_CONNECTION_H +#include #include #include #include @@ -182,8 +183,9 @@ private: int mTotalAcksNeeded, mTotalAcksReceived; #endif - mutable Mutex mDestroyLock; - bool mDestroyed; + // Used to track if this object was inappropriately used after destroy(). + std::atomic_bool mDestroyed; + bool mHasSensorAccess; // Store a mapping of sensor handles to required AppOp for a sensor. This map only contains a -- cgit v1.2.3 From 9fb3ea9c93b98a77e7899299019542e333da21fb Mon Sep 17 00:00:00 2001 From: Arthur Ishiguro Date: Tue, 22 Sep 2020 13:05:15 -0700 Subject: Prevent mEventCache UAF in SensorEventConnection Since there is no check to see if SensorEventConnection has been destroyed, the mEventCache pointer can still be used even after it was freed. Bug: 168211968 Test: Run test code that attempts to enable a sensor after destroying the SensorEventConnection, and verify no system_server crash occurs. Change-Id: Ia9275b7cc574df371cdb2e1b80c6699df193b580 Merged-In: Ia9275b7cc574df371cdb2e1b80c6699df193b580 --- services/sensorservice/SensorEventConnection.cpp | 28 +++++++++++++++--------- services/sensorservice/SensorEventConnection.h | 6 +++-- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/services/sensorservice/SensorEventConnection.cpp b/services/sensorservice/SensorEventConnection.cpp index 0e409409f2..1811716af9 100644 --- a/services/sensorservice/SensorEventConnection.cpp +++ b/services/sensorservice/SensorEventConnection.cpp @@ -14,6 +14,7 @@ * limitations under the License. */ +#include #include #include @@ -45,20 +46,13 @@ SensorService::SensorEventConnection::SensorEventConnection( SensorService::SensorEventConnection::~SensorEventConnection() { ALOGD_IF(DEBUG_CONNECTIONS, "~SensorEventConnection(%p)", this); destroy(); -} - -void SensorService::SensorEventConnection::destroy() { - Mutex::Autolock _l(mDestroyLock); - - // destroy once only - if (mDestroyed) { - return; - } - mService->cleanupConnection(this); if (mEventCache != nullptr) { delete[] mEventCache; } +} + +void SensorService::SensorEventConnection::destroy() { mDestroyed = true; } @@ -603,6 +597,11 @@ status_t SensorService::SensorEventConnection::enableDisable( int handle, bool enabled, nsecs_t samplingPeriodNs, nsecs_t maxBatchReportLatencyNs, int reservedFlags) { + if (mDestroyed) { + android_errorWriteLog(0x534e4554, "168211968"); + return DEAD_OBJECT; + } + status_t err; if (enabled) { err = mService->enable(this, handle, samplingPeriodNs, maxBatchReportLatencyNs, @@ -617,10 +616,19 @@ status_t SensorService::SensorEventConnection::enableDisable( status_t SensorService::SensorEventConnection::setEventRate( int handle, nsecs_t samplingPeriodNs) { + if (mDestroyed) { + android_errorWriteLog(0x534e4554, "168211968"); + return DEAD_OBJECT; + } + return mService->setEventRate(this, handle, samplingPeriodNs, mOpPackageName); } status_t SensorService::SensorEventConnection::flush() { + if (mDestroyed) { + return DEAD_OBJECT; + } + return mService->flushSensor(this, mOpPackageName); } diff --git a/services/sensorservice/SensorEventConnection.h b/services/sensorservice/SensorEventConnection.h index fd881cbc0b..d3528551bb 100644 --- a/services/sensorservice/SensorEventConnection.h +++ b/services/sensorservice/SensorEventConnection.h @@ -17,6 +17,7 @@ #ifndef ANDROID_SENSOR_EVENT_CONNECTION_H #define ANDROID_SENSOR_EVENT_CONNECTION_H +#include #include #include #include @@ -182,8 +183,9 @@ private: int mTotalAcksNeeded, mTotalAcksReceived; #endif - mutable Mutex mDestroyLock; - bool mDestroyed; + // Used to track if this object was inappropriately used after destroy(). + std::atomic_bool mDestroyed; + bool mHasSensorAccess; // Store a mapping of sensor handles to required AppOp for a sensor. This map only contains a -- cgit v1.2.3 From f1bf7dd095ac2f632442663cb16aeef4691b93e7 Mon Sep 17 00:00:00 2001 From: Arthur Ishiguro Date: Tue, 22 Sep 2020 13:05:15 -0700 Subject: Prevent mEventCache UAF in SensorEventConnection Since there is no check to see if SensorEventConnection has been destroyed, the mEventCache pointer can still be used even after it was freed. Bug: 168211968 Test: Run test code that attempts to enable a sensor after destroying the SensorEventConnection, and verify no system_server crash occurs. Change-Id: Ia9275b7cc574df371cdb2e1b80c6699df193b580 Merged-In: Ia9275b7cc574df371cdb2e1b80c6699df193b580 (cherry picked from commit 3e9afc163256db661b9039120d07501b3a8a7d99) --- services/sensorservice/SensorEventConnection.cpp | 28 +++++++++++++++--------- services/sensorservice/SensorEventConnection.h | 5 +++-- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/services/sensorservice/SensorEventConnection.cpp b/services/sensorservice/SensorEventConnection.cpp index b4b5f98609..6c8671289d 100644 --- a/services/sensorservice/SensorEventConnection.cpp +++ b/services/sensorservice/SensorEventConnection.cpp @@ -14,6 +14,7 @@ * limitations under the License. */ +#include #include #include @@ -47,20 +48,13 @@ SensorService::SensorEventConnection::SensorEventConnection( SensorService::SensorEventConnection::~SensorEventConnection() { ALOGD_IF(DEBUG_CONNECTIONS, "~SensorEventConnection(%p)", this); destroy(); -} - -void SensorService::SensorEventConnection::destroy() { - Mutex::Autolock _l(mDestroyLock); - - // destroy once only - if (mDestroyed) { - return; - } - mService->cleanupConnection(this); if (mEventCache != nullptr) { delete[] mEventCache; } +} + +void SensorService::SensorEventConnection::destroy() { mDestroyed = true; } @@ -665,6 +659,11 @@ status_t SensorService::SensorEventConnection::enableDisable( int handle, bool enabled, nsecs_t samplingPeriodNs, nsecs_t maxBatchReportLatencyNs, int reservedFlags) { + if (mDestroyed) { + android_errorWriteLog(0x534e4554, "168211968"); + return DEAD_OBJECT; + } + status_t err; if (enabled) { err = mService->enable(this, handle, samplingPeriodNs, maxBatchReportLatencyNs, @@ -679,10 +678,19 @@ status_t SensorService::SensorEventConnection::enableDisable( status_t SensorService::SensorEventConnection::setEventRate( int handle, nsecs_t samplingPeriodNs) { + if (mDestroyed) { + android_errorWriteLog(0x534e4554, "168211968"); + return DEAD_OBJECT; + } + return mService->setEventRate(this, handle, samplingPeriodNs, mOpPackageName); } status_t SensorService::SensorEventConnection::flush() { + if (mDestroyed) { + return DEAD_OBJECT; + } + return mService->flushSensor(this, mOpPackageName); } diff --git a/services/sensorservice/SensorEventConnection.h b/services/sensorservice/SensorEventConnection.h index 8f2d5db28f..9487a39a92 100644 --- a/services/sensorservice/SensorEventConnection.h +++ b/services/sensorservice/SensorEventConnection.h @@ -17,6 +17,7 @@ #ifndef ANDROID_SENSOR_EVENT_CONNECTION_H #define ANDROID_SENSOR_EVENT_CONNECTION_H +#include #include #include #include @@ -182,8 +183,8 @@ private: int mTotalAcksNeeded, mTotalAcksReceived; #endif - mutable Mutex mDestroyLock; - bool mDestroyed; + // Used to track if this object was inappropriately used after destroy(). + std::atomic_bool mDestroyed; // Store a mapping of sensor handles to required AppOp for a sensor. This map only contains a // valid mapping for sensors that require a permission in order to reduce the lookup time. -- cgit v1.2.3 From 995b24692cd557b71b46be0a402ad8b0ee5c1a09 Mon Sep 17 00:00:00 2001 From: JW Wang Date: Tue, 6 Oct 2020 13:42:41 +0800 Subject: Fixes De data is not backed up correctly when user locked (1/n) https://android.googlesource.com/platform/frameworks/native/+/fda19ecdd6df8f43d3368781a633792723fb9965/cmds/installd/InstalldNativeService.cpp#933 When an APK is stage installed, snapshotAppData(..., FLAG_STORAGE_DE) is called before user unlocked. The function bails out early at #937 because the folder is still encrypted. We should move the if block at #958 above so we can back up De data correctly even when Ce directories not ready. Note we have to check if the De folder exists before making a copy. (Cherry-picked from 1d643c77d3fcb7dfe0bf4dabed2964bbfca04c6f) Bug: 169594054 Test: atest StagedRollbackTest Merged-In: I2ca810bd9495de3bed58378a41b47863c6e8f8dd Change-Id: I2ca810bd9495de3bed58378a41b47863c6e8f8dd --- cmds/installd/InstalldNativeService.cpp | 51 +++++++++++++++++---------------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/cmds/installd/InstalldNativeService.cpp b/cmds/installd/InstalldNativeService.cpp index 34727270e4..856b545c7c 100644 --- a/cmds/installd/InstalldNativeService.cpp +++ b/cmds/installd/InstalldNativeService.cpp @@ -876,6 +876,33 @@ binder::Status InstalldNativeService::snapshotAppData( auto scope_guard = android::base::make_scope_guard(deleter); + if (storageFlags & FLAG_STORAGE_DE) { + auto from = create_data_user_de_package_path(volume_uuid, user, package_name); + auto to = create_data_misc_de_rollback_path(volume_uuid, user, snapshotId); + auto rollback_package_path = create_data_misc_de_rollback_package_path(volume_uuid, user, + snapshotId, package_name); + + int rc = create_dir_if_needed(to.c_str(), kRollbackFolderMode); + if (rc != 0) { + return error(rc, "Failed to create folder " + to); + } + + rc = delete_dir_contents(rollback_package_path, true /* ignore_if_missing */); + if (rc != 0) { + return error(rc, "Failed clearing existing snapshot " + rollback_package_path); + } + + // Check if we have data to copy. + if (access(from.c_str(), F_OK) == 0) { + rc = copy_directory_recursive(from.c_str(), to.c_str()); + } + if (rc != 0) { + res = error(rc, "Failed copying " + from + " to " + to); + clear_de_on_exit = true; + return res; + } + } + // The app may not have any data at all, in which case it's OK to skip here. auto from_ce = create_data_user_ce_package_path(volume_uuid, user, package_name); if (access(from_ce.c_str(), F_OK) != 0) { @@ -901,30 +928,6 @@ binder::Status InstalldNativeService::snapshotAppData( LOG(WARNING) << "Failed to clear code_cache of app " << packageName; } - if (storageFlags & FLAG_STORAGE_DE) { - auto from = create_data_user_de_package_path(volume_uuid, user, package_name); - auto to = create_data_misc_de_rollback_path(volume_uuid, user, snapshotId); - auto rollback_package_path = create_data_misc_de_rollback_package_path(volume_uuid, user, - snapshotId, package_name); - - int rc = create_dir_if_needed(to.c_str(), kRollbackFolderMode); - if (rc != 0) { - return error(rc, "Failed to create folder " + to); - } - - rc = delete_dir_contents(rollback_package_path, true /* ignore_if_missing */); - if (rc != 0) { - return error(rc, "Failed clearing existing snapshot " + rollback_package_path); - } - - rc = copy_directory_recursive(from.c_str(), to.c_str()); - if (rc != 0) { - res = error(rc, "Failed copying " + from + " to " + to); - clear_de_on_exit = true; - return res; - } - } - if (storageFlags & FLAG_STORAGE_CE) { auto from = create_data_user_ce_package_path(volume_uuid, user, package_name); auto to = create_data_misc_ce_rollback_path(volume_uuid, user, snapshotId); -- cgit v1.2.3 From 7c227cc333b85938a1ad0f860655bb83567ca755 Mon Sep 17 00:00:00 2001 From: Jon Spivack Date: Tue, 27 Oct 2020 19:29:14 -0700 Subject: libbinder: Add ClientCounterCallbackImpl to LazyServiceRegistrar This extra layer of indirection below ClientCounterCallback fixes a shared pointer ownership issue between LazyServiceRegistrar and ServiceManager. It also allows for implementation changes (like this one) without changing headers and breaking VNDK. Bug: 170212632 Test: Manual (Went through reproduction steps in bug on cf_x86_phone-userdebug) Test: atest aidl_lazy_test Change-Id: I4164a6d44e567c752726953e85aee0e91c6b525e Merged-In: I4164a6d44e567c752726953e85aee0e91c6b525e --- libs/binder/LazyServiceRegistrar.cpp | 46 +++++++++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 11 deletions(-) diff --git a/libs/binder/LazyServiceRegistrar.cpp b/libs/binder/LazyServiceRegistrar.cpp index 6f49aa1607..f2c5139b56 100644 --- a/libs/binder/LazyServiceRegistrar.cpp +++ b/libs/binder/LazyServiceRegistrar.cpp @@ -29,16 +29,12 @@ namespace internal { using AidlServiceManager = android::os::IServiceManager; -class ClientCounterCallback : public ::android::os::BnClientCallback { +class ClientCounterCallbackImpl : public ::android::os::BnClientCallback { public: - ClientCounterCallback() : mNumConnectedServices(0), mForcePersist(false) {} + ClientCounterCallbackImpl() : mNumConnectedServices(0), mForcePersist(false) {} bool registerService(const sp& service, const std::string& name, bool allowIsolated, int dumpFlags); - - /** - * Set a flag to prevent services from automatically shutting down - */ void forcePersist(bool persist); protected: @@ -69,7 +65,23 @@ private: bool mForcePersist; }; -bool ClientCounterCallback::registerService(const sp& service, const std::string& name, +class ClientCounterCallback { +public: + ClientCounterCallback(); + + bool registerService(const sp& service, const std::string& name, + bool allowIsolated, int dumpFlags); + + /** + * Set a flag to prevent services from automatically shutting down + */ + void forcePersist(bool persist); + +private: + sp mImpl; +}; + +bool ClientCounterCallbackImpl::registerService(const sp& service, const std::string& name, bool allowIsolated, int dumpFlags) { auto manager = interface_cast(asBinder(defaultServiceManager())); @@ -95,7 +107,7 @@ bool ClientCounterCallback::registerService(const sp& service, const st return true; } -void ClientCounterCallback::forcePersist(bool persist) { +void ClientCounterCallbackImpl::forcePersist(bool persist) { mForcePersist = persist; if(!mForcePersist) { // Attempt a shutdown in case the number of clients hit 0 while the flag was on @@ -107,7 +119,7 @@ void ClientCounterCallback::forcePersist(bool persist) { * onClients is oneway, so no need to worry about multi-threading. Note that this means multiple * invocations could occur on different threads however. */ -Status ClientCounterCallback::onClients(const sp& service, bool clients) { +Status ClientCounterCallbackImpl::onClients(const sp& service, bool clients) { if (clients) { mNumConnectedServices++; } else { @@ -122,7 +134,7 @@ Status ClientCounterCallback::onClients(const sp& service, bool clients return Status::ok(); } -void ClientCounterCallback::tryShutdown() { +void ClientCounterCallbackImpl::tryShutdown() { if(mNumConnectedServices > 0) { // Should only shut down if there are no clients return; @@ -143,7 +155,6 @@ void ClientCounterCallback::tryShutdown() { bool success = manager->tryUnregisterService(entry.first, entry.second.service).isOk(); - if (!success) { ALOGI("Failed to unregister service %s", entry.first.c_str()); break; @@ -168,6 +179,19 @@ void ClientCounterCallback::tryShutdown() { } } +ClientCounterCallback::ClientCounterCallback() { + mImpl = new ClientCounterCallbackImpl(); +} + +bool ClientCounterCallback::registerService(const sp& service, const std::string& name, + bool allowIsolated, int dumpFlags) { + return mImpl->registerService(service, name, allowIsolated, dumpFlags); +} + +void ClientCounterCallback::forcePersist(bool persist) { + mImpl->forcePersist(persist); +} + } // namespace internal LazyServiceRegistrar::LazyServiceRegistrar() { -- cgit v1.2.3 From 58f5cfa56d5282e69a7580dc4bb97603c409f003 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Wed, 18 Nov 2020 00:32:42 +0000 Subject: libbinder: check null bytes in readString*Inplace This is entirely defensive, since the only real guarantee we have here from these APIs is that a buffer of a given length is available. However, since we write 0's here, presumably to guard against people assuming these are null-terminated strings, we might as well enforce that they are actually null terminated. Bug: 172655291 Test: binderParcelTest (added in newer CL) Change-Id: Ie879112540155f6a93b97aeaf3d41ed8ba4ae79f Merged-In: Ie879112540155f6a93b97aeaf3d41ed8ba4ae79f (cherry picked from commit 51e02b16c397c44ddf81a0736cf6045cd4c44128) --- libs/binder/Parcel.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp index 9642a87f4e..1f7d27e0e9 100644 --- a/libs/binder/Parcel.cpp +++ b/libs/binder/Parcel.cpp @@ -1869,7 +1869,7 @@ const char* Parcel::readString8Inplace(size_t* outLen) const if (size >= 0 && size < INT32_MAX) { *outLen = size; const char* str = (const char*)readInplace(size+1); - if (str != nullptr) { + if (str != nullptr && str[size] == '\0') { return str; } } @@ -1929,7 +1929,7 @@ const char16_t* Parcel::readString16Inplace(size_t* outLen) const if (size >= 0 && size < INT32_MAX) { *outLen = size; const char16_t* str = (const char16_t*)readInplace((size+1)*sizeof(char16_t)); - if (str != nullptr) { + if (str != nullptr && str[size] == u'\0') { return str; } } -- cgit v1.2.3 From 61d0f84881cfc1bbac513ccd156c56603a48cc90 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Fri, 4 Dec 2020 21:13:03 +0000 Subject: libbinder: readString*Inplace SafetyNet (II) SafetyNet logs (this time for failure case, instead of success case). Bug: 172655291 Test: adb logcat -b events | grep snet # exactly one occurance w/ repro (c/p'd from 34af0637666f43ae62040ad1bad76468423feba2) Merged-In: I75ace071693c0a4579ed9477f7b9212a6e27c36d Change-Id: I75ace071693c0a4579ed9477f7b9212a6e27c36d --- libs/binder/Parcel.cpp | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp index 1f7d27e0e9..b7ad660317 100644 --- a/libs/binder/Parcel.cpp +++ b/libs/binder/Parcel.cpp @@ -1869,8 +1869,11 @@ const char* Parcel::readString8Inplace(size_t* outLen) const if (size >= 0 && size < INT32_MAX) { *outLen = size; const char* str = (const char*)readInplace(size+1); - if (str != nullptr && str[size] == '\0') { - return str; + if (str != nullptr) { + if (str[size] == '\0') { + return str; + } + android_errorWriteLog(0x534e4554, "172655291"); } } *outLen = 0; @@ -1929,8 +1932,11 @@ const char16_t* Parcel::readString16Inplace(size_t* outLen) const if (size >= 0 && size < INT32_MAX) { *outLen = size; const char16_t* str = (const char16_t*)readInplace((size+1)*sizeof(char16_t)); - if (str != nullptr && str[size] == u'\0') { - return str; + if (str != nullptr) { + if (str[size] == u'\0') { + return str; + } + android_errorWriteLog(0x534e4554, "172655291"); } } *outLen = 0; -- cgit v1.2.3 From 58fbf9fb2c82ee09b0d572e0936b8370c4d35aef Mon Sep 17 00:00:00 2001 From: Denis Hsu Date: Wed, 17 Jun 2020 10:17:30 +0800 Subject: SF: update mInputFlinger on main thread mInputFlinger can be accessed by bootFinished() and updateInputFlinger(). These function can run in different thread at the same time, but mInputFlinger was not protected. update mInputFlinger on main thread Fixes: 157871763 Fixes: 147009853 Fixes: 169256435 Test: adb shell stop / start Test: boot ok Test: Race was only reproducible in monkey tests Change-Id: I4d87e90793a88a646aaa1ae5806f118f1ae51e30 Merged-In: I4d87e90793a88a646aaa1ae5806f118f1ae51e30 --- services/surfaceflinger/SurfaceFlinger.cpp | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 1a25b8230e..bc134cb158 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -538,13 +538,6 @@ void SurfaceFlinger::bootFinished() if (window != 0) { window->linkToDeath(static_cast(this)); } - sp input(defaultServiceManager()->getService( - String16("inputflinger"))); - if (input == nullptr) { - ALOGE("Failed to link to input service"); - } else { - mInputFlinger = interface_cast(input); - } if (mVrFlinger) { mVrFlinger->OnBootFinished(); @@ -559,7 +552,15 @@ void SurfaceFlinger::bootFinished() LOG_EVENT_LONG(LOGTAG_SF_STOP_BOOTANIM, ns2ms(systemTime(SYSTEM_TIME_MONOTONIC))); - postMessageAsync(new LambdaMessage([this]() NO_THREAD_SAFETY_ANALYSIS { + sp input(defaultServiceManager()->getService(String16("inputflinger"))); + + postMessageAsync(new LambdaMessage([=]() NO_THREAD_SAFETY_ANALYSIS { + if (input == nullptr) { + ALOGE("Failed to link to input service"); + } else { + mInputFlinger = interface_cast(input); + } + readPersistentProperties(); mBootStage = BootStage::FINISHED; -- cgit v1.2.3