From 95c129e50575276805938e6bea98e361db26a830 Mon Sep 17 00:00:00 2001
From: Felka Chang <felkachang@google.com>
Date: Fri, 20 Aug 2021 17:48:22 +0800
Subject: [PATCH] Declare mCommitted as AtomicBoolean in
 PackageInstallerSession

mCommitted is declared as boolean and is accessed with holding mLock.
mCommitted is set to be true only in streamValidateAndCommit(). After
that, the value of mCommitted shouldn't set back to false.

In order to reduce the chance to make a deadlock situation happen,
this patch changes mCommitted declaration from boolean to
AtomicBoolean.

Test: atest \
                CtsStagedInstallHostTestCases \
                CtsPackageInstallTestCases \
                CtsAtomicInstallTestCases
Test: Presubmit Treehugger

Fix: 197088926
Change-Id: I7e7e952e26c07518539a3e3f587f699c7ecef782
---
 .../server/pm/PackageInstallerSession.java    | 35 +++++++++++--------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java
index 958c769ba7d19..8e5b9db1fcdde 100644
--- a/services/core/java/com/android/server/pm/PackageInstallerSession.java
+++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java
@@ -348,8 +348,9 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
     private boolean mSealed = false;
     @GuardedBy("mLock")
     private boolean mShouldBeSealed = false;
-    @GuardedBy("mLock")
-    private boolean mCommitted = false;
+
+    private final AtomicBoolean mCommitted = new AtomicBoolean(false);
+
     @GuardedBy("mLock")
     private boolean mRelinquished = false;
 
@@ -692,11 +693,10 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
                     return;
                 }
                 mDestroyed = true;
-                boolean isCommitted = mCommitted;
                 List<PackageInstallerSession> childSessions = getChildSessionsLocked();
                 r = () -> {
                     assertNotLocked("abandonStaged");
-                    if (isCommitted) {
+                    if (mCommitted.get()) {
                         mStagingManager.abortCommittedSession(this);
                     }
                     cleanStageDir(childSessions);
@@ -1069,7 +1069,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
         }
 
         mPrepared = prepared;
-        mCommitted = committed;
+        mCommitted.set(committed);
         mDestroyed = destroyed;
         mStagedSession = params.isStaged ? new StagedSession(isReady, isApplied, isFailed,
                 stagedSessionErrorCode, stagedSessionErrorMessage) : null;
@@ -1139,7 +1139,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
                     mResolvedBaseFile.getAbsolutePath() : null;
             info.progress = mProgress;
             info.sealed = mSealed;
-            info.isCommitted = mCommitted;
+            info.isCommitted = mCommitted.get();
             info.active = mActiveCount.get() > 0;
 
             info.mode = params.mode;
@@ -1195,9 +1195,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
 
     /** {@hide} */
     boolean isCommitted() {
-        synchronized (mLock) {
-            return mCommitted;
-        }
+        return mCommitted.get();
     }
 
     /** {@hide} */
@@ -1235,7 +1233,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
     @GuardedBy("mLock")
     private void assertPreparedAndNotCommittedOrDestroyedLocked(String cookie) {
         assertPreparedAndNotDestroyedLocked(cookie);
-        if (mCommitted) {
+        if (mCommitted.get()) {
             throw new SecurityException(cookie + " not allowed after commit");
         }
     }
@@ -1276,7 +1274,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
 
     @GuardedBy("mProgressLock")
     private void computeProgressLocked(boolean forcePublish) {
-        if (!isIncrementalInstallation() || !mCommitted) {
+        if (!isIncrementalInstallation() || !mCommitted.get()) {
             mProgress = MathUtils.constrain(mClientProgress * 0.8f, 0f, 0.8f)
                     + MathUtils.constrain(mInternalProgress * 0.2f, 0f, 0.2f);
         } else {
@@ -2004,7 +2002,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
         //             single / child sessions.
         try {
             synchronized (mLock) {
-                if (mCommitted) {
+                if (mCommitted.get()) {
                     return true;
                 }
                 // Read transfers from the original owner stay open, but as the session's data
@@ -2037,7 +2035,13 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
                 // will probably close their end.
                 mActiveCount.incrementAndGet();
 
-                mCommitted = true;
+                if (!mCommitted.compareAndSet(false /*expect*/, true /*update*/)) {
+                    throw new PackageManagerException(
+                            INSTALL_FAILED_INTERNAL_ERROR,
+                            TextUtils.formatSimple(
+                                    "The mCommitted of session %d should be false originally",
+                                    sessionId));
+                }
                 committedMillis = System.currentTimeMillis();
             }
             return true;
@@ -4097,7 +4101,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
     private boolean canBeAddedAsChild(int parentCandidate) {
         synchronized (mLock) {
             return (!hasParentSessionId() || mParentSessionId == parentCandidate)
-                    && !mCommitted && !mDestroyed;
+                    && !mCommitted.get()
+                    && !mDestroyed;
         }
     }
 
@@ -4541,7 +4546,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
                 writeStringAttribute(out, ATTR_SESSION_STAGE_CID, stageCid);
             }
             writeBooleanAttribute(out, ATTR_PREPARED, mPrepared);
-            writeBooleanAttribute(out, ATTR_COMMITTED, mCommitted);
+            writeBooleanAttribute(out, ATTR_COMMITTED, mCommitted.get());
             writeBooleanAttribute(out, ATTR_DESTROYED, mDestroyed);
             writeBooleanAttribute(out, ATTR_SEALED, mSealed);