commit b0ea448eebe1422d5e42e8b86a02bed5daac2fd7
author Alex Buynytskyy <alexbuy@google.com> Mon May 04 18:39:58 2020 -0700
committer Alex Buynytskyy <alexbuy@google.com> Mon May 04 18:41:47 2020 -0700
tree 96073cf4a8873013f30009e2a417c3bbb963bbc7
parent ed2789354a437ac39c1b0f17c12d328ce7dc302a

Additional lock to avoid access to deleted object.

Bug: b/155692497
Fixes: 155692497
Test: atest PackageManagerShellCommandTest PackageManagerShellCommandIncrementalTest IncrementalServiceTest
Change-Id: Ie02012bd5a6c0640d54c5390d3726a5405042408

services/incremental/IncrementalService.cpp

diff --git a/services/incremental/IncrementalService.cpp b/services/incremental/IncrementalService.cpp
index 992a4ef..c3c2157 100644
--- a/services/incremental/IncrementalService.cpp
+++ b/services/incremental/IncrementalService.cpp
@@ -1670,9 +1670,15 @@
 
 void IncrementalService::DataLoaderStub::cleanupResources() {
     requestDestroy();
+
+    auto now = Clock::now();
+
+    std::unique_lock lock(mMutex);
     mParams = {};
     mControl = {};
-    waitForStatus(IDataLoaderStatusListener::DATA_LOADER_DESTROYED, std::chrono::seconds(60));
+    mStatusCondition.wait_until(lock, now + 60s, [this] {
+        return mCurrentStatus == IDataLoaderStatusListener::DATA_LOADER_DESTROYED;
+    });
     mListener = {};
     mId = kInvalidStorageId;
 }
@@ -1706,7 +1712,7 @@
 bool IncrementalService::DataLoaderStub::setTargetStatus(int newStatus) {
     int oldStatus, curStatus;
     {
-        std::unique_lock lock(mStatusMutex);
+        std::unique_lock lock(mMutex);
         oldStatus = mTargetStatus;
         curStatus = mCurrentStatus;
         setTargetStatusLocked(newStatus);
@@ -1721,13 +1727,6 @@
     mTargetStatusTs = Clock::now();
 }
 
-bool IncrementalService::DataLoaderStub::waitForStatus(int status, Clock::duration duration) {
-    auto now = Clock::now();
-    std::unique_lock lock(mStatusMutex);
-    return mStatusCondition.wait_until(lock, now + duration,
-                                       [this, status] { return mCurrentStatus == status; });
-}
-
 bool IncrementalService::DataLoaderStub::bind() {
     bool result = false;
     auto status = mService.mDataLoaderManager->bindToDataLoader(mId, mParams, this, &result);
@@ -1776,7 +1775,7 @@
     int currentStatus;
     int targetStatus;
     {
-        std::unique_lock lock(mStatusMutex);
+        std::unique_lock lock(mMutex);
         currentStatus = mCurrentStatus;
         targetStatus = mTargetStatus;
     }
@@ -1828,8 +1827,9 @@
     }
 
     int targetStatus, oldStatus;
+    DataLoaderStatusListener listener;
     {
-        std::unique_lock lock(mStatusMutex);
+        std::unique_lock lock(mMutex);
         if (mCurrentStatus == newStatus) {
             return binder::Status::ok();
         }
@@ -1838,6 +1838,8 @@
         mCurrentStatus = newStatus;
         targetStatus = mTargetStatus;
 
+        listener = mListener;
+
         if (mCurrentStatus == IDataLoaderStatusListener::DATA_LOADER_UNAVAILABLE) {
             // For unavailable, reset target status.
             setTargetStatusLocked(IDataLoaderStatusListener::DATA_LOADER_UNAVAILABLE);
@@ -1847,8 +1849,8 @@
     LOG(DEBUG) << "Current status update for DataLoader " << mId << ": " << oldStatus << " -> "
                << newStatus << " (target " << targetStatus << ")";
 
-    if (mListener) {
-        mListener->onStatusChanged(mountId, newStatus);
+    if (listener) {
+        listener->onStatusChanged(mountId, newStatus);
     }
 
     fsmStep();