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

Commit 74008c84 authored by Lee Shombert's avatar Lee Shombert
Browse files

Fix race condition in PM snapshots 

Bug: 177499032

Fix a race condition in snapshot generation.  The root cause is that
the "invalid" flag and the current snapshot are not read atomically.
Therefore, the invalid flag that is read might not apply to the
snapshot being held by thread.

Three things are fixed in this change:

 1. If the current thread already holds mLock then the live computer
    is used even if the snapshot computer was requested.  See the bug
    for a description of why this works and why it is important even
    without items 2 and 3.

 2. Otherwise, the snapshot lock it taken to decide if the current
    snapshot can be reused or if it must be rebuilt.

 3. Two formatting/editorial nits are fixed.

Test: atest
 * FrameworksServicesTests:UserSystemPackageInstallerTest
 * FrameworksServicesTests:PackageManagerSettingsTests
 * FrameworksServicesTests:PackageManagerServiceTest
 * FrameworksServicesTests:AppsFilterTest
 * FrameworksServicesTests:PackageInstallerSessionTest
 * FrameworksServicesTests:ScanTests
 * UserLifecycleTests#startUser
 * UserLifecycleTests#stopUser
 * UserLifecycleTests#switchUser

Change-Id: I67dfcfde87e2b4cbc74b66a7d2921e602883be83
parent 3b82486c

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

+45 −24
Original line number Diff line number Diff line
@@ -464,7 +464,7 @@ import java.util.function.Predicate; 
/**

 * Keep track of all those APKs everywhere.

 * <p>

 * Internally there are two important locks:

 * Internally there are three important locks:

 * <ul>

 * <li>{@link #mLock} is used to guard all in-memory parsed package details

 * and other related state. It is a fine-grained lock that should only be held
@@ -475,6 +475,10 @@ import java.util.function.Predicate; 
 * this lock should never be acquired while already holding {@link #mLock}.

 * Conversely, it's safe to acquire {@link #mLock} momentarily while already

 * holding {@link #mInstallLock}.

 * <li>{@link #mSnapshotLock} is used to guard access to two snapshot fields: the snapshot

 * itself and the snapshot invalidation flag.  This lock should never be acquired while

 * already holding {@link #mLock}. Conversely, it's safe to acquire {@link #mLock}

 * momentarily while already holding {@link #mSnapshotLock}.

 * </ul>

 * Many internal methods rely on the caller to hold the appropriate locks, and

 * this contract is expressed through method name suffixes:
@@ -485,6 +489,8 @@ import java.util.function.Predicate; 
 * <li>fooLPr(): the caller must hold {@link #mLock} for reading

 * <li>fooLPw(): the caller must hold {@link #mLock} for writing

 * </ul>

 * {@link #mSnapshotLock} is taken in exactly one place - {@code snapshotComputer()}.  It

 * should not be taken anywhere else or used for any other purpose.

 * <p>

 * Because this class is very central to the platform's security; please run all

 * CTS and unit tests whenever making modifications:
@@ -4732,6 +4738,14 @@ public class PackageManagerService extends IPackageManager.Stub 
    // Manager.

    private static volatile boolean sSnapshotCorked = false;













    /**













     * This lock is used to make reads from {@link #sSnapshotInvalid} and













     * {@link #mSnapshotComputer} atomic inside {@code snapshotComputer()}.  This lock is













     * not meant to be used outside that method.  This lock must be taken before













     * {@link #mLock} is taken.













     */













    private final Object mSnapshotLock = new Object();



























    // A counter of all queries that hit the cache.















    private AtomicInteger mSnapshotHits = new AtomicInteger(0);
























@@ -4759,10 +4773,16 @@ public class PackageManagerService extends IPackageManager.Stub













        if (!SNAPSHOT_ENABLED) {















            return mLiveComputer;















        }













        if (Thread.holdsLock(mLock)) {













            // If the current thread holds mLock then it may have modified state but not













            // yet invalidated the snapshot.  Always give the thread the live computer.













            return mLiveComputer;













        }















        int hits = 0;















        if (TRACE_CACHES) {















            hits = mSnapshotHits.incrementAndGet();















        }













        synchronized (mSnapshotLock) {















            Computer c = mSnapshotComputer;















            if (sSnapshotCorked && (c != null)) {















                // Snapshots are corked, which means new ones should not be built right now.









@@ -4789,6 +4809,7 @@ public class PackageManagerService extends IPackageManager.Stub













            }















            return c;















        }













    }





























    /**















     * Rebuild the cached computer.  mSnapshotComputer is temporarily set to null to block