diff options
3 files changed, 188 insertions, 21 deletions
diff --git a/libartservice/service/java/com/android/server/art/DexUseManagerLocal.java b/libartservice/service/java/com/android/server/art/DexUseManagerLocal.java index 8d47cb6ba0..704dabe034 100644 --- a/libartservice/service/java/com/android/server/art/DexUseManagerLocal.java +++ b/libartservice/service/java/com/android/server/art/DexUseManagerLocal.java @@ -113,6 +113,16 @@ public class DexUseManagerLocal { */ @VisibleForTesting public static final long INTERVAL_MS = 15_000; + // Impose a limit on the input accepted by notifyDexContainersLoaded per owning package. + /** @hide */ + @VisibleForTesting public static final int MAX_PATH_LENGTH = 4096; + + /** @hide */ + @VisibleForTesting public static final int MAX_CLASS_LOADER_CONTEXT_LENGTH = 10000; + + /** @hide */ + private static final int MAX_SECONDARY_DEX_FILES_PER_OWNER = 500; + private static final Object sLock = new Object(); // The static field is associated with the class and the class loader that loads it. In the @@ -527,7 +537,7 @@ public class DexUseManagerLocal { } // Check remaining packages. Don't check for shared libraries because it might be too - // expansive to do so and the time complexity is O(n) no matter we do it or not. + // expensive to do so and the time complexity is O(n) no matter we do it or not. for (PackageState pkgState : packageStates.values()) { if (visitedPackages.contains(pkgState.getPackageName())) { continue; @@ -657,16 +667,40 @@ public class DexUseManagerLocal { private void addSecondaryDexUse(@NonNull String owningPackageName, @NonNull String dexPath, @NonNull String loadingPackageName, boolean isolatedProcess, @NonNull String classLoaderContext, @NonNull String abiName, long lastUsedAtMs) { + DexLoader loader = DexLoader.create(loadingPackageName, isolatedProcess); + // This is to avoid a loading package from using up the SecondaryDexUse entries for another + // package (up to the MAX_SECONDARY_DEX_FILES_PER_OWNER limit). We don't care about the + // loading package messing up its own SecondaryDexUse entries. + // Note that we are using system_server's permission to check the existence. This is fine + // with the assumption that the file must be world readable to be used by other apps. + // We could use artd's permission to check the existence, and then there wouldn't be any + // permission issue, but that requires bringing up the artd service, which may be too + // expensive. + // TODO(jiakaiz): Check if the assumption is true. + if (isLoaderOtherApp(loader, owningPackageName) && !mInjector.pathExists(dexPath)) { + AsLog.w("Not recording non-existent secondary dex file '" + dexPath + "'"); + return; + } synchronized (mLock) { + PackageDexUse packageDexUse = mDexUse.mPackageDexUseByOwningPackageName.computeIfAbsent( + owningPackageName, k -> new PackageDexUse()); SecondaryDexUse secondaryDexUse = - mDexUse.mPackageDexUseByOwningPackageName - .computeIfAbsent(owningPackageName, k -> new PackageDexUse()) - .mSecondaryDexUseByDexFile.computeIfAbsent( - dexPath, k -> new SecondaryDexUse()); + packageDexUse.mSecondaryDexUseByDexFile.computeIfAbsent(dexPath, k -> { + if (packageDexUse.mSecondaryDexUseByDexFile.size() + >= mInjector.getMaxSecondaryDexFilesPerOwner()) { + AsLog.w("Not recording too many secondary dex use entries for " + + owningPackageName); + return null; + } + return new SecondaryDexUse(); + }); + if (secondaryDexUse == null) { + return; + } secondaryDexUse.mUserHandle = mInjector.getCallingUserHandle(); - SecondaryDexUseRecord record = secondaryDexUse.mRecordByLoader.computeIfAbsent( - DexLoader.create(loadingPackageName, isolatedProcess), - k -> new SecondaryDexUseRecord()); + SecondaryDexUseRecord record = + secondaryDexUse.mRecordByLoader.computeIfAbsent( + loader, k -> new SecondaryDexUseRecord()); record.mClassLoaderContext = classLoaderContext; record.mAbiName = abiName; record.mLastUsedAtMs = lastUsedAtMs; @@ -772,13 +806,23 @@ public class DexUseManagerLocal { } for (var entry : classLoaderContextByDexContainerFile.entrySet()) { - Utils.assertNonEmpty(entry.getKey()); - String errorMsg = ArtJni.validateDexPath(entry.getKey()); + String dexPath = entry.getKey(); + String classLoaderContext = entry.getValue(); + Utils.assertNonEmpty(dexPath); + if (dexPath.length() > MAX_PATH_LENGTH) { + throw new IllegalArgumentException( + "Dex path too long - exceeds " + MAX_PATH_LENGTH + " chars"); + } + String errorMsg = ArtJni.validateDexPath(dexPath); if (errorMsg != null) { throw new IllegalArgumentException(errorMsg); } - Utils.assertNonEmpty(entry.getValue()); - errorMsg = ArtJni.validateClassLoaderContext(entry.getKey(), entry.getValue()); + Utils.assertNonEmpty(classLoaderContext); + if (classLoaderContext.length() > MAX_CLASS_LOADER_CONTEXT_LENGTH) { + throw new IllegalArgumentException("Class loader context too long - exceeds " + + MAX_CLASS_LOADER_CONTEXT_LENGTH + " chars"); + } + errorMsg = ArtJni.validateClassLoaderContext(dexPath, classLoaderContext); if (errorMsg != null) { throw new IllegalArgumentException(errorMsg); } @@ -1354,6 +1398,10 @@ public class DexUseManagerLocal { return System.currentTimeMillis(); } + public boolean pathExists(String path) { + return new File(path).exists(); + } + @NonNull public String getFilename() { return FILENAME; @@ -1404,5 +1452,9 @@ public class DexUseManagerLocal { public boolean isIsolatedUid(int uid) { return Process.isIsolatedUid(uid); } + + public int getMaxSecondaryDexFilesPerOwner() { + return MAX_SECONDARY_DEX_FILES_PER_OWNER; + } } } diff --git a/libartservice/service/javatests/com/android/server/art/DexUseManagerTest.java b/libartservice/service/javatests/com/android/server/art/DexUseManagerTest.java index d404b4cbda..f0002b80d7 100644 --- a/libartservice/service/javatests/com/android/server/art/DexUseManagerTest.java +++ b/libartservice/service/javatests/com/android/server/art/DexUseManagerTest.java @@ -28,6 +28,7 @@ import static org.mockito.Mockito.argThat; import static org.mockito.Mockito.eq; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -85,6 +86,9 @@ public class DexUseManagerTest { private static final String INVISIBLE_BASE_APK = "/somewhere/app/" + INVISIBLE_PKG_NAME + "/base.apk"; + // A reduced limit to make the test run faster. + private static final int MAX_SECONDARY_DEX_FILES_PER_OWNER_FOR_TESTING = 50; + @Rule public StaticMockitoRule mockitoRule = new StaticMockitoRule( SystemProperties.class, Constants.class, ArtJni.class); @@ -173,6 +177,7 @@ public class DexUseManagerTest { lenient().when(mInjector.getArtd()).thenReturn(mArtd); lenient().when(mInjector.getCurrentTimeMillis()).thenReturn(0l); + lenient().when(mInjector.pathExists(any())).thenReturn(true); lenient().when(mInjector.getFilename()).thenReturn(mTempFile.getPath()); lenient() .when(mInjector.createScheduledExecutor()) @@ -185,6 +190,9 @@ public class DexUseManagerTest { lenient().when(mInjector.getCallingUserHandle()).thenReturn(mUserHandle); lenient().when(mInjector.getCallingUid()).thenReturn(110001); lenient().when(mInjector.isIsolatedUid(anyInt())).thenReturn(false); + lenient() + .when(mInjector.getMaxSecondaryDexFilesPerOwner()) + .thenReturn(MAX_SECONDARY_DEX_FILES_PER_OWNER_FOR_TESTING); mDexUseManager = new DexUseManagerLocal(mInjector); mDexUseManager.systemReady(); @@ -639,11 +647,11 @@ public class DexUseManagerTest { @Test public void testCheckedSecondaryDexNotFound() throws Exception { - when(mArtd.getDexFileVisibility(mCeDir + "/foo.apk")).thenReturn(FileVisibility.NOT_FOUND); - mDexUseManager.notifyDexContainersLoaded( mSnapshot, OWNING_PKG_NAME, Map.of(mCeDir + "/foo.apk", "CLC")); + when(mArtd.getDexFileVisibility(mCeDir + "/foo.apk")).thenReturn(FileVisibility.NOT_FOUND); + assertThat(mDexUseManager.getCheckedSecondaryDexInfo( OWNING_PKG_NAME, true /* excludeObsoleteDexesAndLoaders */)) .isEmpty(); @@ -825,6 +833,18 @@ public class DexUseManagerTest { } @Test(expected = IllegalArgumentException.class) + public void testTooLongDexPath() throws Exception { + mDexUseManager.notifyDexContainersLoaded(mSnapshot, OWNING_PKG_NAME, + Map.of("/" + "X".repeat(DexUseManagerLocal.MAX_PATH_LENGTH), "CLC")); + } + + @Test + public void testMaxLengthDexPath() throws Exception { + mDexUseManager.notifyDexContainersLoaded(mSnapshot, OWNING_PKG_NAME, + Map.of("/" + "X".repeat(DexUseManagerLocal.MAX_PATH_LENGTH - 1), "CLC")); + } + + @Test(expected = IllegalArgumentException.class) public void testInvalidDexPath() throws Exception { lenient().when(ArtJni.validateDexPath(any())).thenReturn("invalid"); mDexUseManager.notifyDexContainersLoaded( @@ -832,6 +852,20 @@ public class DexUseManagerTest { } @Test(expected = IllegalArgumentException.class) + public void testTooLongClassLoaderContext() throws Exception { + mDexUseManager.notifyDexContainersLoaded(mSnapshot, OWNING_PKG_NAME, + Map.of(mCeDir + "/foo.apk", + "X".repeat(DexUseManagerLocal.MAX_CLASS_LOADER_CONTEXT_LENGTH + 1))); + } + + @Test + public void testMaxLengthClassLoaderContext() throws Exception { + mDexUseManager.notifyDexContainersLoaded(mSnapshot, OWNING_PKG_NAME, + Map.of(mCeDir + "/foo.apk", + "X".repeat(DexUseManagerLocal.MAX_CLASS_LOADER_CONTEXT_LENGTH))); + } + + @Test(expected = IllegalArgumentException.class) public void testInvalidClassLoaderContext() throws Exception { lenient().when(ArtJni.validateClassLoaderContext(any(), any())).thenReturn("invalid"); mDexUseManager.notifyDexContainersLoaded( @@ -874,6 +908,87 @@ public class DexUseManagerTest { mSnapshot, OWNING_PKG_NAME, Map.of(BASE_APK, "CLC")); } + @Test + public void testExistingExternalSecondaryDexPath() throws Exception { + mMockClock.advanceTime(DexUseManagerLocal.INTERVAL_MS); // Save. + long oldFileSize = mTempFile.length(); + + String existingDexPath = mCeDir + "/foo.apk"; + when(mInjector.pathExists(existingDexPath)).thenReturn(true); + mDexUseManager.notifyDexContainersLoaded( + mSnapshot, LOADING_PKG_NAME, Map.of(existingDexPath, "PCL[]")); + + mMockClock.advanceTime(DexUseManagerLocal.INTERVAL_MS); // Save. + assertThat(mTempFile.length()).isGreaterThan(oldFileSize); + } + + @Test + public void testNonexistingExternalSecondaryDexPath() throws Exception { + mMockClock.advanceTime(DexUseManagerLocal.INTERVAL_MS); // Save. + long oldFileSize = mTempFile.length(); + + String nonexistingDexPath = mCeDir + "/foo.apk"; + when(mInjector.pathExists(nonexistingDexPath)).thenReturn(false); + mDexUseManager.notifyDexContainersLoaded( + mSnapshot, LOADING_PKG_NAME, Map.of(nonexistingDexPath, "PCL[]")); + + mMockClock.advanceTime(DexUseManagerLocal.INTERVAL_MS); // Save. + assertThat(mTempFile.length()).isEqualTo(oldFileSize); + } + + @Test + public void testInternalSecondaryDexPath() throws Exception { + mMockClock.advanceTime(DexUseManagerLocal.INTERVAL_MS); // Save. + long oldFileSize = mTempFile.length(); + + String nonexistingDexPath = mCeDir + "/foo.apk"; + lenient().when(mInjector.pathExists(nonexistingDexPath)).thenReturn(false); + mDexUseManager.notifyDexContainersLoaded( + mSnapshot, OWNING_PKG_NAME, Map.of(nonexistingDexPath, "PCL[]")); + verify(mArtd, never()).getDexFileVisibility(nonexistingDexPath); + + mMockClock.advanceTime(DexUseManagerLocal.INTERVAL_MS); // Save. + assertThat(mTempFile.length()).isGreaterThan(oldFileSize); + } + + @Test + public void testLimitSecondaryDexFiles() throws Exception { + for (int n = 0; n < MAX_SECONDARY_DEX_FILES_PER_OWNER_FOR_TESTING - 1; ++n) { + mDexUseManager.notifyDexContainersLoaded(mSnapshot, LOADING_PKG_NAME, + Map.of(String.format("%s/%04d/foo.apk", mCeDir, n), "CLC")); + } + mMockClock.advanceTime(DexUseManagerLocal.INTERVAL_MS); // Save. + long oldFileSize = mTempFile.length(); + + mDexUseManager.notifyDexContainersLoaded( + mSnapshot, LOADING_PKG_NAME, Map.of(mCeDir + "/9998/foo.apk", "CLC")); + mMockClock.advanceTime(DexUseManagerLocal.INTERVAL_MS); // Save. + assertThat(mTempFile.length()).isGreaterThan(oldFileSize); + + oldFileSize = mTempFile.length(); + mDexUseManager.notifyDexContainersLoaded( + mSnapshot, LOADING_PKG_NAME, Map.of(mCeDir + "/9999/foo.apk", "CLC")); + mMockClock.advanceTime(DexUseManagerLocal.INTERVAL_MS); // Save. + assertThat(mTempFile.length()).isEqualTo(oldFileSize); + + // Can still add loading packages to existing entries after the limit is reached. + mDexUseManager.notifyDexContainersLoaded( + mSnapshot, OWNING_PKG_NAME, Map.of(mCeDir + "/9998/foo.apk", "CLC")); + mMockClock.advanceTime(DexUseManagerLocal.INTERVAL_MS); // Save. + assertThat(mTempFile.length()).isGreaterThan(oldFileSize); + } + + @Test + public void testLimitSecondaryDexFilesSingleCall() throws Exception { + Map<String, String> clcByDexFile = new HashMap<>(); + for (int n = 0; n < MAX_SECONDARY_DEX_FILES_PER_OWNER_FOR_TESTING + 1; ++n) { + clcByDexFile.put(String.format("%s/%04d/foo.apk", mCeDir, n), "CLC"); + } + mDexUseManager.notifyDexContainersLoaded(mSnapshot, LOADING_PKG_NAME, clcByDexFile); + assertThat(mDexUseManager.getSecondaryDexInfo(OWNING_PKG_NAME)) + .hasSize(MAX_SECONDARY_DEX_FILES_PER_OWNER_FOR_TESTING); + } + private AndroidPackage createPackage(String packageName) { AndroidPackage pkg = mock(AndroidPackage.class); lenient().when(pkg.getStorageUuid()).thenReturn(StorageManager.UUID_DEFAULT); diff --git a/libartservice/service/proto/dex_use.proto b/libartservice/service/proto/dex_use.proto index 1dd962dbf4..1960882ad5 100644 --- a/libartservice/service/proto/dex_use.proto +++ b/libartservice/service/proto/dex_use.proto @@ -29,31 +29,31 @@ message DexUseProto { } message PackageDexUseProto { - string owning_package_name = 1; + string owning_package_name = 1; // key repeated PrimaryDexUseProto primary_dex_use = 2; repeated SecondaryDexUseProto secondary_dex_use = 3; } message PrimaryDexUseProto { - string dex_file = 1; + string dex_file = 1; // key repeated PrimaryDexUseRecordProto record = 2; } message PrimaryDexUseRecordProto { - string loading_package_name = 1; - bool isolated_process = 2; + string loading_package_name = 1; // key + bool isolated_process = 2; // key int64 last_used_at_ms = 3; } message SecondaryDexUseProto { - string dex_file = 1; + string dex_file = 1; // key Int32Value user_id = 2; // Must be explicitly set. repeated SecondaryDexUseRecordProto record = 3; } message SecondaryDexUseRecordProto { - string loading_package_name = 1; - bool isolated_process = 2; + string loading_package_name = 1; // key + bool isolated_process = 2; // key string class_loader_context = 3; string abi_name = 4; int64 last_used_at_ms = 5; |