Merge "Skip unknown change IDs when adding/removing overrides on release builds" into sc-v2-dev

services/core/java/com/android/server/compat/CompatConfig.java

@@ -222,9 +222,16 @@ final class CompatConfig {
      *
      * @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.
      *
      * <p>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.
      *
      * <p>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) {

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<Long> 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.");
             }

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<Long, PackageOverride> overrides = new HashMap<>();
-        CompatibilityOverrideConfig config = new CompatibilityOverrideConfig(
+        overrides.put(disabledChangeId1, new PackageOverride.Builder()
-                Collections.singletonMap(disabledChangeId1,
-                        new PackageOverride.Builder()
                 .setMaxVersionCode(99L)
                 .setEnabled(true)
-                                .build()));
+                .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);
 
-        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<Long, PackageOverride> 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<Long> 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");
+                "foo.bar", /* skipUnknownChangeIds */ true);
-        compatConfig.addOverrides(
+        compatConfig.addPackageOverrides(
                 new CompatibilityOverrideConfig(
                         Collections.singletonMap(
                                 2L,
                                 new PackageOverride.Builder().setEnabled(false).build())),
-                "bar.baz");
+                "bar.baz", /* skipUnknownChangeIds */ true);
 
         assertThat(readFile(overridesFile)).isEqualTo("<?xml version=\"1.0\" encoding=\"utf-8\"?>\n"
                 + "<overrides>\n"
@@ -847,12 +895,12 @@ public class CompatConfigTest {
         compatConfig.forceNonDebuggableFinalForTest(true);
         compatConfig.initOverrides(overridesFile, new File(""));
 
-        compatConfig.addOverrides(new CompatibilityOverrideConfig(Collections.singletonMap(1L,
+        compatConfig.addPackageOverrides(new CompatibilityOverrideConfig(
-                new PackageOverride.Builder()
+                Collections.singletonMap(1L, new PackageOverride.Builder()
                         .setMinVersionCode(99L)
                         .setMaxVersionCode(101L)
                         .setEnabled(true)
-                        .build())), "foo.bar");
+                        .build())), "foo.bar", /* skipUnknownChangeIds */ true);
 
         assertThat(readFile(overridesFile)).isEqualTo("<?xml version=\"1.0\" encoding=\"utf-8\"?>\n"
                 + "<overrides>\n"