diff --git a/services/core/java/com/android/server/compat/CompatConfig.java b/services/core/java/com/android/server/compat/CompatConfig.java index 2bf1ccd519390..f2408cfa41249 100644 --- a/services/core/java/com/android/server/compat/CompatConfig.java +++ b/services/core/java/com/android/server/compat/CompatConfig.java @@ -220,11 +220,18 @@ final class CompatConfig { * Overrides the enabled state for a given change and app. * * - * @param overrides list of overrides to default changes config. - * @param packageName app for which the overrides will be applied. + * @param overrides list of overrides to default changes config. + * @param packageName app for which the overrides will be applied. + * @param skipUnknownChangeIds whether to skip unknown change IDs in {@code overrides}. */ - synchronized void addOverrides(CompatibilityOverrideConfig overrides, String packageName) { + synchronized void addPackageOverrides(CompatibilityOverrideConfig overrides, + String packageName, boolean skipUnknownChangeIds) { for (Long changeId : overrides.overrides.keySet()) { + if (skipUnknownChangeIds && !isKnownChangeId(changeId)) { + Slog.w(TAG, "Trying to add overrides for unknown Change ID " + changeId + ". " + + "Skipping Change ID."); + continue; + } addOverrideUnsafe(changeId, packageName, overrides.overrides.get(changeId)); } saveOverrides(); @@ -335,7 +342,8 @@ final class CompatConfig { /** * Removes all overrides previously added via {@link #addOverride(long, String, boolean)} or - * {@link #addOverrides(CompatibilityOverrideConfig, String)} for a certain package. + * {@link #addPackageOverrides(CompatibilityOverrideConfig, String, boolean)} for a certain + * package. * *

This restores the default behaviour for the given app. * @@ -356,7 +364,8 @@ final class CompatConfig { /** * Removes overrides whose change ID is specified in {@code overridesToRemove} that were * previously added via {@link #addOverride(long, String, boolean)} or - * {@link #addOverrides(CompatibilityOverrideConfig, String)} for a certain package. + * {@link #addPackageOverrides(CompatibilityOverrideConfig, String, boolean)} for a certain + * package. * *

This restores the default behaviour for the given change IDs and app. * @@ -367,6 +376,11 @@ final class CompatConfig { String packageName) { boolean shouldInvalidateCache = false; for (Long changeId : overridesToRemove.changeIds) { + if (!isKnownChangeId(changeId)) { + Slog.w(TAG, "Trying to remove overrides for unknown Change ID " + changeId + ". " + + "Skipping Change ID."); + continue; + } shouldInvalidateCache |= removeOverrideUnsafe(changeId, packageName); } if (shouldInvalidateCache) { diff --git a/services/core/java/com/android/server/compat/PlatformCompat.java b/services/core/java/com/android/server/compat/PlatformCompat.java index b32d1d7496803..6ea89d4454024 100644 --- a/services/core/java/com/android/server/compat/PlatformCompat.java +++ b/services/core/java/com/android/server/compat/PlatformCompat.java @@ -205,7 +205,8 @@ public class PlatformCompat extends IPlatformCompat.Stub { overridesMap.put(change, new PackageOverride.Builder().setEnabled(false) .build()); } - mCompatConfig.addOverrides(new CompatibilityOverrideConfig(overridesMap), packageName); + mCompatConfig.addPackageOverrides(new CompatibilityOverrideConfig(overridesMap), + packageName, /* skipUnknownChangeIds */ false); killPackage(packageName); } @@ -220,7 +221,8 @@ public class PlatformCompat extends IPlatformCompat.Stub { overridesMap.put(change, new PackageOverride.Builder().setEnabled(false) .build()); } - mCompatConfig.addOverrides(new CompatibilityOverrideConfig(overridesMap), packageName); + mCompatConfig.addPackageOverrides(new CompatibilityOverrideConfig(overridesMap), + packageName, /* skipUnknownChangeIds */ false); } @Override @@ -229,7 +231,7 @@ public class PlatformCompat extends IPlatformCompat.Stub { // TODO(b/183630314): Unify the permission enforcement with the other setOverrides* methods. checkCompatChangeOverrideOverridablePermission(); checkAllCompatOverridesAreOverridable(overrides.overrides.keySet()); - mCompatConfig.addOverrides(overrides, packageName); + mCompatConfig.addPackageOverrides(overrides, packageName, /* skipUnknownChangeIds= */ true); } @Override @@ -435,7 +437,7 @@ public class PlatformCompat extends IPlatformCompat.Stub { private void checkAllCompatOverridesAreOverridable(Collection changeIds) { for (Long changeId : changeIds) { - if (!mCompatConfig.isOverridable(changeId)) { + if (isKnownChangeId(changeId) && !mCompatConfig.isOverridable(changeId)) { throw new SecurityException("Only change ids marked as Overridable can be " + "overridden."); } diff --git a/services/tests/servicestests/src/com/android/server/compat/CompatConfigTest.java b/services/tests/servicestests/src/com/android/server/compat/CompatConfigTest.java index 0248b9b887886..d926dcba54a31 100644 --- a/services/tests/servicestests/src/com/android/server/compat/CompatConfigTest.java +++ b/services/tests/servicestests/src/com/android/server/compat/CompatConfigTest.java @@ -265,9 +265,10 @@ public class CompatConfigTest { } @Test - public void testInstallerCanSetOverrides() throws Exception { + public void testInstallerCanAddOverrides() throws Exception { final long disabledChangeId1 = 1234L; final long disabledChangeId2 = 1235L; + final long unknownChangeId = 1236L; // We make disabledChangeId2 non-overridable to make sure it is ignored. CompatConfig compatConfig = CompatConfigBuilder.create(mBuildClassifier, mContext) .addDisabledOverridableChangeWithId(disabledChangeId1) @@ -284,19 +285,25 @@ public class CompatConfigTest { // Force the validator to prevent overriding non-overridable changes by using a user build. when(mBuildClassifier.isDebuggableBuild()).thenReturn(false); when(mBuildClassifier.isFinalBuild()).thenReturn(true); + Map overrides = new HashMap<>(); + overrides.put(disabledChangeId1, new PackageOverride.Builder() + .setMaxVersionCode(99L) + .setEnabled(true) + .build()); + // Adding an unknown change ID to make sure it's skipped if skipUnknownChangeIds is true. + overrides.put(unknownChangeId, new PackageOverride.Builder().setEnabled(false).build()); + CompatibilityOverrideConfig config = new CompatibilityOverrideConfig(overrides); - CompatibilityOverrideConfig config = new CompatibilityOverrideConfig( - Collections.singletonMap(disabledChangeId1, - new PackageOverride.Builder() - .setMaxVersionCode(99L) - .setEnabled(true) - .build())); - - compatConfig.addOverrides(config, "com.some.package"); + compatConfig.addPackageOverrides(config, "com.some.package", /* skipUnknownChangeIds */ + true); assertThat(compatConfig.isChangeEnabled(disabledChangeId1, applicationInfo)).isTrue(); assertThat(compatConfig.isChangeEnabled(disabledChangeId2, applicationInfo)).isFalse(); + // Making sure the unknown change ID is still unknown and isChangeEnabled returns true. + assertThat(compatConfig.isKnownChangeId(unknownChangeId)).isFalse(); + assertThat(compatConfig.isChangeEnabled(unknownChangeId, applicationInfo)).isTrue(); } + @Test public void testPreventInstallerSetNonOverridable() throws Exception { final long disabledChangeId1 = 1234L; @@ -326,13 +333,46 @@ public class CompatConfigTest { CompatibilityOverrideConfig config = new CompatibilityOverrideConfig(overrides); assertThrows(SecurityException.class, - () -> compatConfig.addOverrides(config, "com.some.package") + () -> compatConfig.addPackageOverrides(config, "com.some.package", + /* skipUnknownChangeIds */ true) ); assertThat(compatConfig.isChangeEnabled(disabledChangeId1, applicationInfo)).isTrue(); assertThat(compatConfig.isChangeEnabled(disabledChangeId2, applicationInfo)).isFalse(); assertThat(compatConfig.isChangeEnabled(disabledChangeId3, applicationInfo)).isFalse(); } + @Test + public void testCanAddOverridesForUnknownChangeIdOnDebugBuild() throws Exception { + final long disabledChangeId = 1234L; + final long unknownChangeId = 1235L; + // We make disabledChangeId2 non-overridable to make sure it is ignored. + CompatConfig compatConfig = CompatConfigBuilder.create(mBuildClassifier, mContext) + .addDisabledChangeWithId(disabledChangeId) + .build(); + ApplicationInfo applicationInfo = ApplicationInfoBuilder.create() + .withPackageName("com.some.package") + .build(); + PackageManager packageManager = mock(PackageManager.class); + when(mContext.getPackageManager()).thenReturn(packageManager); + when(packageManager.getApplicationInfo(eq("com.some.package"), anyInt())) + .thenReturn(applicationInfo); + + when(mBuildClassifier.isDebuggableBuild()).thenReturn(true); + Map overrides = new HashMap<>(); + overrides.put(disabledChangeId, new PackageOverride.Builder().setEnabled(true).build()); + // Adding an unknown change ID to make sure it isn't skipped if skipUnknownChangeIds is + // false. + overrides.put(unknownChangeId, new PackageOverride.Builder().setEnabled(false).build()); + CompatibilityOverrideConfig config = new CompatibilityOverrideConfig(overrides); + + compatConfig.addPackageOverrides(config, "com.some.package", /* skipUnknownChangeIds */ + false); + assertThat(compatConfig.isChangeEnabled(disabledChangeId, applicationInfo)).isTrue(); + // Making sure the unknown change ID is now known and has an override. + assertThat(compatConfig.isKnownChangeId(unknownChangeId)).isTrue(); + assertThat(compatConfig.isChangeEnabled(unknownChangeId, applicationInfo)).isFalse(); + } + @Test public void testApplyDeferredOverridesAfterInstallingApp() throws Exception { ApplicationInfo applicationInfo = ApplicationInfoBuilder.create() @@ -377,7 +417,8 @@ public class CompatConfigTest { .setMaxVersionCode(99L) .setEnabled(true) .build())); - compatConfig.addOverrides(config, "com.installed.foo"); + compatConfig.addPackageOverrides(config, "com.installed.foo", /* skipUnknownChangeIds */ + true); assertThat(compatConfig.isChangeEnabled(1234L, applicationInfo)).isFalse(); // Add override that does include the installed app version @@ -388,7 +429,8 @@ public class CompatConfigTest { .setMaxVersionCode(100L) .setEnabled(true) .build())); - compatConfig.addOverrides(config, "com.installed.foo"); + compatConfig.addPackageOverrides(config, "com.installed.foo", /* skipUnknownChangeIds */ + true); assertThat(compatConfig.isChangeEnabled(1234L, applicationInfo)).isTrue(); } @@ -411,7 +453,8 @@ public class CompatConfigTest { .setMaxVersionCode(99L) .setEnabled(true) .build())); - compatConfig.addOverrides(config, "com.notinstalled.foo"); + compatConfig.addPackageOverrides(config, "com.notinstalled.foo", /* skipUnknownChangeIds */ + true); assertThat(compatConfig.isChangeEnabled(1234L, applicationInfo)).isFalse(); // Pretend the app is now installed. @@ -557,6 +600,7 @@ public class CompatConfigTest { final long disabledChangeId1 = 1234L; final long disabledChangeId2 = 1235L; final long enabledChangeId = 1236L; + final long unknownChangeId = 1237L; // We make disabledChangeId2 non-overridable to make sure it is ignored. CompatConfig compatConfig = CompatConfigBuilder.create(mBuildClassifier, mContext) .addDisabledOverridableChangeWithId(disabledChangeId1) @@ -583,6 +627,8 @@ public class CompatConfigTest { Set overridesToRemove = new HashSet<>(); overridesToRemove.add(disabledChangeId1); overridesToRemove.add(enabledChangeId); + // Adding an unknown change ID to make sure it's skipped. + overridesToRemove.add(unknownChangeId); CompatibilityOverridesToRemoveConfig config = new CompatibilityOverridesToRemoveConfig( overridesToRemove); @@ -590,6 +636,8 @@ public class CompatConfigTest { assertThat(compatConfig.isChangeEnabled(disabledChangeId1, applicationInfo)).isFalse(); assertThat(compatConfig.isChangeEnabled(disabledChangeId2, applicationInfo)).isTrue(); assertThat(compatConfig.isChangeEnabled(enabledChangeId, applicationInfo)).isTrue(); + // Making sure the unknown change ID is still unknown. + assertThat(compatConfig.isKnownChangeId(unknownChangeId)).isFalse(); } @Test @@ -797,18 +845,18 @@ public class CompatConfigTest { .build()); when(mPackageManager.getApplicationInfo(eq("bar.baz"), anyInt())) .thenThrow(new NameNotFoundException()); - compatConfig.addOverrides( + compatConfig.addPackageOverrides( new CompatibilityOverrideConfig( Collections.singletonMap( 1L, new PackageOverride.Builder().setEnabled(true).build())), - "foo.bar"); - compatConfig.addOverrides( + "foo.bar", /* skipUnknownChangeIds */ true); + compatConfig.addPackageOverrides( new CompatibilityOverrideConfig( Collections.singletonMap( 2L, new PackageOverride.Builder().setEnabled(false).build())), - "bar.baz"); + "bar.baz", /* skipUnknownChangeIds */ true); assertThat(readFile(overridesFile)).isEqualTo("\n" + "\n" @@ -847,12 +895,12 @@ public class CompatConfigTest { compatConfig.forceNonDebuggableFinalForTest(true); compatConfig.initOverrides(overridesFile, new File("")); - compatConfig.addOverrides(new CompatibilityOverrideConfig(Collections.singletonMap(1L, - new PackageOverride.Builder() + compatConfig.addPackageOverrides(new CompatibilityOverrideConfig( + Collections.singletonMap(1L, new PackageOverride.Builder() .setMinVersionCode(99L) .setMaxVersionCode(101L) .setEnabled(true) - .build())), "foo.bar"); + .build())), "foo.bar", /* skipUnknownChangeIds */ true); assertThat(readFile(overridesFile)).isEqualTo("\n" + "\n"