From eb832a8a336274260be8490d8aa3ad8592cd87f2 Mon Sep 17 00:00:00 2001 From: Chun-Wei Wang Date: Fri, 12 Aug 2022 12:16:13 +0000 Subject: [PATCH] Fix wrong path is returned when 2 files have the same name Before the fix, the cache is reused when 2 files have the same name. Now the full file path is included when calculating the cache key. This will reduce the chance of key collision. We also check if the paths match to ensure 2 different files are never mapped to the same cache entry. Bug: 242639590 Test: atest PackageParserTest Change-Id: I4d240407f6fd6de9a64f9ea46339d73202d40927 --- .../server/pm/parsing/PackageCacher.java | 9 +++- .../android/server/pm/PackageParserTest.java | 49 ++++++++++++++----- .../server/pm/parsing/TestPackageParser2.kt | 6 ++- 3 files changed, 50 insertions(+), 14 deletions(-) diff --git a/services/core/java/com/android/server/pm/parsing/PackageCacher.java b/services/core/java/com/android/server/pm/parsing/PackageCacher.java index f7a6eef29deec..d3a64bb0b9acc 100644 --- a/services/core/java/com/android/server/pm/parsing/PackageCacher.java +++ b/services/core/java/com/android/server/pm/parsing/PackageCacher.java @@ -62,6 +62,8 @@ public class PackageCacher { StringBuilder sb = new StringBuilder(packageFile.getName()); sb.append('-'); sb.append(flags); + sb.append('-'); + sb.append(packageFile.getAbsolutePath().hashCode()); return sb.toString(); } @@ -171,7 +173,12 @@ public class PackageCacher { } final byte[] bytes = IoUtils.readFileAsByteArray(cacheFile.getAbsolutePath()); - return fromCacheEntry(bytes); + ParsedPackage parsed = fromCacheEntry(bytes); + if (!packageFile.getAbsolutePath().equals(parsed.getPath())) { + // Don't use this cache if the path doesn't match + return null; + } + return parsed; } catch (Throwable e) { Slog.w(TAG, "Error reading package cache: ", e); diff --git a/services/tests/servicestests/src/com/android/server/pm/PackageParserTest.java b/services/tests/servicestests/src/com/android/server/pm/PackageParserTest.java index 0841bfc1d88c6..7ce7ac9a84cf2 100644 --- a/services/tests/servicestests/src/com/android/server/pm/PackageParserTest.java +++ b/services/tests/servicestests/src/com/android/server/pm/PackageParserTest.java @@ -17,6 +17,7 @@ package com.android.server.pm; import static com.android.server.pm.permission.CompatibilityPermissionInfo.COMPAT_PERMS; +import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; import static org.junit.Assert.assertArrayEquals; @@ -101,7 +102,6 @@ import java.io.IOException; import java.io.InputStream; import java.lang.reflect.Array; import java.lang.reflect.Field; -import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.util.ArrayList; import java.util.Arrays; @@ -259,6 +259,38 @@ public class PackageParserTest { return tmpFile; } + /** + * Extracts the asset file to $mTmpDir/$dirname/$filename. + */ + private File extractFile(String filename, String dirname) throws Exception { + final Context context = InstrumentationRegistry.getTargetContext(); + File dir = new File(mTmpDir, dirname); + dir.mkdir(); + final File tmpFile = new File(dir, filename); + try (InputStream inputStream = context.getAssets().openNonAsset(filename)) { + Files.copy(inputStream, tmpFile.toPath(), REPLACE_EXISTING); + } + return tmpFile; + } + + /** + * Tests the path of cached ParsedPackage. + */ + @Test + public void testCache_SameFileName() throws Exception { + // Prepare 2 package files with the same name but different paths + TestPackageParser2 parser = new TestPackageParser2(mTmpDir); + final File f1 = extractFile(TEST_APP1_APK, "dir1"); + final File f2 = extractFile(TEST_APP1_APK, "dir2"); + // Sleep for a while so that the cache file will be newer and valid + Thread.sleep(1000); + ParsedPackage pr1 = parser.parsePackage(f1, 0, true); + ParsedPackage pr2 = parser.parsePackage(f2, 0, true); + // Check the path of cached ParsedPackage + assertThat(pr1.getPath()).isEqualTo(f1.getAbsolutePath()); + assertThat(pr2.getPath()).isEqualTo(f2.getAbsolutePath()); + } + /** * Tests AndroidManifest.xml with no android:isolatedSplits attribute. */ @@ -632,8 +664,8 @@ public class PackageParserTest { } /** - * A trivial subclass of package parser that only caches the package name, and throws away - * all other information. + * A subclass of package parser that adds a "cache_" prefix to the package name for the cached + * results. This is used by tests to tell if a ParsedPackage is generated from the cache or not. */ public static class CachePackageNameParser extends PackageParser2 { @@ -656,16 +688,11 @@ public class PackageParserTest { void setCacheDir(@NonNull File cacheDir) { this.mCacher = new PackageCacher(cacheDir) { - @Override - public byte[] toCacheEntry(ParsedPackage pkg) { - return ("cache_" + pkg.getPackageName()).getBytes(StandardCharsets.UTF_8); - } - @Override public ParsedPackage fromCacheEntry(byte[] cacheEntry) { - return ((ParsedPackage) PackageImpl.forTesting( - new String(cacheEntry, StandardCharsets.UTF_8)) - .hideAsParsed()); + ParsedPackage parsed = super.fromCacheEntry(cacheEntry); + parsed.setPackageName("cache_" + parsed.getPackageName()); + return parsed; } }; } diff --git a/services/tests/servicestests/src/com/android/server/pm/parsing/TestPackageParser2.kt b/services/tests/servicestests/src/com/android/server/pm/parsing/TestPackageParser2.kt index 6ee91b89cadcf..2332817911f71 100644 --- a/services/tests/servicestests/src/com/android/server/pm/parsing/TestPackageParser2.kt +++ b/services/tests/servicestests/src/com/android/server/pm/parsing/TestPackageParser2.kt @@ -17,9 +17,11 @@ package com.android.server.pm.parsing import android.content.pm.ApplicationInfo +import java.io.File -class TestPackageParser2 : PackageParser2(null /* separateProcesses */, null /* displayMetrics */, - null /* cacheDir */, object : PackageParser2.Callback() { +class TestPackageParser2(var cacheDir: File? = null) : PackageParser2( + null /* separateProcesses */, null /* displayMetrics */, + cacheDir /* cacheDir */, object : PackageParser2.Callback() { override fun isChangeEnabled(changeId: Long, appInfo: ApplicationInfo): Boolean { return true }