From 4c2b9555b7b52359ea14e201d7ec61b8edaf6232 Mon Sep 17 00:00:00 2001 From: Calin Juravle Date: Thu, 10 Aug 2017 17:23:00 -0700 Subject: [PATCH] Ensure dexopt is executed only with consistent package data The main objects bookkeeping packages (PackageParser.Package and its corresponding ApplicationInfo) may be updated at different times. This creates a window where the data stored in the ApplicationInfo is out of date with respect to the data stored in PackageParser.Package. During install, the two objects are "synced" during doRename which updates the package code paths. This CLs moves the dexopt invocation from the install flow after doRename to ensure that dexopt logic gets a consistent view of the package. Bug: 64493351 Test: run cts-dev -t android.appsecurity.cts.ClassloaderSplitsTest -m CtsAppSecurityHostTestCases inspect oat files after adb install-multiple CtsClassloaderSplitApp/CtsClassloaderSplitApp.apk CtsClassloaderSplitAppFeatureA/CtsClassloaderSplitAppFeatureA.apk CtsClassloaderSplitAppFeatureB/CtsClassloaderSplitAppFeatureB.apk Change-Id: I9131bcf49eb473a8fdc5eb0032d94080d4e9e94b --- .../server/pm/PackageDexOptimizer.java | 12 +++ .../server/pm/PackageManagerService.java | 74 +++++++++++-------- 2 files changed, 56 insertions(+), 30 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageDexOptimizer.java b/services/core/java/com/android/server/pm/PackageDexOptimizer.java index 2f4f5ab723da8..698d3871e3c51 100644 --- a/services/core/java/com/android/server/pm/PackageDexOptimizer.java +++ b/services/core/java/com/android/server/pm/PackageDexOptimizer.java @@ -39,6 +39,7 @@ import com.android.server.pm.dex.PackageDexUsage; import java.io.File; import java.io.IOException; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import java.util.Map; @@ -149,6 +150,17 @@ public class PackageDexOptimizer { String[] classLoaderContexts = DexoptUtils.getClassLoaderContexts( pkg.applicationInfo, sharedLibraries); + // Sanity check that we do not call dexopt with inconsistent data. + if (paths.size() != classLoaderContexts.length) { + String[] splitCodePaths = pkg.applicationInfo.getSplitCodePaths(); + throw new IllegalStateException("Inconsistent information " + + "between PackageParser.Package and its ApplicationInfo. " + + "pkg.getAllCodePaths=" + paths + + " pkg.applicationInfo.getBaseCodePath=" + pkg.applicationInfo.getBaseCodePath() + + " pkg.applicationInfo.getSplitCodePaths=" + + (splitCodePaths == null ? "null" : Arrays.toString(splitCodePaths))); + } + int result = DEX_OPT_SKIPPED; for (int i = 0; i < paths.size(); i++) { // Skip paths that have no code. diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index e2e866e64bf68..af00f9f5375fa 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -18565,36 +18565,6 @@ public class PackageManagerService extends IPackageManager.Stub Slog.e(TAG, "updateAllSharedLibrariesLPw failed: " + e.getMessage()); } } - - // dexopt can take some time to complete, so, for instant apps, we skip this - // step during installation. Instead, we'll take extra time the first time the - // instant app starts. It's preferred to do it this way to provide continuous - // progress to the user instead of mysteriously blocking somewhere in the - // middle of running an instant app. The default behaviour can be overridden - // via gservices. - if (!instantApp || Global.getInt( - mContext.getContentResolver(), Global.INSTANT_APP_DEXOPT_ENABLED, 0) != 0) { - Trace.traceBegin(TRACE_TAG_PACKAGE_MANAGER, "dexopt"); - // Do not run PackageDexOptimizer through the local performDexOpt - // method because `pkg` may not be in `mPackages` yet. - // - // Also, don't fail application installs if the dexopt step fails. - DexoptOptions dexoptOptions = new DexoptOptions(pkg.packageName, - REASON_INSTALL, - DexoptOptions.DEXOPT_BOOT_COMPLETE); - mPackageDexOptimizer.performDexOpt(pkg, pkg.usesLibraryFiles, - null /* instructionSets */, - getOrCreateCompilerPackageStats(pkg), - mDexManager.getPackageUseInfoOrDefault(pkg.packageName), - dexoptOptions); - Trace.traceEnd(TRACE_TAG_PACKAGE_MANAGER); - } - - // Notify BackgroundDexOptService that the package has been changed. - // If this is an update of a package which used to fail to compile, - // BDOS will remove it from its blacklist. - // TODO: Layering violation - BackgroundDexOptService.notifyPackageChanged(pkg.packageName); } if (!args.doRename(res.returnCode, pkg, oldCodePath)) { @@ -18602,6 +18572,50 @@ public class PackageManagerService extends IPackageManager.Stub return; } + // Verify if we need to dexopt the app. + // + // NOTE: it is *important* to call dexopt after doRename which will sync the + // package data from PackageParser.Package and its corresponding ApplicationInfo. + // + // We only need to dexopt if the package meets ALL of the following conditions: + // 1) it is not forward locked. + // 2) it is not on on an external ASEC container. + // 3) it is not an instant app or if it is then dexopt is enabled via gservices. + // + // Note that we do not dexopt instant apps by default. dexopt can take some time to + // complete, so we skip this step during installation. Instead, we'll take extra time + // the first time the instant app starts. It's preferred to do it this way to provide + // continuous progress to the useur instead of mysteriously blocking somewhere in the + // middle of running an instant app. The default behaviour can be overridden + // via gservices. + final boolean performDexopt = !forwardLocked + && !pkg.applicationInfo.isExternalAsec() + && (!instantApp || Global.getInt(mContext.getContentResolver(), + Global.INSTANT_APP_DEXOPT_ENABLED, 0) != 0); + + if (performDexopt) { + Trace.traceBegin(TRACE_TAG_PACKAGE_MANAGER, "dexopt"); + // Do not run PackageDexOptimizer through the local performDexOpt + // method because `pkg` may not be in `mPackages` yet. + // + // Also, don't fail application installs if the dexopt step fails. + DexoptOptions dexoptOptions = new DexoptOptions(pkg.packageName, + REASON_INSTALL, + DexoptOptions.DEXOPT_BOOT_COMPLETE); + mPackageDexOptimizer.performDexOpt(pkg, pkg.usesLibraryFiles, + null /* instructionSets */, + getOrCreateCompilerPackageStats(pkg), + mDexManager.getPackageUseInfoOrDefault(pkg.packageName), + dexoptOptions); + Trace.traceEnd(TRACE_TAG_PACKAGE_MANAGER); + } + + // Notify BackgroundDexOptService that the package has been changed. + // If this is an update of a package which used to fail to compile, + // BackgroundDexOptService will remove it from its blacklist. + // TODO: Layering violation + BackgroundDexOptService.notifyPackageChanged(pkg.packageName); + startIntentFilterVerifications(args.user.getIdentifier(), replace, pkg); try (PackageFreezer freezer = freezePackageForInstall(pkgName, installFlags,