From f29d07a62a78f73a21d649f6737992c267d6f170 Mon Sep 17 00:00:00 2001 From: Todd Kennedy Date: Mon, 8 Aug 2016 15:17:43 -0700 Subject: [PATCH] Don't call into pkg mgr svc w/ lock held In general, there's no reason to hold the lock when calling into external components. However, in practice, this can deadlock. When dumping its state, Package Manager Service holds a lock while it calls into other packages to dump their state. These other packages (such as Package Installer Session) typically take their own lock to dump coherent state. And, thus, a deadlock. Bug: 30419998 Change-Id: I0807b8742316d084e381be8721d1b3d41143b82c --- .../server/pm/PackageInstallerSession.java | 49 +++++++++++-------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index 6cdc40f7a81cc..583128444cfca 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -109,6 +109,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { final int installerUid; final SessionParams params; final long createdMillis; + final int defaultContainerGid; /** Staging location where client data is written. */ final File stageDir; @@ -199,13 +200,19 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { private final Handler.Callback mHandlerCallback = new Handler.Callback() { @Override public boolean handleMessage(Message msg) { + // Cache package manager data without the lock held + final PackageInfo pkgInfo = mPm.getPackageInfo( + params.appPackageName, PackageManager.GET_SIGNATURES /*flags*/, userId); + final ApplicationInfo appInfo = mPm.getApplicationInfo( + params.appPackageName, 0, userId); + synchronized (mLock) { if (msg.obj != null) { mRemoteObserver = (IPackageInstallObserver2) msg.obj; } try { - commitLocked(); + commitLocked(pkgInfo, appInfo); } catch (PackageManagerException e) { final String completeMsg = ExceptionUtils.getCompleteMessage(e); Slog.e(TAG, "Commit of session " + sessionId + " failed: " + completeMsg); @@ -264,6 +271,9 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } else { mPermissionsAccepted = false; } + final int uid = mPm.getPackageUid(PackageManagerService.DEFAULT_CONTAINER_PACKAGE, + PackageManager.MATCH_SYSTEM_ONLY, UserHandle.USER_SYSTEM); + defaultContainerGid = UserHandle.getSharedAppGid(uid); } public SessionInfo generateInfo() { @@ -520,7 +530,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { mHandler.obtainMessage(MSG_COMMIT, adapter.getBinder()).sendToTarget(); } - private void commitLocked() throws PackageManagerException { + private void commitLocked(PackageInfo pkgInfo, ApplicationInfo appInfo) + throws PackageManagerException { if (mDestroyed) { throw new PackageManagerException(INSTALL_FAILED_INTERNAL_ERROR, "Session destroyed"); } @@ -538,7 +549,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { // Verify that stage looks sane with respect to existing application. // This currently only ensures packageName, versionCode, and certificate // consistency. - validateInstallLocked(); + validateInstallLocked(pkgInfo, appInfo); Preconditions.checkNotNull(mPackageName); Preconditions.checkNotNull(mSignatures); @@ -650,7 +661,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { * Note that upgrade compatibility is still performed by * {@link PackageManagerService}. */ - private void validateInstallLocked() throws PackageManagerException { + private void validateInstallLocked(PackageInfo pkgInfo, ApplicationInfo appInfo) + throws PackageManagerException { mPackageName = null; mVersionCode = -1; mSignatures = null; @@ -729,10 +741,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { if (removeSplitList.size() > 0) { // validate split names marked for removal - final int flags = mSignatures == null ? PackageManager.GET_SIGNATURES : 0; - final PackageInfo pkg = mPm.getPackageInfo(params.appPackageName, flags, userId); for (String splitName : removeSplitList) { - if (!ArrayUtils.contains(pkg.splitNames, splitName)) { + if (!ArrayUtils.contains(pkgInfo.splitNames, splitName)) { throw new PackageManagerException(INSTALL_FAILED_INVALID_APK, "Split not found: " + splitName); } @@ -740,11 +750,11 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { // ensure we've got appropriate package name, version code and signatures if (mPackageName == null) { - mPackageName = pkg.packageName; - mVersionCode = pkg.versionCode; + mPackageName = pkgInfo.packageName; + mVersionCode = pkgInfo.versionCode; } if (mSignatures == null) { - mSignatures = pkg.signatures; + mSignatures = pkgInfo.signatures; } } @@ -757,8 +767,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } else { // Partial installs must be consistent with existing install - final ApplicationInfo app = mPm.getApplicationInfo(mPackageName, 0, userId); - if (app == null) { + if (appInfo == null) { throw new PackageManagerException(INSTALL_FAILED_INVALID_APK, "Missing existing base package for " + mPackageName); } @@ -766,8 +775,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { final PackageLite existing; final ApkLite existingBase; try { - existing = PackageParser.parsePackageLite(new File(app.getCodePath()), 0); - existingBase = PackageParser.parseApkLite(new File(app.getBaseCodePath()), + existing = PackageParser.parsePackageLite(new File(appInfo.getCodePath()), 0); + existingBase = PackageParser.parseApkLite(new File(appInfo.getBaseCodePath()), PackageParser.PARSE_COLLECT_CERTIFICATES); } catch (PackageParserException e) { throw PackageManagerException.from(e); @@ -777,7 +786,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { // Inherit base if not overridden if (mResolvedBaseFile == null) { - mResolvedBaseFile = new File(app.getBaseCodePath()); + mResolvedBaseFile = new File(appInfo.getBaseCodePath()); mResolvedInheritedFiles.add(mResolvedBaseFile); } @@ -794,7 +803,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } // Inherit compiled oat directory. - final File packageInstallDir = (new File(app.getBaseCodePath())).getParentFile(); + final File packageInstallDir = (new File(appInfo.getBaseCodePath())).getParentFile(); mInheritedFilesBase = packageInstallDir; final File oatDir = new File(packageInstallDir, "oat"); if (oatDir.exists()) { @@ -822,7 +831,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } } - private void assertApkConsistent(String tag, ApkLite apk) throws PackageManagerException { + private void assertApkConsistent(String tag, ApkLite apk) + throws PackageManagerException { if (!mPackageName.equals(apk.packageName)) { throw new PackageManagerException(INSTALL_FAILED_INVALID_APK, tag + " package " + apk.packageName + " inconsistent with " + mPackageName); @@ -1035,10 +1045,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { "Failed to finalize container " + cid); } - final int uid = mPm.getPackageUid(PackageManagerService.DEFAULT_CONTAINER_PACKAGE, - PackageManager.MATCH_SYSTEM_ONLY, UserHandle.USER_SYSTEM); - final int gid = UserHandle.getSharedAppGid(uid); - if (!PackageHelper.fixSdPermissions(cid, gid, null)) { + if (!PackageHelper.fixSdPermissions(cid, defaultContainerGid, null)) { throw new PackageManagerException(INSTALL_FAILED_CONTAINER_ERROR, "Failed to fix permissions on container " + cid); }