commit 0f8ab670c09988da64732a09d3a67d913e458900
author Eric Laurent <elaurent@google.com> Thu Sep 17 05:12:56 2009 -0700
committer Eric Laurent <elaurent@google.com> Thu Sep 17 09:26:04 2009 -0700
tree 80e59c55884ab0d53778829ebfafe1898dd37eeb
parent 5140a13b5eff0b9ba89cb954ba645ca468257548

Fix issue 2127371: Possible race condition in AudioFlinger::openRecord() when a Track is being destroyed.

The fix consists in locking AudioFlinger::mLock mutex in the TrackBase destructor before clearing the strong pointer to the shared memory client. The mutex is not locked in removeclient() any more which implies that we must make sure that the Client destructor is always called from the TrackBase destructor or that we hold the mLock mutex before calling deleting the Client.

libs/audioflinger/AudioFlinger.cpp

diff --git a/libs/audioflinger/AudioFlinger.cpp b/libs/audioflinger/AudioFlinger.cpp
index add358b..e534447 100644
--- a/libs/audioflinger/AudioFlinger.cpp
+++ b/libs/audioflinger/AudioFlinger.cpp
@@ -307,6 +307,9 @@
     if (lStatus == NO_ERROR) {
         trackHandle = new TrackHandle(track);
     } else {
+        // remove local strong reference to Client before deleting the Track so that the Client
+        // destructor is called by the TrackBase destructor with mLock held
+        client.clear();
         track.clear();
     }
 
@@ -707,10 +710,10 @@
     }
 }
 
-void AudioFlinger::removeClient(pid_t pid)
+// removeClient_l() must be called with AudioFlinger::mLock held
+void AudioFlinger::removeClient_l(pid_t pid)
 {
-    LOGV("removeClient() pid %d, tid %d, calling tid %d", pid, gettid(), IPCThreadState::self()->getCallingPid());
-    Mutex::Autolock _l(mLock);
+    LOGV("removeClient_l() pid %d, tid %d, calling tid %d", pid, gettid(), IPCThreadState::self()->getCallingPid());
     mClients.removeItem(pid);
 }
 
@@ -2078,7 +2081,10 @@
         }
     }
     mCblkMemory.clear();            // and free the shared memory
-    mClient.clear();
+    if (mClient != NULL) {
+        Mutex::Autolock _l(mClient->audioFlinger()->mLock);
+        mClient.clear();
+    }
 }
 
 void AudioFlinger::ThreadBase::TrackBase::releaseBuffer(AudioBufferProvider::Buffer* buffer)
@@ -2712,9 +2718,10 @@
     // 1 MB of address space is good for 32 tracks, 8 buffers each, 4 KB/buffer
 }
 
+// Client destructor must be called with AudioFlinger::mLock held
 AudioFlinger::Client::~Client()
 {
-    mAudioFlinger->removeClient(mPid);
+    mAudioFlinger->removeClient_l(mPid);
 }
 
 const sp<MemoryDealer>& AudioFlinger::Client::heap() const
@@ -2820,6 +2827,9 @@
                                                    format, channelCount, frameCount, flags);
     }
     if (recordTrack->getCblk() == NULL) {
+        // remove local strong reference to Client before deleting the RecordTrack so that the Client
+        // destructor is called by the TrackBase destructor with mLock held
+        client.clear();
         recordTrack.clear();
         lStatus = NO_MEMORY;
         goto Exit;