From 0ebd5f95b68a3a5c9e5509f21938c9e51e74d71b Mon Sep 17 00:00:00 2001 From: Eric Laurent Date: Wed, 19 Nov 2014 19:04:52 -0800 Subject: AudioSystem: fix cross deadlock Do not hold gLockAPS when calling AudioPolicyService::registerClient() in get_audio_policy_service(). registerClient() will need to acquire the AudioPolicyService mutex and if at the same time a method called from AudioPolicyService (with mutex held) calls back into AudioSystem and get_audio_policy_service() a cross deadlock occurs. Same preventive fix for get_audio_flinger(). Use a separate mutex for notification client list in AudioPolicyService. This prevents deadlocking if registerClient() is called as a consequence of AudioFlinger calling back into AudioPolicyManager while executing a method with AudioPolicyService locked Bug: 18403952. Bug: 18450065. Change-Id: Ia832e41aede8bc6c843fc615508fbdd74e0863b5 --- include/media/AudioSystem.h | 4 +- media/libmedia/AudioSystem.cpp | 101 ++++++++++++++++------------ services/audiopolicy/AudioPolicyService.cpp | 17 +++-- services/audiopolicy/AudioPolicyService.h | 2 +- 4 files changed, 71 insertions(+), 53 deletions(-) diff --git a/include/media/AudioSystem.h b/include/media/AudioSystem.h index d54eca7..1614525 100644 --- a/include/media/AudioSystem.h +++ b/include/media/AudioSystem.h @@ -90,7 +90,7 @@ public: static void setErrorCallback(audio_error_callback cb); // helper function to obtain AudioFlinger service handle - static const sp& get_audio_flinger(); + static const sp get_audio_flinger(); static float linearToLog(int volume); static int logToLinear(float volume); @@ -270,7 +270,7 @@ public: // and output configuration cache (gOutputs) static void clearAudioConfigCache(); - static const sp& get_audio_policy_service(); + static const sp get_audio_policy_service(); // helpers for android.media.AudioManager.getProperty(), see description there for meaning static uint32_t getPrimaryOutputSamplingRate(); diff --git a/media/libmedia/AudioSystem.cpp b/media/libmedia/AudioSystem.cpp index 0e608f8..1f8e9b6 100644 --- a/media/libmedia/AudioSystem.cpp +++ b/media/libmedia/AudioSystem.cpp @@ -51,33 +51,40 @@ size_t AudioSystem::gInBuffSize = 0; // zero indicates cache is invalid sp AudioSystem::gAudioPortCallback; // establish binder interface to AudioFlinger service -const sp& AudioSystem::get_audio_flinger() +const sp AudioSystem::get_audio_flinger() { - Mutex::Autolock _l(gLock); - if (gAudioFlinger == 0) { - sp sm = defaultServiceManager(); - sp binder; - do { - binder = sm->getService(String16("media.audio_flinger")); - if (binder != 0) - break; - ALOGW("AudioFlinger not published, waiting..."); - usleep(500000); // 0.5 s - } while (true); - if (gAudioFlingerClient == NULL) { - gAudioFlingerClient = new AudioFlingerClient(); - } else { - if (gAudioErrorCallback) { - gAudioErrorCallback(NO_ERROR); + sp af; + sp afc; + { + Mutex::Autolock _l(gLock); + if (gAudioFlinger == 0) { + sp sm = defaultServiceManager(); + sp binder; + do { + binder = sm->getService(String16("media.audio_flinger")); + if (binder != 0) + break; + ALOGW("AudioFlinger not published, waiting..."); + usleep(500000); // 0.5 s + } while (true); + if (gAudioFlingerClient == NULL) { + gAudioFlingerClient = new AudioFlingerClient(); + } else { + if (gAudioErrorCallback) { + gAudioErrorCallback(NO_ERROR); + } } + binder->linkToDeath(gAudioFlingerClient); + gAudioFlinger = interface_cast(binder); + LOG_ALWAYS_FATAL_IF(gAudioFlinger == 0); + afc = gAudioFlingerClient; } - binder->linkToDeath(gAudioFlingerClient); - gAudioFlinger = interface_cast(binder); - LOG_ALWAYS_FATAL_IF(gAudioFlinger == 0); - gAudioFlinger->registerClient(gAudioFlingerClient); + af = gAudioFlinger; } - - return gAudioFlinger; + if (afc != 0) { + af->registerClient(afc); + } + return af; } /* static */ status_t AudioSystem::checkAudioFlinger() @@ -546,29 +553,37 @@ sp AudioSystem::gAudioPolicyServiceClient // establish binder interface to AudioPolicy service -const sp& AudioSystem::get_audio_policy_service() -{ - Mutex::Autolock _l(gLockAPS); - if (gAudioPolicyService == 0) { - sp sm = defaultServiceManager(); - sp binder; - do { - binder = sm->getService(String16("media.audio_policy")); - if (binder != 0) - break; - ALOGW("AudioPolicyService not published, waiting..."); - usleep(500000); // 0.5 s - } while (true); - if (gAudioPolicyServiceClient == NULL) { - gAudioPolicyServiceClient = new AudioPolicyServiceClient(); +const sp AudioSystem::get_audio_policy_service() +{ + sp ap; + sp apc; + { + Mutex::Autolock _l(gLockAPS); + if (gAudioPolicyService == 0) { + sp sm = defaultServiceManager(); + sp binder; + do { + binder = sm->getService(String16("media.audio_policy")); + if (binder != 0) + break; + ALOGW("AudioPolicyService not published, waiting..."); + usleep(500000); // 0.5 s + } while (true); + if (gAudioPolicyServiceClient == NULL) { + gAudioPolicyServiceClient = new AudioPolicyServiceClient(); + } + binder->linkToDeath(gAudioPolicyServiceClient); + gAudioPolicyService = interface_cast(binder); + LOG_ALWAYS_FATAL_IF(gAudioPolicyService == 0); + apc = gAudioPolicyServiceClient; } - binder->linkToDeath(gAudioPolicyServiceClient); - gAudioPolicyService = interface_cast(binder); - LOG_ALWAYS_FATAL_IF(gAudioPolicyService == 0); - gAudioPolicyService->registerClient(gAudioPolicyServiceClient); + ap = gAudioPolicyService; + } + if (apc != 0) { + ap->registerClient(apc); } - return gAudioPolicyService; + return ap; } // --------------------------------------------------------------------------- diff --git a/services/audiopolicy/AudioPolicyService.cpp b/services/audiopolicy/AudioPolicyService.cpp index dd4067f..6a4a669 100644 --- a/services/audiopolicy/AudioPolicyService.cpp +++ b/services/audiopolicy/AudioPolicyService.cpp @@ -149,7 +149,7 @@ AudioPolicyService::~AudioPolicyService() void AudioPolicyService::registerClient(const sp& client) { - Mutex::Autolock _l(mLock); + Mutex::Autolock _l(mNotificationClientsLock); uid_t uid = IPCThreadState::self()->getCallingUid(); if (mNotificationClients.indexOfKey(uid) < 0) { @@ -168,14 +168,17 @@ void AudioPolicyService::registerClient(const sp& cli // removeNotificationClient() is called when the client process dies. void AudioPolicyService::removeNotificationClient(uid_t uid) { - Mutex::Autolock _l(mLock); - - mNotificationClients.removeItem(uid); - + { + Mutex::Autolock _l(mNotificationClientsLock); + mNotificationClients.removeItem(uid); + } #ifndef USE_LEGACY_AUDIO_POLICY + { + Mutex::Autolock _l(mLock); if (mAudioPolicyManager) { mAudioPolicyManager->clearAudioPatches(uid); } + } #endif } @@ -186,7 +189,7 @@ void AudioPolicyService::onAudioPortListUpdate() void AudioPolicyService::doOnAudioPortListUpdate() { - Mutex::Autolock _l(mLock); + Mutex::Autolock _l(mNotificationClientsLock); for (size_t i = 0; i < mNotificationClients.size(); i++) { mNotificationClients.valueAt(i)->onAudioPortListUpdate(); } @@ -212,7 +215,7 @@ status_t AudioPolicyService::clientReleaseAudioPatch(audio_patch_handle_t handle void AudioPolicyService::doOnAudioPatchListUpdate() { - Mutex::Autolock _l(mLock); + Mutex::Autolock _l(mNotificationClientsLock); for (size_t i = 0; i < mNotificationClients.size(); i++) { mNotificationClients.valueAt(i)->onAudioPatchListUpdate(); } diff --git a/services/audiopolicy/AudioPolicyService.h b/services/audiopolicy/AudioPolicyService.h index 4e68ab1..f1db309 100644 --- a/services/audiopolicy/AudioPolicyService.h +++ b/services/audiopolicy/AudioPolicyService.h @@ -495,7 +495,7 @@ private: AudioPolicyClient *mAudioPolicyClient; DefaultKeyedVector< uid_t, sp > mNotificationClients; - + Mutex mNotificationClientsLock; // protects mNotificationClients // Manage all effects configured in audio_effects.conf sp mAudioPolicyEffects; audio_mode_t mPhoneState; -- cgit v1.1