MediaPlayerService: fix access of mPlayer in client

Test: poc doesn't crash
Bug: 38234812
Change-Id: I6f9be046ff66d2d5bed27bd712287e4ead550830
(cherry picked from commit 502c2f405355c3253990ac4edae345ac1907f916)
CVE-2017-0770

1 files changed, 34 insertions, 8 deletions

diff --git a/media/libmediaplayerservice/MediaPlayerService.cpp b/media/libmediaplayerservice/MediaPlayerService.cpp
index 61afe99404..a2ec69182e 100644
--- a/media/libmediaplayerservice/MediaPlayerService.cpp
+++ b/media/libmediaplayerservice/MediaPlayerService.cpp
@@ -84,6 +84,9 @@
 #include "HTTPBase.h"
 #include "RemoteDisplay.h"
 
+static const int kDumpLockRetries = 50;
+static const int kDumpLockSleepUs = 20000;
+
 namespace {
 using android::media::Metadata;
 using android::status_t;
@@ -417,12 +420,32 @@ status_t MediaPlayerService::Client::dump(int fd, const Vector<String16>& args)
     snprintf(buffer, 255, "  pid(%d), connId(%d), status(%d), looping(%s)\n",
             mPid, mConnId, mStatus, mLoop?"true": "false");
     result.append(buffer);
+
+    sp<MediaPlayerBase> p;
+    sp<AudioOutput> audioOutput;
+    bool locked = false;
+    for (int i = 0; i < kDumpLockRetries; ++i) {
+        if (mLock.tryLock() == NO_ERROR) {
+            locked = true;
+            break;
+        }
+        usleep(kDumpLockSleepUs);
+    }
+
+    if (locked) {
+        p = mPlayer;
+        audioOutput = mAudioOutput;
+        mLock.unlock();
+    } else {
+        result.append("  lock is taken, no dump from player and audio output\n");
+    }
     write(fd, result.string(), result.size());
-    if (mPlayer != NULL) {
-        mPlayer->dump(fd, args);
+
+    if (p != NULL) {
+        p->dump(fd, args);
     }
-    if (mAudioOutput != 0) {
-        mAudioOutput->dump(fd, args);
+    if (audioOutput != 0) {
+        audioOutput->dump(fd, args);
     }
     write(fd, "\n", 1);
     return NO_ERROR;
@@ -591,7 +614,10 @@ MediaPlayerService::Client::Client(
 MediaPlayerService::Client::~Client()
 {
     ALOGV("Client(%d) destructor pid = %d", mConnId, mPid);
-    mAudioOutput.clear();
+    {
+        Mutex::Autolock l(mLock);
+        mAudioOutput.clear();
+    }
     wp<Client> client(this);
     disconnect();
     mService->removeClient(client);
@@ -610,10 +636,9 @@ void MediaPlayerService::Client::disconnect()
         Mutex::Autolock l(mLock);
         p = mPlayer;
         mClient.clear();
+        mPlayer.clear();
     }
 
-    mPlayer.clear();
-
     // clear the notification to prevent callbacks to dead client
     // and reset the player. We assume the player will serialize
     // access to itself if necessary.
@@ -634,7 +659,7 @@ void MediaPlayerService::Client::disconnect()
 sp<MediaPlayerBase> MediaPlayerService::Client::createPlayer(player_type playerType)
 {
     // determine if we have the right player type
-    sp<MediaPlayerBase> p = mPlayer;
+    sp<MediaPlayerBase> p = getPlayer();
     if ((p != NULL) && (p->playerType() != playerType)) {
         ALOGV("delete player");
         p.clear();
@@ -691,6 +716,7 @@ void MediaPlayerService::Client::setDataSource_post(
     }
 
     if (mStatus == OK) {
+        Mutex::Autolock l(mLock);
         mPlayer = p;
     }
 }