From dd91ce21677b5dbb42e9d5ecf25c9db66616211e Mon Sep 17 00:00:00 2001
From: Mikhail Naganov <mnaganov@google.com>
Date: Mon, 13 Jan 2020 11:34:30 -0800
Subject: [PATCH] PatchPanel: Fix crash when tearing down "pass thru" software
 patch

The pass thru software patch is normally used with the MSD module.

There is no need to establish a peer reference from PatchRecord
because its I/O code only gets executed by PatchTrack
initiative. In fact, this also means PassthroughPatchRecord
can't be stopped synchronously, so the usual software patch tear
down logic in PatchPanel leads to a null pointer dereference
because it races with PatchTrack I/O activity on a separate
thread.

Fix by skipping the sp<> setup / clear logic in PatchPanel
for PassthroughPatchRecord.

Bug: 147599144
Test: force teardown of MSD patch, check logcat for crash
Change-Id: I4a2abc16ad705244767b33ea529e1ace2213d19f
---
 services/audioflinger/PatchPanel.cpp | 14 +++++++++++---
 services/audioflinger/PatchPanel.h   |  9 ++++++---
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/services/audioflinger/PatchPanel.cpp b/services/audioflinger/PatchPanel.cpp
index ee5bd7553c..51ed79739d 100644
--- a/services/audioflinger/PatchPanel.cpp
+++ b/services/audioflinger/PatchPanel.cpp
@@ -490,10 +490,12 @@ status_t AudioFlinger::PatchPanel::Patch::createConnections(PatchPanel *panel)
     }
 
     sp<RecordThread::PatchRecord> tempRecordTrack;
+    const bool usePassthruPatchRecord =
+            (inputFlags & AUDIO_INPUT_FLAG_DIRECT) && (outputFlags & AUDIO_OUTPUT_FLAG_DIRECT);
     const size_t playbackFrameCount = mPlayback.thread()->frameCount();
     const size_t recordFrameCount = mRecord.thread()->frameCount();
     size_t frameCount = 0;
-    if ((inputFlags & AUDIO_INPUT_FLAG_DIRECT) && (outputFlags & AUDIO_OUTPUT_FLAG_DIRECT)) {
+    if (usePassthruPatchRecord) {
         // PassthruPatchRecord producesBufferOnDemand, so use
         // maximum of playback and record thread framecounts
         frameCount = std::max(playbackFrameCount, recordFrameCount);
@@ -550,8 +552,14 @@ status_t AudioFlinger::PatchPanel::Patch::createConnections(PatchPanel *panel)
     }
 
     // tie playback and record tracks together
-    mRecord.setTrackAndPeer(tempRecordTrack, tempPatchTrack);
-    mPlayback.setTrackAndPeer(tempPatchTrack, tempRecordTrack);
+    // In the case of PassthruPatchRecord no I/O activity happens on RecordThread,
+    // everything is driven from PlaybackThread. Thus AudioBufferProvider methods
+    // of PassthruPatchRecord can only be called if the corresponding PatchTrack
+    // is alive. There is no need to hold a reference, and there is no need
+    // to clear it. In fact, since playback stopping is asynchronous, there is
+    // no proper time when clearing could be done.
+    mRecord.setTrackAndPeer(tempRecordTrack, tempPatchTrack, !usePassthruPatchRecord);
+    mPlayback.setTrackAndPeer(tempPatchTrack, tempRecordTrack, true /*holdReference*/);
 
     // start capture and playback
     mRecord.track()->start(AudioSystem::SYNC_EVENT_NONE, AUDIO_SESSION_NONE);
diff --git a/services/audioflinger/PatchPanel.h b/services/audioflinger/PatchPanel.h
index 181e27c2ea..689fdfec74 100644
--- a/services/audioflinger/PatchPanel.h
+++ b/services/audioflinger/PatchPanel.h
@@ -123,18 +123,20 @@ private:
             mCloseThread = closeThread;
         }
         template <typename T>
-        void setTrackAndPeer(const sp<TrackType>& track, const sp<T> &peer) {
+        void setTrackAndPeer(const sp<TrackType>& track, const sp<T> &peer, bool holdReference) {
             mTrack = track;
             mThread->addPatchTrack(mTrack);
-            mTrack->setPeerProxy(peer, true /* holdReference */);
+            mTrack->setPeerProxy(peer, holdReference);
+            mClearPeerProxy = holdReference;
         }
-        void clearTrackPeer() { if (mTrack) mTrack->clearPeerProxy(); }
+        void clearTrackPeer() { if (mClearPeerProxy && mTrack) mTrack->clearPeerProxy(); }
         void stopTrack() { if (mTrack) mTrack->stop(); }
 
         void swap(Endpoint &other) noexcept {
             using std::swap;
             swap(mThread, other.mThread);
             swap(mCloseThread, other.mCloseThread);
+            swap(mClearPeerProxy, other.mClearPeerProxy);
             swap(mHandle, other.mHandle);
             swap(mTrack, other.mTrack);
         }
@@ -146,6 +148,7 @@ private:
     private:
         sp<ThreadType> mThread;
         bool mCloseThread = true;
+        bool mClearPeerProxy = true;
         audio_patch_handle_t mHandle = AUDIO_PATCH_HANDLE_NONE;
         sp<TrackType> mTrack;
     };