Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 4ce1273f authored by JW Wang's avatar JW Wang
Browse files

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<PackageInstallerSession>) which is callable
   when mLock is held.

Bug: 159568595
Test: atest StagedInstallTest
Change-Id: I512ea07a4f8750431b306fa6e3c47a600a50b64f
parent 98176cac

services/core/java/com/android/server/pm/PackageInstallerSession.java

+44 −20
Original line number Diff line number Diff line
@@ -402,6 +402,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() {
@@ -1355,7 +1356,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { 
    private boolean markAsSealed(@NonNull IntentSender statusReceiver, boolean forTransfer) {

        Objects.requireNonNull(statusReceiver);



        List<PackageInstallerSession> childSessions = getChildSessions();

        List<PackageInstallerSession> childSessions = getChildSessionsNotLocked();



        synchronized (mLock) {

            assertCallerIsOwnerOrRootLocked();
@@ -1438,7 +1439,11 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { 
     *

     * <p> This method is handy to prevent potential deadlocks (b/123391593)

     */

    private @Nullable List<PackageInstallerSession> getChildSessions() {

    private @Nullable List<PackageInstallerSession> getChildSessionsNotLocked() {

        if (Thread.holdsLock(mLock)) {

            Slog.wtf(TAG, "Calling thread " + Thread.currentThread().getName()

                    + " is holding mLock", new Throwable());

        }

        List<PackageInstallerSession> childSessions = null;

        if (isMultiPackage()) {

            final int[] childSessionIds = getChildSessionIds();
@@ -1607,7 +1612,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { 
                return;

            }

        }

        List<PackageInstallerSession> childSessions = getChildSessions();

        List<PackageInstallerSession> childSessions = getChildSessionsNotLocked();

        synchronized (mLock) {

            try {

                sealLocked(childSessions);
@@ -1651,7 +1656,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { 
            throw new SecurityException("Can only transfer sessions that use public options");

        }



        List<PackageInstallerSession> childSessions = getChildSessions();

        List<PackageInstallerSession> childSessions = getChildSessionsNotLocked();



        synchronized (mLock) {

            assertCallerIsOwnerOrRootLocked();
@@ -1703,7 +1708,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<PackageInstallerSession> childSessions = getChildSessions();

        List<PackageInstallerSession> childSessions = getChildSessionsNotLocked();



        try {

            synchronized (mLock) {
@@ -2603,6 +2608,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { 
                    "Session " + sessionId + " is a child of multi-package session "

                            + mParentSessionId +  " and may not be abandoned directly.");

        }



        List<PackageInstallerSession> childSessions = getChildSessionsNotLocked();

        synchronized (mLock) {

            if (params.isStaged && mDestroyed) {

                // If a user abandons staged session in an unsafe state, then system will try to
@@ -2626,7 +2633,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { 
                    mCallback.onStagedSessionChanged(this);

                    return;

                }

                cleanStageDir();

                cleanStageDir(childSessions);

            }



            if (mRelinquished) {
@@ -3056,7 +3063,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { 
            mStagedSessionErrorMessage = errorMessage;

            Slog.d(TAG, "Marking session " + sessionId + " as failed: " + errorMessage);

        }

        cleanStageDir();

        cleanStageDirNotLocked();

        mCallback.onStagedSessionChanged(this);

    }
@@ -3071,7 +3078,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { 
            mStagedSessionErrorMessage = "";

            Slog.d(TAG, "Marking session " + sessionId + " as applied");

        }

        cleanStageDir();

        cleanStageDirNotLocked();

        mCallback.onStagedSessionChanged(this);

    }
@@ -3129,12 +3136,30 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { 
        }

    }



    private void cleanStageDir() {

        if (isMultiPackage()) {

            for (int childSessionId : getChildSessionIds()) {

                mSessionProvider.getSession(childSessionId).cleanStageDir();

    /**

     * <b>must not hold {@link #mLock}</b>

     */

    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<PackageInstallerSession> childSessions) {

        if (childSessions != null) {

            for (PackageInstallerSession childSession : childSessions) {

                if (childSession != null) {

                    childSession.cleanStageDir();

                }

            }

        } else {

            cleanStageDir();

        }

    }



    private void cleanStageDir() {

        if (mIncrementalFileStorages != null) {

            mIncrementalFileStorages.cleanUp();

            mIncrementalFileStorages = null;
@@ -3144,7 +3169,6 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { 
        } catch (InstallerException ignored) {

        }

    }

    }



    void dump(IndentingPrintWriter pw) {

        synchronized (mLock) {