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
diff --git a/services/audioflinger/PatchPanel.cpp b/services/audioflinger/PatchPanel.cpp
index ee5bd75..51ed797 100644
--- a/services/audioflinger/PatchPanel.cpp
+++ b/services/audioflinger/PatchPanel.cpp
@@ -490,10 +490,12 @@
}
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 @@
}
// 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 181e27c..689fdfe 100644
--- a/services/audioflinger/PatchPanel.h
+++ b/services/audioflinger/PatchPanel.h
@@ -123,18 +123,20 @@
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:
sp<ThreadType> mThread;
bool mCloseThread = true;
+ bool mClearPeerProxy = true;
audio_patch_handle_t mHandle = AUDIO_PATCH_HANDLE_NONE;
sp<TrackType> mTrack;
};