From f7e3a3a2eb753da4500b9000c113ae0f3b058792 Mon Sep 17 00:00:00 2001
From: Mikhail Naganov <mnaganov@google.com>
Date: Mon, 22 Apr 2019 16:43:26 -0700
Subject: audioflinger: Fix reference counting protocol in OpPlayAudioMonitor

OpPlayAudioMonitor was constructing a weak pointer to itself
in the constructor. This practice can lead to crashes due to
race conditions vs object destruction. This code is now moved
to onFirstRef method which is called when at least one strong
reference exists.

This change also reduces the number of created OpPlayAudioMonitor
objects by using a factory method.

Bug: 130038586
Test: enable / disable DND mode
Change-Id: I22e63a883ebaa25b9c96e79271bb9693b5ed75cd
---
 services/audioflinger/PlaybackTracks.h | 11 +++++++--
 services/audioflinger/Tracks.cpp       | 43 ++++++++++++++++++++++------------
 2 files changed, 37 insertions(+), 17 deletions(-)

(limited to 'services/audioflinger')

diff --git a/services/audioflinger/PlaybackTracks.h b/services/audioflinger/PlaybackTracks.h
index 4fd72a7917..56be433d4b 100644
--- a/services/audioflinger/PlaybackTracks.h
+++ b/services/audioflinger/PlaybackTracks.h
@@ -22,11 +22,16 @@
 // Checks and monitors OP_PLAY_AUDIO
 class OpPlayAudioMonitor : public RefBase {
 public:
-    OpPlayAudioMonitor(uid_t uid, audio_usage_t usage, int id, audio_stream_type_t streamType);
     ~OpPlayAudioMonitor() override;
     bool hasOpPlayAudio() const;
 
+    static sp<OpPlayAudioMonitor> createIfNeeded(
+            uid_t uid, audio_usage_t usage, int id, audio_stream_type_t streamType);
+
 private:
+    OpPlayAudioMonitor(uid_t uid, audio_usage_t usage, int id);
+    void onFirstRef() override;
+
     AppOpsManager mAppOpsManager;
 
     class PlayAudioOpCallback : public BnAppOpsCallback {
@@ -209,7 +214,9 @@ public:
 
     int fastIndex() const { return mFastIndex; }
 
-    bool isPlaybackRestricted() const { return !mOpPlayAudioMonitor->hasOpPlayAudio(); }
+    bool isPlaybackRestricted() const {
+        // The monitor is only created for tracks that can be silenced.
+        return mOpPlayAudioMonitor ? !mOpPlayAudioMonitor->hasOpPlayAudio() : false; }
 
 protected:
 
diff --git a/services/audioflinger/Tracks.cpp b/services/audioflinger/Tracks.cpp
index 2ff80c65ea..8d59431a65 100644
--- a/services/audioflinger/Tracks.cpp
+++ b/services/audioflinger/Tracks.cpp
@@ -381,26 +381,28 @@ status_t AudioFlinger::TrackHandle::onTransact(
 // ----------------------------------------------------------------------------
 //      AppOp for audio playback
 // -------------------------------
-AudioFlinger::PlaybackThread::OpPlayAudioMonitor::OpPlayAudioMonitor(uid_t uid, audio_usage_t usage,
-        int id, audio_stream_type_t streamType)
-            : mHasOpPlayAudio(true), mUid(uid), mUsage((int32_t) usage), mId(id)
+
+// static
+sp<AudioFlinger::PlaybackThread::OpPlayAudioMonitor>
+AudioFlinger::PlaybackThread::OpPlayAudioMonitor::createIfNeeded(
+            uid_t uid, audio_usage_t usage, int id, audio_stream_type_t streamType)
 {
     if (isAudioServerOrRootUid(uid)) {
-        ALOGD("OpPlayAudio: not muting track:%d usage:%d root or audioserver", mId, usage);
-        return;
+        ALOGD("OpPlayAudio: not muting track:%d usage:%d root or audioserver", id, usage);
+        return nullptr;
     }
     // stream type has been filtered by audio policy to indicate whether it can be muted
     if (streamType == AUDIO_STREAM_ENFORCED_AUDIBLE) {
-        ALOGD("OpPlayAudio: not muting track:%d usage:%d ENFORCED_AUDIBLE", mId, usage);
-        return;
-    }
-    PermissionController permissionController;
-    permissionController.getPackagesForUid(uid, mPackages);
-    checkPlayAudioForUsage();
-    if (!mPackages.isEmpty()) {
-        mOpCallback = new PlayAudioOpCallback(this);
-        mAppOpsManager.startWatchingMode(AppOpsManager::OP_PLAY_AUDIO, mPackages[0], mOpCallback);
+        ALOGD("OpPlayAudio: not muting track:%d usage:%d ENFORCED_AUDIBLE", id, usage);
+        return nullptr;
     }
+    return new OpPlayAudioMonitor(uid, usage, id);
+}
+
+AudioFlinger::PlaybackThread::OpPlayAudioMonitor::OpPlayAudioMonitor(
+        uid_t uid, audio_usage_t usage, int id)
+        : mHasOpPlayAudio(true), mUid(uid), mUsage((int32_t) usage), mId(id)
+{
 }
 
 AudioFlinger::PlaybackThread::OpPlayAudioMonitor::~OpPlayAudioMonitor()
@@ -411,6 +413,17 @@ AudioFlinger::PlaybackThread::OpPlayAudioMonitor::~OpPlayAudioMonitor()
     mOpCallback.clear();
 }
 
+void AudioFlinger::PlaybackThread::OpPlayAudioMonitor::onFirstRef()
+{
+    PermissionController permissionController;
+    permissionController.getPackagesForUid(mUid, mPackages);
+    checkPlayAudioForUsage();
+    if (!mPackages.isEmpty()) {
+        mOpCallback = new PlayAudioOpCallback(this);
+        mAppOpsManager.startWatchingMode(AppOpsManager::OP_PLAY_AUDIO, mPackages[0], mOpCallback);
+    }
+}
+
 bool AudioFlinger::PlaybackThread::OpPlayAudioMonitor::hasOpPlayAudio() const {
     return mHasOpPlayAudio.load();
 }
@@ -492,7 +505,7 @@ AudioFlinger::PlaybackThread::Track::Track(
     mPresentationCompleteFrames(0),
     mFrameMap(16 /* sink-frame-to-track-frame map memory */),
     mVolumeHandler(new media::VolumeHandler(sampleRate)),
-    mOpPlayAudioMonitor(new OpPlayAudioMonitor(uid, attr.usage, id(), streamType)),
+    mOpPlayAudioMonitor(OpPlayAudioMonitor::createIfNeeded(uid, attr.usage, id(), streamType)),
     // mSinkTimestamp
     mFastIndex(-1),
     mCachedVolume(1.0),
-- 
cgit v1.2.3