From 12b716ce09943cc7b427f192d986e6830822c166 Mon Sep 17 00:00:00 2001 From: Mikhail Naganov Date: Thu, 30 Apr 2020 22:37:43 +0000 Subject: [PATCH] Enforce basic thread safety in Audio Policy Service Enable clang thread safety analysis. Convert comments on TS into directives. Fix missing lock acquisitions. Add missing default initializers for the fields of UidPolicy and SensorPrivacyPolicy Also remove unused fields mpAudioPolicyDev and mpAudioPolicy from AudioPolicyService. TODO: Consider protecting pointers to command threads, APM instance and the client, and system usages with a separate lock. Bug: 70398235 Bug: 155336464 Test: make, flash, and test audio on device execute "adb shell cmd media.audio_policy" commands Change-Id: I47b132c8b5c977812fad5c89fa57882a37779c18 Merged-In: I47b132c8b5c977812fad5c89fa57882a37779c18 --- services/audiopolicy/service/Android.mk | 2 +- .../service/AudioPolicyEffects.cpp | 5 +- .../service/AudioPolicyService.cpp | 86 ++++++++++++------- .../audiopolicy/service/AudioPolicyService.h | 42 ++++----- 4 files changed, 80 insertions(+), 55 deletions(-) diff --git a/services/audiopolicy/service/Android.mk b/services/audiopolicy/service/Android.mk index 94e4811e24..680b077f10 100644 --- a/services/audiopolicy/service/Android.mk +++ b/services/audiopolicy/service/Android.mk @@ -44,7 +44,7 @@ LOCAL_STATIC_LIBRARIES := \ LOCAL_MODULE:= libaudiopolicyservice LOCAL_CFLAGS += -fvisibility=hidden -LOCAL_CFLAGS += -Wall -Werror +LOCAL_CFLAGS += -Wall -Werror -Wthread-safety include $(BUILD_SHARED_LIBRARY) diff --git a/services/audiopolicy/service/AudioPolicyEffects.cpp b/services/audiopolicy/service/AudioPolicyEffects.cpp index 738a2793d4..1ec0c5e3ba 100644 --- a/services/audiopolicy/service/AudioPolicyEffects.cpp +++ b/services/audiopolicy/service/AudioPolicyEffects.cpp @@ -928,7 +928,10 @@ status_t AudioPolicyEffects::loadAudioEffectXmlConfig() { loadProcessingChain(result.parsedConfig->preprocess, mInputSources); loadProcessingChain(result.parsedConfig->postprocess, mOutputStreams); - loadDeviceProcessingChain(result.parsedConfig->deviceprocess, mDeviceEffects); + { + Mutex::Autolock _l(mLock); + loadDeviceProcessingChain(result.parsedConfig->deviceprocess, mDeviceEffects); + } // Casting from ssize_t to status_t is probably safe, there should not be more than 2^31 errors return result.nbSkippedElement; } diff --git a/services/audiopolicy/service/AudioPolicyService.cpp b/services/audiopolicy/service/AudioPolicyService.cpp index d743be9ab6..8c052100a3 100644 --- a/services/audiopolicy/service/AudioPolicyService.cpp +++ b/services/audiopolicy/service/AudioPolicyService.cpp @@ -58,8 +58,6 @@ static const String16 sManageAudioPolicyPermission("android.permission.MANAGE_AU AudioPolicyService::AudioPolicyService() : BnAudioPolicyService(), - mpAudioPolicyDev(NULL), - mpAudioPolicy(NULL), mAudioPolicyManager(NULL), mAudioPolicyClient(NULL), mPhoneState(AUDIO_MODE_INVALID), @@ -78,21 +76,19 @@ void AudioPolicyService::onFirstRef() mAudioPolicyClient = new AudioPolicyClient(this); mAudioPolicyManager = createAudioPolicyManager(mAudioPolicyClient); - - mSupportedSystemUsages = std::vector {}; } // load audio processing modules - spaudioPolicyEffects = new AudioPolicyEffects(); + sp audioPolicyEffects = new AudioPolicyEffects(); + sp uidPolicy = new UidPolicy(this); + sp sensorPrivacyPolicy = new SensorPrivacyPolicy(this); { Mutex::Autolock _l(mLock); mAudioPolicyEffects = audioPolicyEffects; + mUidPolicy = uidPolicy; + mSensorPrivacyPolicy = sensorPrivacyPolicy; } - - mUidPolicy = new UidPolicy(this); - mUidPolicy->registerSelf(); - - mSensorPrivacyPolicy = new SensorPrivacyPolicy(this); - mSensorPrivacyPolicy->registerSelf(); + uidPolicy->registerSelf(); + sensorPrivacyPolicy->registerSelf(); } AudioPolicyService::~AudioPolicyService() @@ -107,9 +103,9 @@ AudioPolicyService::~AudioPolicyService() mAudioPolicyEffects.clear(); mUidPolicy->unregisterSelf(); - mUidPolicy.clear(); - mSensorPrivacyPolicy->unregisterSelf(); + + mUidPolicy.clear(); mSensorPrivacyPolicy.clear(); } @@ -172,20 +168,20 @@ void AudioPolicyService::setAudioVolumeGroupCallbacksEnabled(bool enabled) // removeNotificationClient() is called when the client process dies. void AudioPolicyService::removeNotificationClient(uid_t uid, pid_t pid) { + bool hasSameUid = false; { Mutex::Autolock _l(mNotificationClientsLock); int64_t token = ((int64_t)uid<<32) | pid; mNotificationClients.removeItem(token); - } - { - Mutex::Autolock _l(mLock); - bool hasSameUid = false; for (size_t i = 0; i < mNotificationClients.size(); i++) { if (mNotificationClients.valueAt(i)->uid() == uid) { hasSameUid = true; break; } } + } + { + Mutex::Autolock _l(mLock); if (mAudioPolicyManager && !hasSameUid) { // called from binder death notification: no need to clear caller identity mAudioPolicyManager->releaseResourcesForUid(uid); @@ -381,10 +377,14 @@ void AudioPolicyService::binderDied(const wp& who) { IPCThreadState::self()->getCallingPid()); } -static bool dumpTryLock(Mutex& mutex) +static bool dumpTryLock(Mutex& mutex) ACQUIRE(mutex) NO_THREAD_SAFETY_ANALYSIS +{ + return mutex.timedLock(kDumpLockTimeoutNs) == NO_ERROR; +} + +static void dumpReleaseLock(Mutex& mutex, bool locked) RELEASE(mutex) NO_THREAD_SAFETY_ANALYSIS { - status_t err = mutex.timedLock(kDumpLockTimeoutNs); - return err == NO_ERROR; + if (locked) mutex.unlock(); } status_t AudioPolicyService::dumpInternals(int fd) @@ -564,7 +564,7 @@ void AudioPolicyService::updateUidStates_l() bool isTopOrLatestSensitive = topSensitiveActive == nullptr ? false : current->uid == topSensitiveActive->uid; - auto canCaptureIfInCallOrCommunication = [&](const auto &recordClient) { + auto canCaptureIfInCallOrCommunication = [&](const auto &recordClient) REQUIRES(mLock) { bool canCaptureCall = recordClient->canCaptureOutput; bool canCaptureCommunication = recordClient->canCaptureOutput || recordClient->uid == mPhoneStateOwnerUid @@ -702,7 +702,7 @@ status_t AudioPolicyService::dump(int fd, const Vector& args __unused) if (!dumpAllowed()) { dumpPermissionDenial(fd); } else { - bool locked = dumpTryLock(mLock); + const bool locked = dumpTryLock(mLock); if (!locked) { String8 result(kDeadlockedString); write(fd, result.string(), result.size()); @@ -719,7 +719,7 @@ status_t AudioPolicyService::dump(int fd, const Vector& args __unused) mPackageManager.dump(fd); - if (locked) mLock.unlock(); + dumpReleaseLock(mLock, locked); } return NO_ERROR; } @@ -838,8 +838,16 @@ status_t AudioPolicyService::handleSetUidState(Vector& args, int err) return BAD_VALUE; } - mUidPolicy->addOverrideUid(uid, active); - return NO_ERROR; + sp uidPolicy; + { + Mutex::Autolock _l(mLock); + uidPolicy = mUidPolicy; + } + if (uidPolicy) { + uidPolicy->addOverrideUid(uid, active); + return NO_ERROR; + } + return NO_INIT; } status_t AudioPolicyService::handleResetUidState(Vector& args, int err) { @@ -859,8 +867,16 @@ status_t AudioPolicyService::handleResetUidState(Vector& args, int err return BAD_VALUE; } - mUidPolicy->removeOverrideUid(uid); - return NO_ERROR; + sp uidPolicy; + { + Mutex::Autolock _l(mLock); + uidPolicy = mUidPolicy; + } + if (uidPolicy) { + uidPolicy->removeOverrideUid(uid); + return NO_ERROR; + } + return NO_INIT; } status_t AudioPolicyService::handleGetUidState(Vector& args, int out, int err) { @@ -880,11 +896,15 @@ status_t AudioPolicyService::handleGetUidState(Vector& args, int out, return BAD_VALUE; } - if (mUidPolicy->isUidActive(uid)) { - return dprintf(out, "active\n"); - } else { - return dprintf(out, "idle\n"); + sp uidPolicy; + { + Mutex::Autolock _l(mLock); + uidPolicy = mUidPolicy; + } + if (uidPolicy) { + return dprintf(out, uidPolicy->isUidActive(uid) ? "active\n" : "idle\n"); } + return NO_INIT; } status_t AudioPolicyService::printHelp(int out) { @@ -1401,7 +1421,7 @@ status_t AudioPolicyService::AudioCommandThread::dump(int fd) result.append(buffer); write(fd, result.string(), result.size()); - bool locked = dumpTryLock(mLock); + const bool locked = dumpTryLock(mLock); if (!locked) { String8 result2(kCmdDeadlockedString); write(fd, result2.string(), result2.size()); @@ -1424,7 +1444,7 @@ status_t AudioPolicyService::AudioCommandThread::dump(int fd) write(fd, result.string(), result.size()); - if (locked) mLock.unlock(); + dumpReleaseLock(mLock, locked); return NO_ERROR; } diff --git a/services/audiopolicy/service/AudioPolicyService.h b/services/audiopolicy/service/AudioPolicyService.h index f77a481db1..869a963d05 100644 --- a/services/audiopolicy/service/AudioPolicyService.h +++ b/services/audiopolicy/service/AudioPolicyService.h @@ -17,6 +17,7 @@ #ifndef ANDROID_AUDIOPOLICYSERVICE_H #define ANDROID_AUDIOPOLICYSERVICE_H +#include #include #include #include @@ -330,13 +331,13 @@ private: AudioPolicyService() ANDROID_API; virtual ~AudioPolicyService(); - status_t dumpInternals(int fd); + status_t dumpInternals(int fd) REQUIRES(mLock); // Handles binder shell commands virtual status_t shellCommand(int in, int out, int err, Vector& args); // Sets whether the given UID records only silence - virtual void setAppState_l(audio_port_handle_t portId, app_state_t state); + virtual void setAppState_l(audio_port_handle_t portId, app_state_t state) REQUIRES(mLock); // Overrides the UID state as if it is idle status_t handleSetUidState(Vector& args, int err); @@ -361,9 +362,9 @@ private: status_t validateUsage(audio_usage_t usage, pid_t pid, uid_t uid); void updateUidStates(); - void updateUidStates_l(); + void updateUidStates_l() REQUIRES(mLock); - void silenceAllRecordings_l(); + void silenceAllRecordings_l() REQUIRES(mLock); static bool isVirtualSource(audio_source_t source); @@ -420,13 +421,13 @@ private: wp mService; Mutex mLock; ActivityManager mAm; - bool mObserverRegistered; + bool mObserverRegistered = false; std::unordered_map> mOverrideUids; std::unordered_map> mCachedUids; - uid_t mAssistantUid; + uid_t mAssistantUid = -1; std::vector mA11yUids; - uid_t mCurrentImeUid; - bool mRttEnabled; + uid_t mCurrentImeUid = -1; + bool mRttEnabled = false; }; // If sensor privacy is enabled then all apps, including those that are active, should be @@ -447,7 +448,7 @@ private: private: wp mService; - std::atomic_bool mSensorPrivacyEnabled; + std::atomic_bool mSensorPrivacyEnabled = false; }; // Thread used to send audio config commands to audio flinger @@ -880,26 +881,27 @@ private: // and possibly back in to audio policy service and acquire mEffectsLock. sp mAudioCommandThread; // audio commands thread sp mOutputCommandThread; // process stop and release output - struct audio_policy_device *mpAudioPolicyDev; - struct audio_policy *mpAudioPolicy; AudioPolicyInterface *mAudioPolicyManager; AudioPolicyClient *mAudioPolicyClient; std::vector mSupportedSystemUsages; - DefaultKeyedVector< int64_t, sp > mNotificationClients; - Mutex mNotificationClientsLock; // protects mNotificationClients + Mutex mNotificationClientsLock; + DefaultKeyedVector> mNotificationClients + GUARDED_BY(mNotificationClientsLock); // Manage all effects configured in audio_effects.conf // never hold AudioPolicyService::mLock when calling AudioPolicyEffects methods as // those can call back into AudioPolicyService methods and try to acquire the mutex - sp mAudioPolicyEffects; - audio_mode_t mPhoneState; - uid_t mPhoneStateOwnerUid; + sp mAudioPolicyEffects GUARDED_BY(mLock); + audio_mode_t mPhoneState GUARDED_BY(mLock); + uid_t mPhoneStateOwnerUid GUARDED_BY(mLock); - sp mUidPolicy; - sp mSensorPrivacyPolicy; + sp mUidPolicy GUARDED_BY(mLock); + sp mSensorPrivacyPolicy GUARDED_BY(mLock); - DefaultKeyedVector< audio_port_handle_t, sp > mAudioRecordClients; - DefaultKeyedVector< audio_port_handle_t, sp > mAudioPlaybackClients; + DefaultKeyedVector> mAudioRecordClients + GUARDED_BY(mLock); + DefaultKeyedVector> mAudioPlaybackClients + GUARDED_BY(mLock); MediaPackageManager mPackageManager; // To check allowPlaybackCapture