DO NOT MERGE - improve audio effect framwework thread safety

- Reorganize handle effect creation code to make sure the effect engine
is created with both thread and effect chain mutex held.
- Reorganize handle disconnect code to make sure the effect engine
is released with both thread and effect chain mutex held.
- Protect IEffect interface methods in EffectHande with a Mutex.
- Only pin effect if the session was acquired first.
- Do not use strong pointer to EffectModule in EffectHandles:
only the EffectChain has a single strong reference to the EffectModule.

Bug: 32707507

CVE-2017-0479
CVE-2017-0480
CVE-2017-0499

Change-Id: Ia1098cba2cd32cc2d1c9dfdff4adc2388dfed80e
(cherry picked from commit b378b73dd7480b584340b8028802c9ca2d625123)
(cherry picked from commit 22e26d8ee73488c58ba3e7928e5da155151abfd0
 with backport by <sultanxda@gmail.com>)

7 files changed, 225 insertions, 123 deletions

diff --git a/services/audioflinger/AudioFlinger.cpp b/services/audioflinger/AudioFlinger.cpp
index 23215dd..9d435e9 100644
--- a/services/audioflinger/AudioFlinger.cpp
+++ b/services/audioflinger/AudioFlinger.cpp
@@ -1346,7 +1346,7 @@ void AudioFlinger::removeNotificationClient(pid_t pid)
     ALOGV("%d died, releasing its sessions", pid);
     size_t num = mAudioSessionRefs.size();
     bool removed = false;
-    for (size_t i = 0; i< num; ) {
+    for (size_t i = 0; i < num; ) {
         AudioSessionRef *ref = mAudioSessionRefs.itemAt(i);
         ALOGV(" pid %d @ %d", ref->mPid, i);
         if (ref->mPid == pid) {
@@ -2343,7 +2343,7 @@ void AudioFlinger::acquireAudioSessionId(int audioSession, pid_t pid)
     }
 
     size_t num = mAudioSessionRefs.size();
-    for (size_t i = 0; i< num; i++) {
+    for (size_t i = 0; i < num; i++) {
         AudioSessionRef *ref = mAudioSessionRefs.editItemAt(i);
         if (ref->mSessionid == audioSession && ref->mPid == caller) {
             ref->mCnt++;
@@ -2364,7 +2364,7 @@ void AudioFlinger::releaseAudioSessionId(int audioSession, pid_t pid)
         caller = pid;
     }
     size_t num = mAudioSessionRefs.size();
-    for (size_t i = 0; i< num; i++) {
+    for (size_t i = 0; i < num; i++) {
         AudioSessionRef *ref = mAudioSessionRefs.itemAt(i);
         if (ref->mSessionid == audioSession && ref->mPid == caller) {
             ref->mCnt--;
@@ -2382,6 +2382,18 @@ void AudioFlinger::releaseAudioSessionId(int audioSession, pid_t pid)
     ALOGW_IF(caller != getpid_cached, "session id %d not found for pid %d", audioSession, caller);
 }
 
+bool AudioFlinger::isSessionAcquired_l(audio_session_t audioSession)
+{
+    size_t num = mAudioSessionRefs.size();
+    for (size_t i = 0; i < num; i++) {
+        AudioSessionRef *ref = mAudioSessionRefs.itemAt(i);
+        if (ref->mSessionid == audioSession) {
+            return true;
+        }
+    }
+    return false;
+}
+
 void AudioFlinger::purgeStaleEffects_l() {
 
     ALOGV("purging stale effects");
@@ -2723,8 +2735,9 @@ sp<IEffect> AudioFlinger::createEffect(
         sp<Client> client = registerPid(pid);
 
         // create effect on selected output thread
+        bool pinned = (sessionId > AUDIO_SESSION_OUTPUT_MIX) && isSessionAcquired_l((audio_session_t)sessionId);
         handle = thread->createEffect_l(client, effectClient, priority, sessionId,
-                &desc, enabled, &lStatus);
+                &desc, enabled, &lStatus, pinned);
         if (handle != 0 && id != NULL) {
             *id = handle->id();
         }
@@ -2924,7 +2937,7 @@ bool AudioFlinger::updateOrphanEffectChains(const sp<AudioFlinger::EffectModule>
     ALOGV("updateOrphanEffectChains session %d index %d", session, index);
     if (index >= 0) {
         sp<EffectChain> chain = mOrphanEffectChains.valueAt(index);
-        if (chain->removeEffect_l(effect) == 0) {
+        if (chain->removeEffect_l(effect, true) == 0) {
             ALOGV("updateOrphanEffectChains removing effect chain at index %d", index);
             mOrphanEffectChains.removeItemsAt(index);
         }
diff --git a/services/audioflinger/AudioFlinger.h b/services/audioflinger/AudioFlinger.h
index 08fa70d..3e9a088 100644
--- a/services/audioflinger/AudioFlinger.h
+++ b/services/audioflinger/AudioFlinger.h
@@ -593,6 +593,7 @@ private:
                 void        removeNotificationClient(pid_t pid);
                 bool isNonOffloadableGlobalEffectEnabled_l();
                 void onNonOffloadableGlobalEffectEnable();
+                bool isSessionAcquired_l(audio_session_t audioSession);
 
                 // Store an effect chain to mOrphanEffectChains keyed vector.
                 // Called when a thread exits and effects are still attached to it.
diff --git a/services/audioflinger/Effects.cpp b/services/audioflinger/Effects.cpp
index b9fe741..fb9e157 100644
--- a/services/audioflinger/Effects.cpp
+++ b/services/audioflinger/Effects.cpp
@@ -59,8 +59,9 @@ AudioFlinger::EffectModule::EffectModule(ThreadBase *thread,
                                         const wp<AudioFlinger::EffectChain>& chain,
                                         effect_descriptor_t *desc,
                                         int id,
-                                        int sessionId)
-    : mPinned(sessionId > AUDIO_SESSION_OUTPUT_MIX),
+                                        audio_session_t sessionId,
+                                        bool pinned)
+    : mPinned(pinned),
       mThread(thread), mChain(chain), mId(id), mSessionId(sessionId),
       mDescriptor(*desc),
       // mConfig is set by configure() and not used before then
@@ -71,7 +72,7 @@ AudioFlinger::EffectModule::EffectModule(ThreadBase *thread,
       mSuspended(false),
       mAudioFlinger(thread->mAudioFlinger)
 {
-    ALOGV("Constructor %p", this);
+    ALOGV("Constructor %p pinned %d", this, pinned);
     int lStatus;
 
     // create effect engine from effect factory
@@ -86,6 +87,8 @@ AudioFlinger::EffectModule::EffectModule(ThreadBase *thread,
         goto Error;
     }
 
+    setOffloaded(thread->type() == ThreadBase::OFFLOAD, thread->id());
+
     ALOGV("Constructor success name %s, Interface %p", mDescriptor.name, mEffectInterface);
     return;
 Error:
@@ -98,9 +101,8 @@ AudioFlinger::EffectModule::~EffectModule()
 {
     ALOGV("Destructor %p", this);
     if (mEffectInterface != NULL) {
-        remove_effect_from_hal_l();
-        // release effect engine
-        EffectRelease(mEffectInterface);
+        ALOGW("EffectModule %p destructor called with unreleased interface", this);
+        release_l();
     }
 }
 
@@ -115,7 +117,7 @@ status_t AudioFlinger::EffectModule::addHandle(EffectHandle *handle)
     size_t i;
     for (i = 0; i < size; i++) {
         EffectHandle *h = mHandles[i];
-        if (h == NULL || h->destroyed_l()) {
+        if (h == NULL || h->disconnected()) {
             continue;
         }
         // first non destroyed handle is considered in control
@@ -143,9 +145,14 @@ status_t AudioFlinger::EffectModule::addHandle(EffectHandle *handle)
     return status;
 }
 
-size_t AudioFlinger::EffectModule::removeHandle(EffectHandle *handle)
+ssize_t AudioFlinger::EffectModule::removeHandle(EffectHandle *handle)
 {
     Mutex::Autolock _l(mLock);
+    return removeHandle_l(handle);
+}
+
+ssize_t AudioFlinger::EffectModule::removeHandle_l(EffectHandle *handle)
+{
     size_t size = mHandles.size();
     size_t i;
     for (i = 0; i < size; i++) {
@@ -154,9 +161,10 @@ size_t AudioFlinger::EffectModule::removeHandle(EffectHandle *handle)
         }
     }
     if (i == size) {
-        return size;
+        ALOGW("%s %p handle not found %p", __FUNCTION__, this, handle);
+        return BAD_VALUE;
     }
-    ALOGV("removeHandle() %p removed handle %p in position %d", this, handle, i);
+    ALOGV("removeHandle_l() %p removed handle %p in position %zu", this, handle, i);
 
     mHandles.removeAt(i);
     // if removed from first place, move effect control from this handle to next in line
@@ -183,7 +191,7 @@ AudioFlinger::EffectHandle *AudioFlinger::EffectModule::controlHandle_l()
     // the first valid handle in the list has control over the module
     for (size_t i = 0; i < mHandles.size(); i++) {
         EffectHandle *h = mHandles[i];
-        if (h != NULL && !h->destroyed_l()) {
+        if (h != NULL && !h->disconnected()) {
             return h;
         }
     }
@@ -191,29 +199,22 @@ AudioFlinger::EffectHandle *AudioFlinger::EffectModule::controlHandle_l()
     return NULL;
 }
 
-size_t AudioFlinger::EffectModule::disconnect(EffectHandle *handle, bool unpinIfLast)
+// unsafe method called when the effect parent thread has been destroyed
+ssize_t AudioFlinger::EffectModule::disconnectHandle(EffectHandle *handle, bool unpinIfLast)
 {
     ALOGV("disconnect() %p handle %p", this, handle);
-    // keep a strong reference on this EffectModule to avoid calling the
-    // destructor before we exit
-    sp<EffectModule> keep(this);
-    {
-        if (removeHandle(handle) == 0) {
-            if (!isPinned() || unpinIfLast) {
-                sp<ThreadBase> thread = mThread.promote();
-                if (thread != 0) {
-                    Mutex::Autolock _l(thread->mLock);
-                    thread->removeEffect_l(this);
-                }
-                sp<AudioFlinger> af = mAudioFlinger.promote();
-                if (af != 0) {
-                    af->updateOrphanEffectChains(this);
-                }
-                AudioSystem::unregisterEffect(mId);
-            }
+    Mutex::Autolock _l(mLock);
+    ssize_t numHandles = removeHandle_l(handle);
+    if ((numHandles == 0) && (!mPinned || unpinIfLast)) {
+        AudioSystem::unregisterEffect(mId);
+        sp<AudioFlinger> af = mAudioFlinger.promote();
+        if (af != 0) {
+            mLock.unlock();
+            af->updateOrphanEffectChains(this);
+            mLock.lock();
         }
     }
-    return mHandles.size();
+    return numHandles;
 }
 
 void AudioFlinger::EffectModule::updateState() {
@@ -528,6 +529,17 @@ status_t AudioFlinger::EffectModule::stop_l()
     return status;
 }
 
+// must be called with EffectChain::mLock held
+void AudioFlinger::EffectModule::release_l()
+{
+    if (mEffectInterface != NULL) {
+        remove_effect_from_hal_l();
+        // release effect engine
+        EffectRelease(mEffectInterface);
+        mEffectInterface = NULL;
+    }
+}
+
 status_t AudioFlinger::EffectModule::remove_effect_from_hal_l()
 {
     if ((mDescriptor.flags & EFFECT_FLAG_TYPE_MASK) == EFFECT_FLAG_TYPE_PRE_PROC ||
@@ -620,7 +632,7 @@ status_t AudioFlinger::EffectModule::command(uint32_t cmdCode,
         uint32_t size = (replySize == NULL) ? 0 : *replySize;
         for (size_t i = 1; i < mHandles.size(); i++) {
             EffectHandle *h = mHandles[i];
-            if (h != NULL && !h->destroyed_l()) {
+            if (h != NULL && !h->disconnected()) {
                 h->commandExecuted(cmdCode, cmdSize, pCmdData, size, pReplyData);
             }
         }
@@ -673,7 +685,7 @@ status_t AudioFlinger::EffectModule::setEnabled_l(bool enabled)
         }
         for (size_t i = 1; i < mHandles.size(); i++) {
             EffectHandle *h = mHandles[i];
-            if (h != NULL && !h->destroyed_l()) {
+            if (h != NULL && !h->disconnected()) {
                 h->setEnabled(enabled);
             }
         }
@@ -838,8 +850,7 @@ bool AudioFlinger::EffectModule::purgeHandles()
     Mutex::Autolock _l(mLock);
     for (size_t i = 0; i < mHandles.size(); i++) {
         EffectHandle *handle = mHandles[i];
-        if (handle != NULL && !handle->destroyed_l()) {
-            handle->effect().clear();
+        if (handle != NULL && !handle->disconnected()) {
             if (handle->hasControl()) {
                 enabled = handle->enabled();
             }
@@ -1066,7 +1077,7 @@ void AudioFlinger::EffectModule::dump(int fd, const Vector<String16>& args __unu
     result.append("\t\t\t  Pid Priority Ctrl Locked client server\n");
     for (size_t i = 0; i < mHandles.size(); ++i) {
         EffectHandle *handle = mHandles[i];
-        if (handle != NULL && !handle->destroyed_l()) {
+        if (handle != NULL && !handle->disconnected()) {
             handle->dumpToBuffer(buffer, SIZE);
             result.append(buffer);
         }
@@ -1092,7 +1103,7 @@ AudioFlinger::EffectHandle::EffectHandle(const sp<EffectModule>& effect,
                                         int32_t priority)
     : BnEffect(),
     mEffect(effect), mEffectClient(effectClient), mClient(client), mCblk(NULL),
-    mPriority(priority), mHasControl(false), mEnabled(false), mDestroyed(false)
+    mPriority(priority), mHasControl(false), mEnabled(false), mDisconnected(false)
 {
     ALOGV("constructor %p", this);
 
@@ -1115,14 +1126,6 @@ AudioFlinger::EffectHandle::EffectHandle(const sp<EffectModule>& effect,
 AudioFlinger::EffectHandle::~EffectHandle()
 {
     ALOGV("Destructor %p", this);
-
-    if (mEffect == 0) {
-        mDestroyed = true;
-        return;
-    }
-    mEffect->lock();
-    mDestroyed = true;
-    mEffect->unlock();
     disconnect(false);
 }
 
@@ -1133,13 +1136,15 @@ status_t AudioFlinger::EffectHandle::initCheck()
 
 status_t AudioFlinger::EffectHandle::enable()
 {
+    AutoMutex _l(mLock);
     ALOGV("enable %p", this);
+    sp<EffectModule> effect = mEffect.promote();
+    if (effect == 0 || mDisconnected) {
+        return DEAD_OBJECT;
+    }
     if (!mHasControl) {
         return INVALID_OPERATION;
     }
-    if (mEffect == 0) {
-        return DEAD_OBJECT;
-    }
 
     if (mEnabled) {
         return NO_ERROR;
@@ -1147,20 +1152,20 @@ status_t AudioFlinger::EffectHandle::enable()
 
     mEnabled = true;
 
-    sp<ThreadBase> thread = mEffect->thread().promote();
+    sp<ThreadBase> thread = effect->thread().promote();
     if (thread != 0) {
-        thread->checkSuspendOnEffectEnabled(mEffect, true, mEffect->sessionId());
+        thread->checkSuspendOnEffectEnabled(effect, true, effect->sessionId());
     }
 
     // checkSuspendOnEffectEnabled() can suspend this same effect when enabled
-    if (mEffect->suspended()) {
+    if (effect->suspended()) {
         return NO_ERROR;
     }
 
-    status_t status = mEffect->setEnabled(true);
+    status_t status = effect->setEnabled(true);
     if (status != NO_ERROR) {
         if (thread != 0) {
-            thread->checkSuspendOnEffectEnabled(mEffect, false, mEffect->sessionId());
+            thread->checkSuspendOnEffectEnabled(effect, false, effect->sessionId());
         }
         mEnabled = false;
     } else {
@@ -1171,13 +1176,13 @@ status_t AudioFlinger::EffectHandle::enable()
                 Mutex::Autolock _l(t->mLock);
                 t->broadcast_l();
             }
-            if (!mEffect->isOffloadable()) {
+            if (!effect->isOffloadable()) {
                 if (thread->type() == ThreadBase::OFFLOAD ||
                    (thread->type() == ThreadBase::DIRECT && thread->mIsDirectPcm)) {
                     PlaybackThread *t = (PlaybackThread *)thread.get();
                     t->invalidateTracks(AUDIO_STREAM_MUSIC);
                 }
-                if (mEffect->sessionId() == AUDIO_SESSION_OUTPUT_MIX) {
+                if (effect->sessionId() == AUDIO_SESSION_OUTPUT_MIX) {
                     thread->mAudioFlinger->onNonOffloadableGlobalEffectEnable();
                 }
             }
@@ -1189,27 +1194,29 @@ status_t AudioFlinger::EffectHandle::enable()
 status_t AudioFlinger::EffectHandle::disable()
 {
     ALOGV("disable %p", this);
+    AutoMutex _l(mLock);
+    sp<EffectModule> effect = mEffect.promote();
+    if (effect == 0 || mDisconnected) {
+        return DEAD_OBJECT;
+    }
     if (!mHasControl) {
         return INVALID_OPERATION;
     }
-    if (mEffect == 0) {
-        return DEAD_OBJECT;
-    }
 
     if (!mEnabled) {
         return NO_ERROR;
     }
     mEnabled = false;
 
-    if (mEffect->suspended()) {
+    if (effect->suspended()) {
         return NO_ERROR;
     }
 
-    status_t status = mEffect->setEnabled(false);
+    status_t status = effect->setEnabled(false);
 
-    sp<ThreadBase> thread = mEffect->thread().promote();
+    sp<ThreadBase> thread = effect->thread().promote();
     if (thread != 0) {
-        thread->checkSuspendOnEffectEnabled(mEffect, false, mEffect->sessionId());
+        thread->checkSuspendOnEffectEnabled(effect, false, effect->sessionId());
         if ((thread->type() == ThreadBase::OFFLOAD) ||
             (thread->type() == ThreadBase::DIRECT && thread->mIsDirectPcm)){
             PlaybackThread *t = (PlaybackThread *)thread.get();
@@ -1223,25 +1230,39 @@ status_t AudioFlinger::EffectHandle::disable()
 
 void AudioFlinger::EffectHandle::disconnect()
 {
+    ALOGV("%s %p", __FUNCTION__, this);
     disconnect(true);
 }
 
 void AudioFlinger::EffectHandle::disconnect(bool unpinIfLast)
 {
-    ALOGV("disconnect(%s)", unpinIfLast ? "true" : "false");
-    if (mEffect == 0) {
+    AutoMutex _l(mLock);
+    ALOGV("disconnect(%s) %p", unpinIfLast ? "true" : "false", this);
+    if (mDisconnected) {
+        if (unpinIfLast) {
+            android_errorWriteLog(0x534e4554, "32707507");
+        }
         return;
     }
-    // restore suspended effects if the disconnected handle was enabled and the last one.
-    if ((mEffect->disconnect(this, unpinIfLast) == 0) && mEnabled) {
-        sp<ThreadBase> thread = mEffect->thread().promote();
-        if (thread != 0) {
-            thread->checkSuspendOnEffectEnabled(mEffect, false, mEffect->sessionId());
+    mDisconnected = true;
+    sp<ThreadBase> thread;
+    {
+        sp<EffectModule> effect = mEffect.promote();
+        if (effect != 0) {
+            thread = effect->thread().promote();
+        }
+    }
+    if (thread != 0) {
+        thread->disconnectEffectHandle(this, unpinIfLast);
+    } else {
+        ALOGW("%s Effect handle %p disconnected after thread destruction", __FUNCTION__, this);
+        // try to cleanup as much as we can
+        sp<EffectModule> effect = mEffect.promote();
+        if (effect != 0) {
+            effect->disconnectHandle(this, unpinIfLast);
         }
     }
 
-    // release sp on module => module destructor can be called now
-    mEffect.clear();
     if (mClient != 0) {
         if (mCblk != NULL) {
             // unlike ~TrackBase(), mCblk is never a local new, so don't delete
@@ -1261,15 +1282,17 @@ status_t AudioFlinger::EffectHandle::command(uint32_t cmdCode,
                                              void *pReplyData)
 {
     ALOGVV("command(), cmdCode: %d, mHasControl: %d, mEffect: %p",
-            cmdCode, mHasControl, (mEffect == 0) ? 0 : mEffect.get());
+            cmdCode, mHasControl, mEffect.unsafe_get());
 
+    AutoMutex _l(mLock);
+    sp<EffectModule> effect = mEffect.promote();
+    if (effect == 0 || mDisconnected) {
+        return DEAD_OBJECT;
+    }
     // only get parameter command is permitted for applications not controlling the effect
     if (!mHasControl && cmdCode != EFFECT_CMD_GET_PARAM) {
         return INVALID_OPERATION;
     }
-    if (mEffect == 0) {
-        return DEAD_OBJECT;
-    }
     if (mClient == 0) {
         return INVALID_OPERATION;
     }
@@ -1314,7 +1337,7 @@ status_t AudioFlinger::EffectHandle::command(uint32_t cmdCode,
 
             int reply = 0;
             uint32_t rsize = sizeof(reply);
-            status_t ret = mEffect->command(EFFECT_CMD_SET_PARAM,
+            status_t ret = effect->command(EFFECT_CMD_SET_PARAM,
                                             size,
                                             param,
                                             &rsize,
@@ -1351,7 +1374,7 @@ status_t AudioFlinger::EffectHandle::command(uint32_t cmdCode,
         return disable();
     }
 
-    return mEffect->command(cmdCode, cmdSize, pCmdData, replySize, pReplyData);
+    return effect->command(cmdCode, cmdSize, pCmdData, replySize, pReplyData);
 }
 
 void AudioFlinger::EffectHandle::setControl(bool hasControl, bool signal, bool enabled)
@@ -1433,7 +1456,6 @@ AudioFlinger::EffectChain::~EffectChain()
     if (mOwnInBuffer) {
         delete mInBuffer;
     }
-
 }
 
 // getEffectFromDesc_l() must be called with ThreadBase::mLock held
@@ -1547,13 +1569,38 @@ void AudioFlinger::EffectChain::process_l()
     }
 }
 
-// addEffect_l() must be called with PlaybackThread::mLock held
+// createEffect_l() must be called with ThreadBase::mLock held
+status_t AudioFlinger::EffectChain::createEffect_l(sp<EffectModule>& effect,
+                                                   ThreadBase *thread,
+                                                   effect_descriptor_t *desc,
+                                                   int id,
+                                                   audio_session_t sessionId,
+                                                   bool pinned)
+{
+    Mutex::Autolock _l(mLock);
+    effect = new EffectModule(thread, this, desc, id, sessionId, pinned);
+    status_t lStatus = effect->status();
+    if (lStatus == NO_ERROR) {
+        lStatus = addEffect_ll(effect);
+    }
+    if (lStatus != NO_ERROR) {
+        effect.clear();
+    }
+    return lStatus;
+}
+
+// addEffect_l() must be called with ThreadBase::mLock held
 status_t AudioFlinger::EffectChain::addEffect_l(const sp<EffectModule>& effect)
 {
+    Mutex::Autolock _l(mLock);
+    return addEffect_ll(effect);
+}
+// addEffect_l() must be called with ThreadBase::mLock and EffectChain::mLock held
+status_t AudioFlinger::EffectChain::addEffect_ll(const sp<EffectModule>& effect)
+{
     effect_descriptor_t desc = effect->desc();
     uint32_t insertPref = desc.flags & EFFECT_FLAG_INSERT_MASK;
 
-    Mutex::Autolock _l(mLock);
     effect->setChain(this);
     sp<ThreadBase> thread = mThread.promote();
     if (thread == 0) {
@@ -1663,8 +1710,9 @@ status_t AudioFlinger::EffectChain::addEffect_l(const sp<EffectModule>& effect)
     return NO_ERROR;
 }
 
-// removeEffect_l() must be called with PlaybackThread::mLock held
-size_t AudioFlinger::EffectChain::removeEffect_l(const sp<EffectModule>& effect)
+// removeEffect_l() must be called with ThreadBase::mLock held
+size_t AudioFlinger::EffectChain::removeEffect_l(const sp<EffectModule>& effect,
+                                                 bool release)
 {
     Mutex::Autolock _l(mLock);
     size_t size = mEffects.size();
@@ -1679,6 +1727,10 @@ size_t AudioFlinger::EffectChain::removeEffect_l(const sp<EffectModule>& effect)
                     mEffects[i]->state() == EffectModule::STOPPING) {
                 mEffects[i]->stop();
             }
+            if (release) {
+                mEffects[i]->release_l();
+            }
+
             if (type == EFFECT_FLAG_TYPE_AUXILIARY) {
                 delete[] effect->inBuffer();
             } else {
@@ -1697,7 +1749,7 @@ size_t AudioFlinger::EffectChain::removeEffect_l(const sp<EffectModule>& effect)
     return mEffects.size();
 }
 
-// setDevice_l() must be called with PlaybackThread::mLock held
+// setDevice_l() must be called with ThreadBase::mLock held
 void AudioFlinger::EffectChain::setDevice_l(audio_devices_t device)
 {
     size_t size = mEffects.size();
@@ -1706,7 +1758,7 @@ void AudioFlinger::EffectChain::setDevice_l(audio_devices_t device)
     }
 }
 
-// setMode_l() must be called with PlaybackThread::mLock held
+// setMode_l() must be called with ThreadBase::mLock held
 void AudioFlinger::EffectChain::setMode_l(audio_mode_t mode)
 {
     size_t size = mEffects.size();
@@ -1715,7 +1767,7 @@ void AudioFlinger::EffectChain::setMode_l(audio_mode_t mode)
     }
 }
 
-// setAudioSource_l() must be called with PlaybackThread::mLock held
+// setAudioSource_l() must be called with ThreadBase::mLock held
 void AudioFlinger::EffectChain::setAudioSource_l(audio_source_t source)
 {
     size_t size = mEffects.size();
@@ -1724,7 +1776,7 @@ void AudioFlinger::EffectChain::setAudioSource_l(audio_source_t source)
     }
 }
 
-// setVolume_l() must be called with PlaybackThread::mLock held
+// setVolume_l() must be called with ThreadBase::mLock held
 bool AudioFlinger::EffectChain::setVolume_l(uint32_t *left, uint32_t *right)
 {
     uint32_t newLeft = *left;
@@ -1876,7 +1928,7 @@ void AudioFlinger::EffectChain::setEffectSuspended_l(
                     effect->setSuspended(false);
                     effect->lock();
                     EffectHandle *handle = effect->controlHandle_l();
-                    if (handle != NULL && !handle->destroyed_l()) {
+                    if (handle != NULL && !handle->disconnected()) {
                         effect->setEnabled_l(handle->enabled());
                     }
                     effect->unlock();
diff --git a/services/audioflinger/Effects.h b/services/audioflinger/Effects.h
index 6f93f81..4d53c0d 100644
--- a/services/audioflinger/Effects.h
+++ b/services/audioflinger/Effects.h
@@ -45,7 +45,8 @@ public:
                     const wp<AudioFlinger::EffectChain>& chain,
                     effect_descriptor_t *desc,
                     int id,
-                    int sessionId);
+                    audio_session_t sessionId,
+                    bool pinned);
     virtual ~EffectModule();
 
     enum effect_state {
@@ -93,8 +94,9 @@ public:
     const wp<ThreadBase>& thread() { return mThread; }
 
     status_t addHandle(EffectHandle *handle);
-    size_t disconnect(EffectHandle *handle, bool unpinIfLast);
-    size_t removeHandle(EffectHandle *handle);
+    ssize_t  disconnectHandle(EffectHandle *handle, bool unpinIfLast);
+    ssize_t removeHandle(EffectHandle *handle);
+    ssize_t removeHandle_l(EffectHandle *handle);
 
     const effect_descriptor_t& desc() const { return mDescriptor; }
     wp<EffectChain>&     chain() { return mChain; }
@@ -120,6 +122,7 @@ public:
     status_t         setOffloaded(bool offloaded, audio_io_handle_t io);
     bool             isOffloaded() const;
     void             addEffectToHal_l();
+    void             release_l();
 
     void             dump(int fd, const Vector<String16>& args);
 
@@ -204,12 +207,17 @@ public:
     bool enabled() const { return mEnabled; }
 
     // Getters
-    int id() const { return mEffect->id(); }
+    wp<EffectModule> effect() const { return mEffect; }
+    int id() const {
+        sp<EffectModule> effect = mEffect.promote();
+        if (effect == 0) {
+            return 0;
+        }
+        return effect->id();
+    }
     int priority() const { return mPriority; }
     bool hasControl() const { return mHasControl; }
-    sp<EffectModule> effect() const { return mEffect; }
-    // destroyed_l() must be called with the associated EffectModule mLock held
-    bool destroyed_l() const { return mDestroyed; }
+    bool disconnected() const { return mDisconnected; }
 
     void dumpToBuffer(char* buffer, size_t size);
 
@@ -218,7 +226,8 @@ protected:
     EffectHandle(const EffectHandle&);
     EffectHandle& operator =(const EffectHandle&);
 
-    sp<EffectModule> mEffect;           // pointer to controlled EffectModule
+    Mutex mLock;                        // protects IEffect method calls
+    wp<EffectModule> mEffect;           // pointer to controlled EffectModule
     sp<IEffectClient> mEffectClient;    // callback interface for client notifications
     /*const*/ sp<Client> mClient;       // client for shared memory allocation, see disconnect()
     sp<IMemory>         mCblkMemory;    // shared memory for control block
@@ -229,8 +238,7 @@ protected:
     bool mHasControl;                   // true if this handle is controlling the effect
     bool mEnabled;                      // cached enable state: needed when the effect is
                                         // restored after being suspended
-    bool mDestroyed;                    // Set to true by destructor. Access with EffectModule
-                                        // mLock held
+    bool mDisconnected;                 // Set to true by disconnect()
 };
 
 // the EffectChain class represents a group of effects associated to one audio session.
@@ -263,8 +271,15 @@ public:
         mLock.unlock();
     }
 
+    status_t createEffect_l(sp<EffectModule>& effect,
+                            ThreadBase *thread,
+                            effect_descriptor_t *desc,
+                            int id,
+                            audio_session_t sessionId,
+                            bool pinned);
     status_t addEffect_l(const sp<EffectModule>& handle);
-    size_t removeEffect_l(const sp<EffectModule>& handle);
+    status_t addEffect_ll(const sp<EffectModule>& handle);
+    size_t removeEffect_l(const sp<EffectModule>& handle, bool release = false);
 
     int sessionId() const { return mSessionId; }
     void setSessionId(int sessionId) { mSessionId = sessionId; }
diff --git a/services/audioflinger/Threads.cpp b/services/audioflinger/Threads.cpp
index e5e8bdb..d7c9c47 100644
--- a/services/audioflinger/Threads.cpp
+++ b/services/audioflinger/Threads.cpp
@@ -1159,7 +1159,8 @@ sp<AudioFlinger::EffectHandle> AudioFlinger::ThreadBase::createEffect_l(
         int sessionId,
         effect_descriptor_t *desc,
         int *enabled,
-        status_t *status)
+        status_t *status,
+        bool pinned)
 {
     sp<EffectModule> effect;
     sp<EffectHandle> handle;
@@ -1253,20 +1254,7 @@ sp<AudioFlinger::EffectHandle> AudioFlinger::ThreadBase::createEffect_l(
             }
             effectRegistered = true;
             // create a new effect module if none present in the chain
-            effect = new EffectModule(this, chain, desc, id, sessionId);
-            lStatus = effect->status();
-            if (lStatus != NO_ERROR) {
-                goto Exit;
-            }
-
-            bool setVal = false;
-            if (mType == OFFLOAD || (mType == DIRECT && mIsDirectPcm)) {
-                setVal = true;
-            }
-
-            effect->setOffloaded(setVal, mId);
-
-            lStatus = chain->addEffect_l(effect);
+            lStatus = chain->createEffect_l(effect, this, desc, id, (audio_session_t)sessionId, pinned);
             if (lStatus != NO_ERROR) {
                 goto Exit;
             }
@@ -1307,6 +1295,33 @@ Exit:
     return handle;
 }
 
+void AudioFlinger::ThreadBase::disconnectEffectHandle(EffectHandle *handle,
+                                                      bool unpinIfLast)
+{
+    bool remove = false;
+    sp<EffectModule> effect;
+    {
+        Mutex::Autolock _l(mLock);
+
+        effect = handle->effect().promote();
+        if (effect == 0) {
+            return;
+        }
+        // restore suspended effects if the disconnected handle was enabled and the last one.
+        remove = (effect->removeHandle(handle) == 0) && (!effect->isPinned() || unpinIfLast);
+        if (remove) {
+            removeEffect_l(effect, true);
+        }
+    }
+    if (remove) {
+        mAudioFlinger->updateOrphanEffectChains(effect);
+        AudioSystem::unregisterEffect(effect->id());
+        if (handle->enabled()) {
+            checkSuspendOnEffectEnabled(effect, false, effect->sessionId());
+        }
+    }
+}
+
 sp<AudioFlinger::EffectModule> AudioFlinger::ThreadBase::getEffect(int sessionId, int effectId)
 {
     Mutex::Autolock _l(mLock);
@@ -1371,9 +1386,9 @@ status_t AudioFlinger::ThreadBase::addEffect_l(const sp<EffectModule>& effect)
     return NO_ERROR;
 }
 
-void AudioFlinger::ThreadBase::removeEffect_l(const sp<EffectModule>& effect) {
+void AudioFlinger::ThreadBase::removeEffect_l(const sp<EffectModule>& effect, bool release) {
 
-    ALOGV("removeEffect_l() %p effect %p", this, effect.get());
+    ALOGV("%s %p effect %p", __FUNCTION__, this, effect.get());
     effect_descriptor_t desc = effect->desc();
     if ((desc.flags & EFFECT_FLAG_TYPE_MASK) == EFFECT_FLAG_TYPE_AUXILIARY) {
         detachAuxEffect_l(effect->id());
@@ -1382,7 +1397,7 @@ void AudioFlinger::ThreadBase::removeEffect_l(const sp<EffectModule>& effect) {
     sp<EffectChain> chain = effect->chain().promote();
     if (chain != 0) {
         // remove effect chain if removing last effect
-        if (chain->removeEffect_l(effect) == 0) {
+        if (chain->removeEffect_l(effect, release) == 0) {
             removeEffectChain_l(chain);
         }
     } else {
diff --git a/services/audioflinger/Threads.h b/services/audioflinger/Threads.h
index 8fab1e4..4009c18 100644
--- a/services/audioflinger/Threads.h
+++ b/services/audioflinger/Threads.h
@@ -309,7 +309,8 @@ public:
                                     int sessionId,
                                     effect_descriptor_t *desc,
                                     int *enabled,
-                                    status_t *status /*non-NULL*/);
+                                    status_t *status /*non-NULL*/,
+                                    bool pinned);
 
                 // return values for hasAudioSession (bit field)
                 enum effect_state {
@@ -346,7 +347,9 @@ public:
                 status_t addEffect_l(const sp< EffectModule>& effect);
                 // remove and effect module. Also removes the effect chain is this was the last
                 // effect
-                void removeEffect_l(const sp< EffectModule>& effect);
+                void removeEffect_l(const sp< EffectModule>& effect, bool release = false);
+                // disconnect an effect handle from module and destroy module if last handle
+                void disconnectEffectHandle(EffectHandle *handle, bool unpinIfLast);
                 // detach all tracks connected to an auxiliary effect
     virtual     void detachAuxEffect_l(int effectId __unused) {}
                 // returns either EFFECT_SESSION if effects on this audio session exist in one
diff --git a/services/audiopolicy/service/AudioPolicyInterfaceImpl.cpp b/services/audiopolicy/service/AudioPolicyInterfaceImpl.cpp
index b23c35e..45b3bb0 100644
--- a/services/audiopolicy/service/AudioPolicyInterfaceImpl.cpp
+++ b/services/audiopolicy/service/AudioPolicyInterfaceImpl.cpp
@@ -417,7 +417,6 @@ void AudioPolicyService::releaseInput(audio_io_handle_t input,
     sp<AudioPolicyEffects>audioPolicyEffects;
     {
         Mutex::Autolock _l(mLock);
-        mAudioPolicyManager->releaseInput(input, session);
         audioPolicyEffects = mAudioPolicyEffects;
     }
     if (audioPolicyEffects != 0) {
@@ -427,6 +426,10 @@ void AudioPolicyService::releaseInput(audio_io_handle_t input,
             ALOGW("Failed to release effects on input %d", input);
         }
     }
+    {
+        Mutex::Autolock _l(mLock);
+        mAudioPolicyManager->releaseInput(input, session);
+    }
 }
 
 status_t AudioPolicyService::initStreamVolume(audio_stream_type_t stream,