From 8a207c40f82fc89e21e3245ddbb3237fc040002e Mon Sep 17 00:00:00 2001 From: JW Wang Date: Mon, 22 Jun 2020 16:22:08 +0800 Subject: Prevent a deadlock in #abandon See b/159568595#comment1 for analysis. 1. rename cleanStageDir to cleanStageDirNotLocked to show it must be called without mLock held. 2. rename getChildSessions to getChildSessionsNotLocked to show it must be called without mLock held. 3. add cleanStageDir(List) which is callable when mLock is held. Bug: 159568595 Test: atest StagedInstallTest Merged-In: I512ea07a4f8750431b306fa6e3c47a600a50b64f Change-Id: I512ea07a4f8750431b306fa6e3c47a600a50b64f (cherry picked from commit 4ce1273f926851bb58d44f59b5643373d4567c81) --- .../android/server/pm/PackageInstallerSession.java | 64 +++++++++++++++------- 1 file changed, 44 insertions(+), 20 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index de8ad6b7db13..994cec2b1e59 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -401,6 +401,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { private boolean mDataLoaderFinished = false; + // TODO(b/159663586): should be protected by mLock private IncrementalFileStorages mIncrementalFileStorages; private static final FileFilter sAddedApkFilter = new FileFilter() { @@ -1353,7 +1354,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { private boolean markAsSealed(@NonNull IntentSender statusReceiver, boolean forTransfer) { Objects.requireNonNull(statusReceiver); - List childSessions = getChildSessions(); + List childSessions = getChildSessionsNotLocked(); synchronized (mLock) { assertCallerIsOwnerOrRootLocked(); @@ -1436,7 +1437,11 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { * *

This method is handy to prevent potential deadlocks (b/123391593) */ - private @Nullable List getChildSessions() { + private @Nullable List getChildSessionsNotLocked() { + if (Thread.holdsLock(mLock)) { + Slog.wtf(TAG, "Calling thread " + Thread.currentThread().getName() + + " is holding mLock", new Throwable()); + } List childSessions = null; if (isMultiPackage()) { final int[] childSessionIds = getChildSessionIds(); @@ -1605,7 +1610,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { return; } } - List childSessions = getChildSessions(); + List childSessions = getChildSessionsNotLocked(); synchronized (mLock) { try { sealLocked(childSessions); @@ -1649,7 +1654,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { throw new SecurityException("Can only transfer sessions that use public options"); } - List childSessions = getChildSessions(); + List childSessions = getChildSessionsNotLocked(); synchronized (mLock) { assertCallerIsOwnerOrRootLocked(); @@ -1701,7 +1706,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { // outside of the lock, because reading the child // sessions with the lock held could lead to deadlock // (b/123391593). - List childSessions = getChildSessions(); + List childSessions = getChildSessionsNotLocked(); try { synchronized (mLock) { @@ -2602,6 +2607,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { "Session " + sessionId + " is a child of multi-package session " + mParentSessionId + " and may not be abandoned directly."); } + + List childSessions = getChildSessionsNotLocked(); synchronized (mLock) { if (params.isStaged && mDestroyed) { // If a user abandons staged session in an unsafe state, then system will try to @@ -2625,7 +2632,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { mCallback.onStagedSessionChanged(this); return; } - cleanStageDir(); + cleanStageDir(childSessions); } if (mRelinquished) { @@ -3055,7 +3062,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { mStagedSessionErrorMessage = errorMessage; Slog.d(TAG, "Marking session " + sessionId + " as failed: " + errorMessage); } - cleanStageDir(); + cleanStageDirNotLocked(); mCallback.onStagedSessionChanged(this); } @@ -3070,7 +3077,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { mStagedSessionErrorMessage = ""; Slog.d(TAG, "Marking session " + sessionId + " as applied"); } - cleanStageDir(); + cleanStageDirNotLocked(); mCallback.onStagedSessionChanged(this); } @@ -3128,20 +3135,37 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } } - private void cleanStageDir() { - if (isMultiPackage()) { - for (int childSessionId : getChildSessionIds()) { - mSessionProvider.getSession(childSessionId).cleanStageDir(); + /** + * must not hold {@link #mLock} + */ + private void cleanStageDirNotLocked() { + if (Thread.holdsLock(mLock)) { + Slog.wtf(TAG, "Calling thread " + Thread.currentThread().getName() + + " is holding mLock", new Throwable()); + } + cleanStageDir(getChildSessionsNotLocked()); + } + + private void cleanStageDir(List childSessions) { + if (childSessions != null) { + for (PackageInstallerSession childSession : childSessions) { + if (childSession != null) { + childSession.cleanStageDir(); + } } } else { - if (mIncrementalFileStorages != null) { - mIncrementalFileStorages.cleanUp(); - mIncrementalFileStorages = null; - } - try { - mPm.mInstaller.rmPackageDir(stageDir.getAbsolutePath()); - } catch (InstallerException ignored) { - } + cleanStageDir(); + } + } + + private void cleanStageDir() { + if (mIncrementalFileStorages != null) { + mIncrementalFileStorages.cleanUp(); + mIncrementalFileStorages = null; + } + try { + mPm.mInstaller.rmPackageDir(stageDir.getAbsolutePath()); + } catch (InstallerException ignored) { } } -- cgit v1.2.3-59-g8ed1b