commit 060c9d6869a698d9605942ee1e2074aeeabc5f79
author Alex Buynytskyy <alexbuy@google.com> Thu Feb 18 20:55:17 2021 -0800
committer Alex Buynytskyy <alexbuy@google.com> Sat Feb 20 06:21:19 2021 +0000
tree d9bb517307501c48021dc3b8413b7a89826cf01e
parent 849587032600a159416035e4978b451969f1d695

Potential deadlock mitigation.

+better error logging

DataLoaders might report user statuses from lifecycle callbacks.
Immediate processing of such might introduce infinite loops/deadlocks e.g.
DataLoader_OnStop -> reportStatus(UNRECOVERABLE) -> fsmStep -> DataLoader_OnStop

Bug: 160634487
Test: atest PackageManagerShellCommandTest PackageManagerShellCommandIncrementalTest IncrementalServiceTest PackageManagerServiceTest ChecksumsTest
Change-Id: Ic68657d7a8cd6c6855b6f5295276a42b3cb09117

services/incremental/IncrementalService.cpp

diff --git a/services/incremental/IncrementalService.cpp b/services/incremental/IncrementalService.cpp
index 886c1e5..2fa927b 100644
--- a/services/incremental/IncrementalService.cpp
+++ b/services/incremental/IncrementalService.cpp
@@ -66,6 +66,8 @@
     static constexpr auto blockSize = 4096;
     static constexpr auto systemPackage = "android"sv;
 
+    static constexpr auto userStatusDelay = 100ms;
+
     static constexpr auto progressUpdateInterval = 1000ms;
     static constexpr auto perUidTimeoutOffset = progressUpdateInterval * 2;
     static constexpr auto minPerUidTimeout = progressUpdateInterval * 3;
@@ -2306,13 +2308,24 @@
         LOG(ERROR) << "Mount ID mismatch: expected " << id() << ", but got: " << mountId;
         return binder::Status::fromServiceSpecificError(-EPERM, "Mount ID mismatch.");
     }
+    if (newStatus == IDataLoaderStatusListener::DATA_LOADER_UNRECOVERABLE) {
+        // User-provided status, let's postpone the handling to avoid possible deadlocks.
+        mService.addTimedJob(*mService.mTimedQueue, id(), Constants::userStatusDelay,
+                             [this, newStatus]() { setCurrentStatus(newStatus); });
+        return binder::Status::ok();
+    }
 
+    setCurrentStatus(newStatus);
+    return binder::Status::ok();
+}
+
+void IncrementalService::DataLoaderStub::setCurrentStatus(int newStatus) {
     int targetStatus, oldStatus;
     DataLoaderStatusListener listener;
     {
         std::unique_lock lock(mMutex);
         if (mCurrentStatus == newStatus) {
-            return binder::Status::ok();
+            return;
         }
 
         oldStatus = mCurrentStatus;
@@ -2332,14 +2345,12 @@
                << newStatus << " (target " << targetStatus << ")";
 
     if (listener) {
-        listener->onStatusChanged(mountId, newStatus);
+        listener->onStatusChanged(id(), newStatus);
     }
 
     fsmStep();
 
     mStatusCondition.notify_all();
-
-    return binder::Status::ok();
 }
 
 binder::Status IncrementalService::DataLoaderStub::reportStreamHealth(MountId mountId,

services/incremental/IncrementalService.h

diff --git a/services/incremental/IncrementalService.h b/services/incremental/IncrementalService.h
index 459ed32..d8f2c91 100644
--- a/services/incremental/IncrementalService.h
+++ b/services/incremental/IncrementalService.h
@@ -234,6 +234,8 @@
         binder::Status onStatusChanged(MountId mount, int newStatus) final;
         binder::Status reportStreamHealth(MountId mount, int newStatus) final;
 
+        void setCurrentStatus(int newStatus);
+
         sp<content::pm::IDataLoader> getDataLoader();
 
         bool bind();