From cbf47916b3e7a971c3a61035eb2633f96fc043cb Mon Sep 17 00:00:00 2001
From: Jeff Sharkey <jsharkey@android.com>
Date: Fri, 12 Sep 2014 09:55:32 -0700
Subject: Fix lock inversion in PackageInstaller.

In a small handful of cases individual sessions call up into the
installer while holding their local locks.  Defend against this by
treating most InternalCallback events as async.  For sealed events,
perform the upcall outside of the session lock.

Bug: 17482676
Change-Id: I265d981c98c8928a0fced09d8b029ca16eb650d9
---
 .../android/server/pm/PackageInstallerService.java | 27 +++++++++++++++-------
 .../android/server/pm/PackageInstallerSession.java | 19 +++++++++------
 2 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/services/core/java/com/android/server/pm/PackageInstallerService.java b/services/core/java/com/android/server/pm/PackageInstallerService.java
index fa09d48..c106546 100644
--- a/services/core/java/com/android/server/pm/PackageInstallerService.java
+++ b/services/core/java/com/android/server/pm/PackageInstallerService.java
@@ -144,6 +144,7 @@ public class PackageInstallerService extends IPackageInstaller.Stub {
 
     private final File mStagingDir;
     private final HandlerThread mInstallThread;
+    private final Handler mInstallHandler;
 
     private final Callbacks mCallbacks;
 
@@ -188,6 +189,8 @@ public class PackageInstallerService extends IPackageInstaller.Stub {
         mInstallThread = new HandlerThread(TAG);
         mInstallThread.start();
 
+        mInstallHandler = new Handler(mInstallThread.getLooper());
+
         mCallbacks = new Callbacks(mInstallThread.getLooper());
 
         mSessionsFile = new AtomicFile(
@@ -961,13 +964,19 @@ public class PackageInstallerService extends IPackageInstaller.Stub {
             mCallbacks.notifySessionProgressChanged(session.sessionId, session.userId, progress);
         }
 
-        public void onSessionFinished(PackageInstallerSession session, boolean success) {
+        public void onSessionFinished(final PackageInstallerSession session, boolean success) {
             mCallbacks.notifySessionFinished(session.sessionId, session.userId, success);
-            synchronized (mSessions) {
-                mSessions.remove(session.sessionId);
-                mHistoricalSessions.put(session.sessionId, session);
-            }
-            writeSessionsAsync();
+
+            mInstallHandler.post(new Runnable() {
+                @Override
+                public void run() {
+                    synchronized (mSessions) {
+                        mSessions.remove(session.sessionId);
+                        mHistoricalSessions.put(session.sessionId, session);
+                        writeSessionsLocked();
+                    }
+                }
+            });
         }
 
         public void onSessionPrepared(PackageInstallerSession session) {
@@ -976,11 +985,13 @@ public class PackageInstallerService extends IPackageInstaller.Stub {
             writeSessionsAsync();
         }
 
-        public void onSessionSealed(PackageInstallerSession session) {
+        public void onSessionSealedBlocking(PackageInstallerSession session) {
             // It's very important that we block until we've recorded the
             // session as being sealed, since we never want to allow mutation
             // after sealing.
-            writeSessionsLocked();
+            synchronized (mSessions) {
+                writeSessionsLocked();
+            }
         }
     }
 }
diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java
index 4cbc1c4..fb0e357 100644
--- a/services/core/java/com/android/server/pm/PackageInstallerSession.java
+++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java
@@ -410,7 +410,9 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
     public void commit(IntentSender statusReceiver) {
         Preconditions.checkNotNull(statusReceiver);
 
+        final boolean wasSealed;
         synchronized (mLock) {
+            wasSealed = mSealed;
             if (!mSealed) {
                 // Verify that all writers are hands-off
                 for (FileBridge bridge : mBridges) {
@@ -418,17 +420,20 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
                         throw new SecurityException("Files still open");
                     }
                 }
-
-                // Persist the fact that we've sealed ourselves to prevent
-                // mutations of any hard links we create.
                 mSealed = true;
-                mCallback.onSessionSealed(this);
             }
+
+            // Client staging is fully done at this point
+            mClientProgress = 1f;
+            computeProgressLocked(true);
         }
 
-        // Client staging is fully done at this point
-        mClientProgress = 1f;
-        computeProgressLocked(true);
+        if (!wasSealed) {
+            // Persist the fact that we've sealed ourselves to prevent
+            // mutations of any hard links we create. We do this without holding
+            // the session lock, since otherwise it's a lock inversion.
+            mCallback.onSessionSealedBlocking(this);
+        }
 
         // This ongoing commit should keep session active, even though client
         // will probably close their end.
-- 
cgit v1.1