From b0c363b21e2ab5bc8bc2f153442ce0e79f6011d7 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Sat, 15 Dec 2018 11:53:03 -0700 Subject: Local and remote isolated storage feature flags. Moving forward as we start enabling isolated storage in various dogfood groups, we'll need to maintain separate values for the feature flag for both "local" and "remote" opinions. Any strongly expressed local opinion will always take precidence over any remote opinion. Any changes to these feature flags means that we need to invalidate any PackageManager parsed APKs, since PackageParser changes it's output depending on the flag state. Since other feature flags are likely to need this type of invalidation in the future, define the PackageManager cache using a SHA-1 hash of a collection of values that should invalidate the cache. Bug: 112545973 Test: atest android.os.SystemPropertiesTest Change-Id: Ifafcdf15e40e694eb4126e06981aeb82df51da33 --- cmds/sm/src/com/android/commands/sm/Sm.java | 27 ++++++-- core/java/android/os/FileUtils.java | 30 +++------ core/java/android/os/SystemProperties.java | 28 ++++++++- core/java/android/os/storage/StorageManager.java | 4 +- core/java/android/provider/Settings.java | 5 ++ .../java/com/android/internal/util/ArrayUtils.java | 7 +++ core/tests/coretests/src/android/os/OsTests.java | 1 - .../src/android/os/SystemPropertiesTest.java | 51 --------------- .../src/android/provider/SettingsBackupTest.java | 4 +- .../src/android/os/SystemPropertiesTest.java | 29 +++++++-- .../com/android/server/StorageManagerService.java | 73 +++++++++++++++++++--- .../android/server/pm/PackageManagerService.java | 39 ++++++------ 12 files changed, 184 insertions(+), 114 deletions(-) delete mode 100644 core/tests/coretests/src/android/os/SystemPropertiesTest.java diff --git a/cmds/sm/src/com/android/commands/sm/Sm.java b/cmds/sm/src/com/android/commands/sm/Sm.java index 783c8c45d603..be5a5bf6c35a 100644 --- a/cmds/sm/src/com/android/commands/sm/Sm.java +++ b/cmds/sm/src/com/android/commands/sm/Sm.java @@ -282,14 +282,31 @@ public final class Sm { StorageManager.DEBUG_VIRTUAL_DISK); } - public void runIsolatedStorage() throws RemoteException { - final boolean enableIsolatedStorage = Boolean.parseBoolean(nextArg()); + public void runIsolatedStorage() { + final int value; + final int mask = StorageManager.DEBUG_ISOLATED_STORAGE_FORCE_ON + | StorageManager.DEBUG_ISOLATED_STORAGE_FORCE_OFF; + switch (nextArg()) { + case "on": + case "true": + value = StorageManager.DEBUG_ISOLATED_STORAGE_FORCE_ON; + break; + case "off": + value = StorageManager.DEBUG_ISOLATED_STORAGE_FORCE_OFF; + break; + case "default": + case "false": + value = 0; + break; + default: + return; + } + // Toggling isolated-storage state will result in a device reboot. So to avoid this command // from erroring out (DeadSystemException), call setDebugFlags() in a separate thread. new Thread(() -> { try { - mSm.setDebugFlags(enableIsolatedStorage ? StorageManager.DEBUG_ISOLATED_STORAGE : 0, - StorageManager.DEBUG_ISOLATED_STORAGE); + mSm.setDebugFlags(value, mask); } catch (RemoteException e) { Log.e(TAG, "Encountered an error!", e); } @@ -334,7 +351,7 @@ public final class Sm { System.err.println(""); System.err.println(" sm set-emulate-fbe [true|false]"); System.err.println(""); - System.err.println(" sm set-isolated-storage [true|false]"); + System.err.println(" sm set-isolated-storage [on|off|default]"); System.err.println(""); return 1; } diff --git a/core/java/android/os/FileUtils.java b/core/java/android/os/FileUtils.java index 567efa7b7afb..428d9e156e37 100644 --- a/core/java/android/os/FileUtils.java +++ b/core/java/android/os/FileUtils.java @@ -49,6 +49,7 @@ import android.util.Slog; import android.webkit.MimeTypeMap; import com.android.internal.annotations.VisibleForTesting; +import com.android.internal.util.ArrayUtils; import com.android.internal.util.SizedInputStream; import libcore.io.IoUtils; @@ -110,8 +111,6 @@ public class FileUtils { public static final Pattern SAFE_FILENAME_PATTERN = Pattern.compile("[\\w%+,./=_-]+"); } - private static final File[] EMPTY = new File[0]; - // non-final so it can be toggled by Robolectric's ShadowFileUtils private static boolean sEnableCopyOptimizations = true; @@ -1164,35 +1163,20 @@ public class FileUtils { /** {@hide} */ public static @NonNull String[] listOrEmpty(@Nullable File dir) { - if (dir == null) return EmptyArray.STRING; - final String[] res = dir.list(); - if (res != null) { - return res; - } else { - return EmptyArray.STRING; - } + return (dir != null) ? ArrayUtils.defeatNullable(dir.list()) + : EmptyArray.STRING; } /** {@hide} */ public static @NonNull File[] listFilesOrEmpty(@Nullable File dir) { - if (dir == null) return EMPTY; - final File[] res = dir.listFiles(); - if (res != null) { - return res; - } else { - return EMPTY; - } + return (dir != null) ? ArrayUtils.defeatNullable(dir.listFiles()) + : ArrayUtils.EMPTY_FILE; } /** {@hide} */ public static @NonNull File[] listFilesOrEmpty(@Nullable File dir, FilenameFilter filter) { - if (dir == null) return EMPTY; - final File[] res = dir.listFiles(filter); - if (res != null) { - return res; - } else { - return EMPTY; - } + return (dir != null) ? ArrayUtils.defeatNullable(dir.listFiles(filter)) + : ArrayUtils.EMPTY_FILE; } /** {@hide} */ diff --git a/core/java/android/os/SystemProperties.java b/core/java/android/os/SystemProperties.java index bdc776dd3702..cbcf600f0fb8 100644 --- a/core/java/android/os/SystemProperties.java +++ b/core/java/android/os/SystemProperties.java @@ -25,10 +25,15 @@ import android.util.MutableInt; import com.android.internal.annotations.GuardedBy; +import libcore.util.HexEncoding; + +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; - /** * Gives access to the system properties store. The system properties * store contains a list of string key-value pairs. @@ -232,6 +237,27 @@ public class SystemProperties { native_report_sysprop_change(); } + /** + * Return a {@code SHA-1} digest of the given keys and their values as a + * hex-encoded string. The ordering of the incoming keys doesn't change the + * digest result. + * + * @hide + */ + public static @NonNull String digestOf(@NonNull String... keys) { + Arrays.sort(keys); + try { + final MessageDigest digest = MessageDigest.getInstance("SHA-1"); + for (String key : keys) { + final String item = key + "=" + get(key) + "\n"; + digest.update(item.getBytes(StandardCharsets.UTF_8)); + } + return HexEncoding.encodeToString(digest.digest()).toLowerCase(); + } catch (NoSuchAlgorithmException e) { + throw new RuntimeException(e); + } + } + private SystemProperties() { } } diff --git a/core/java/android/os/storage/StorageManager.java b/core/java/android/os/storage/StorageManager.java index d315383a5775..8b1d3c6c839f 100644 --- a/core/java/android/os/storage/StorageManager.java +++ b/core/java/android/os/storage/StorageManager.java @@ -226,7 +226,9 @@ public class StorageManager { /** {@hide} */ public static final int DEBUG_VIRTUAL_DISK = 1 << 5; /** {@hide} */ - public static final int DEBUG_ISOLATED_STORAGE = 1 << 6; + public static final int DEBUG_ISOLATED_STORAGE_FORCE_ON = 1 << 6; + /** {@hide} */ + public static final int DEBUG_ISOLATED_STORAGE_FORCE_OFF = 1 << 7; /** {@hide} */ public static final int FLAG_STORAGE_DE = IInstalld.FLAG_STORAGE_DE; diff --git a/core/java/android/provider/Settings.java b/core/java/android/provider/Settings.java index 3e451fe2cf68..853928c9ef38 100644 --- a/core/java/android/provider/Settings.java +++ b/core/java/android/provider/Settings.java @@ -12932,6 +12932,11 @@ public final class Settings { public static final String CONTENT_CAPTURE_SERVICE_EXPLICITLY_ENABLED = "content_capture_service_explicitly_enabled"; + /** {@hide} */ + public static final String ISOLATED_STORAGE_LOCAL = "isolated_storage_local"; + /** {@hide} */ + public static final String ISOLATED_STORAGE_REMOTE = "isolated_storage_remote"; + /** * Settings to backup. This is here so that it's in the same place as the settings * keys and easy to update. diff --git a/core/java/com/android/internal/util/ArrayUtils.java b/core/java/com/android/internal/util/ArrayUtils.java index f669e94c1779..226b8db1540b 100644 --- a/core/java/com/android/internal/util/ArrayUtils.java +++ b/core/java/com/android/internal/util/ArrayUtils.java @@ -24,6 +24,7 @@ import dalvik.system.VMRuntime; import libcore.util.EmptyArray; +import java.io.File; import java.lang.reflect.Array; import java.util.ArrayList; import java.util.Arrays; @@ -42,6 +43,8 @@ public class ArrayUtils { private static final int CACHE_SIZE = 73; private static Object[] sCache = new Object[CACHE_SIZE]; + public static final File[] EMPTY_FILE = new File[0]; + private ArrayUtils() { /* cannot be instantiated */ } public static byte[] newUnpaddedByteArray(int minLen) { @@ -645,6 +648,10 @@ public class ArrayUtils { return (val != null) ? val : EmptyArray.STRING; } + public static @NonNull File[] defeatNullable(@Nullable File[] val) { + return (val != null) ? val : EMPTY_FILE; + } + /** * Throws {@link ArrayIndexOutOfBoundsException} if the index is out of bounds. * diff --git a/core/tests/coretests/src/android/os/OsTests.java b/core/tests/coretests/src/android/os/OsTests.java index 985fa4f3cfc8..2b841269e5ae 100644 --- a/core/tests/coretests/src/android/os/OsTests.java +++ b/core/tests/coretests/src/android/os/OsTests.java @@ -33,7 +33,6 @@ public class OsTests { suite.addTestSuite(MessageQueueTest.class); suite.addTestSuite(MessengerTest.class); suite.addTestSuite(PatternMatcherTest.class); - suite.addTestSuite(SystemPropertiesTest.class); return suite; } diff --git a/core/tests/coretests/src/android/os/SystemPropertiesTest.java b/core/tests/coretests/src/android/os/SystemPropertiesTest.java deleted file mode 100644 index 25868ce0b702..000000000000 --- a/core/tests/coretests/src/android/os/SystemPropertiesTest.java +++ /dev/null @@ -1,51 +0,0 @@ -/* - * Copyright (C) 2006 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package android.os; - -import static junit.framework.Assert.assertEquals; -import junit.framework.TestCase; - -import android.os.SystemProperties; -import android.test.suitebuilder.annotation.SmallTest; - -public class SystemPropertiesTest extends TestCase { - private static final String KEY = "com.android.frameworks.coretests"; - @SmallTest - public void testProperties() throws Exception { - if (false) { - String value; - - SystemProperties.set(KEY, ""); - value = SystemProperties.get(KEY, "default"); - assertEquals("default", value); - - SystemProperties.set(KEY, "AAA"); - value = SystemProperties.get(KEY, "default"); - assertEquals("AAA", value); - - value = SystemProperties.get(KEY); - assertEquals("AAA", value); - - SystemProperties.set(KEY, ""); - value = SystemProperties.get(KEY, "default"); - assertEquals("default", value); - - value = SystemProperties.get(KEY); - assertEquals("", value); - } - } -} diff --git a/core/tests/coretests/src/android/provider/SettingsBackupTest.java b/core/tests/coretests/src/android/provider/SettingsBackupTest.java index 3a37fb627b26..1f74b7cb51a3 100644 --- a/core/tests/coretests/src/android/provider/SettingsBackupTest.java +++ b/core/tests/coretests/src/android/provider/SettingsBackupTest.java @@ -543,7 +543,9 @@ public class SettingsBackupTest { Settings.Global.CHAINED_BATTERY_ATTRIBUTION_ENABLED, Settings.Global.HIDDEN_API_BLACKLIST_EXEMPTIONS, Settings.Global.BACKUP_AGENT_TIMEOUT_PARAMETERS, - Settings.Global.BACKUP_MULTI_USER_ENABLED); + Settings.Global.BACKUP_MULTI_USER_ENABLED, + Settings.Global.ISOLATED_STORAGE_LOCAL, + Settings.Global.ISOLATED_STORAGE_REMOTE); private static final Set BACKUP_BLACKLISTED_SECURE_SETTINGS = newHashSet( Settings.Secure.ACCESSIBILITY_SOFT_KEYBOARD_MODE, diff --git a/core/tests/systemproperties/src/android/os/SystemPropertiesTest.java b/core/tests/systemproperties/src/android/os/SystemPropertiesTest.java index 933e54e840c5..928351e7de8c 100644 --- a/core/tests/systemproperties/src/android/os/SystemPropertiesTest.java +++ b/core/tests/systemproperties/src/android/os/SystemPropertiesTest.java @@ -16,13 +16,13 @@ package android.os; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.TimeUnit; +import android.test.suitebuilder.annotation.SmallTest; import junit.framework.TestCase; -import android.os.SystemProperties; -import android.test.suitebuilder.annotation.SmallTest; +import java.util.Objects; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; public class SystemPropertiesTest extends TestCase { private static final String KEY = "sys.testkey"; @@ -188,4 +188,25 @@ public class SystemPropertiesTest extends TestCase { fail("InterruptedException"); } } + + @SmallTest + public void testDigestOf() { + final String empty = SystemProperties.digestOf(); + final String finger = SystemProperties.digestOf("ro.build.fingerprint"); + final String fingerBrand = SystemProperties.digestOf( + "ro.build.fingerprint", "ro.product.brand"); + final String brandFinger = SystemProperties.digestOf( + "ro.product.brand", "ro.build.fingerprint"); + + // Shouldn't change over time + assertTrue(Objects.equals(finger, SystemProperties.digestOf("ro.build.fingerprint"))); + + // Different properties means different results + assertFalse(Objects.equals(empty, finger)); + assertFalse(Objects.equals(empty, fingerBrand)); + assertFalse(Objects.equals(finger, fingerBrand)); + + // Same properties means same result + assertTrue(Objects.equals(fingerBrand, brandFinger)); + } } diff --git a/services/core/java/com/android/server/StorageManagerService.java b/services/core/java/com/android/server/StorageManagerService.java index 581d4350d28d..6c62725bb5bd 100644 --- a/services/core/java/com/android/server/StorageManagerService.java +++ b/services/core/java/com/android/server/StorageManagerService.java @@ -57,6 +57,7 @@ import android.app.KeyguardManager; import android.app.admin.SecurityLog; import android.app.usage.StorageStatsManager; import android.content.BroadcastReceiver; +import android.content.ContentResolver; import android.content.Context; import android.content.Intent; import android.content.IntentFilter; @@ -777,6 +778,18 @@ class StorageManagerService extends IStorageManager.Stub } }); refreshZramSettings(); + + // Toggle isolated-enable system property in response to settings + mContext.getContentResolver().registerContentObserver( + Settings.Global.getUriFor(Settings.Global.ISOLATED_STORAGE_REMOTE), + false /*notifyForDescendants*/, + new ContentObserver(null /* current thread */) { + @Override + public void onChange(boolean selfChange) { + refreshIsolatedStorageSettings(); + } + }); + refreshIsolatedStorageSettings(); } /** @@ -802,6 +815,32 @@ class StorageManagerService extends IStorageManager.Stub } } + private void refreshIsolatedStorageSettings() { + final int local = Settings.Global.getInt(mContext.getContentResolver(), + Settings.Global.ISOLATED_STORAGE_LOCAL, 0); + final int remote = Settings.Global.getInt(mContext.getContentResolver(), + Settings.Global.ISOLATED_STORAGE_REMOTE, 0); + + // Walk down precedence chain; we prefer local settings first, then + // remote settings, before finally falling back to hard-coded default. + final boolean res; + if (local == -1) { + res = false; + } else if (local == 1) { + res = true; + } else if (remote == -1) { + res = false; + } else if (remote == 1) { + res = true; + } else { + res = false; + } + + Slog.d(TAG, "Isolated storage local flag " + local + " and remote flag " + + remote + " resolved to " + res); + SystemProperties.set(StorageManager.PROP_ISOLATED_STORAGE, Boolean.toString(res)); + } + /** * MediaProvider has a ton of code that makes assumptions about storage * paths never changing, so we outright kill them to pick up new state. @@ -2208,18 +2247,22 @@ class StorageManagerService extends IStorageManager.Stub } } - if ((mask & StorageManager.DEBUG_ISOLATED_STORAGE) != 0) { - final boolean enabled = (flags & StorageManager.DEBUG_ISOLATED_STORAGE) != 0; + if ((mask & (StorageManager.DEBUG_ISOLATED_STORAGE_FORCE_ON + | StorageManager.DEBUG_ISOLATED_STORAGE_FORCE_OFF)) != 0) { + final int value; + if ((flags & StorageManager.DEBUG_ISOLATED_STORAGE_FORCE_ON) != 0) { + value = 1; + } else if ((flags & StorageManager.DEBUG_ISOLATED_STORAGE_FORCE_OFF) != 0) { + value = -1; + } else { + value = 0; + } final long token = Binder.clearCallingIdentity(); try { - SystemProperties.set(StorageManager.PROP_ISOLATED_STORAGE, - Boolean.toString(enabled)); - - // Some of the storage related permissions get fiddled with during - // package scanning. So, delete the package cache to force PackageManagerService - // to do package scanning. - FileUtils.deleteContents(Environment.getPackageCacheDirectory()); + Settings.Global.putInt(mContext.getContentResolver(), + Settings.Global.ISOLATED_STORAGE_LOCAL, value); + refreshIsolatedStorageSettings(); // Perform hard reboot to kick policy into place mContext.getSystemService(PowerManager.class).reboot(null); @@ -3758,6 +3801,8 @@ class StorageManagerService extends IStorageManager.Stub pw.println(); pw.println("Primary storage UUID: " + mPrimaryStorageUuid); + + pw.println(); final Pair pair = StorageManager.getPrimaryStoragePathAndSize(); if (pair == null) { pw.println("Internal storage total size: N/A"); @@ -3770,8 +3815,18 @@ class StorageManagerService extends IStorageManager.Stub pw.print(DataUnit.MEBIBYTES.toBytes(pair.second)); pw.println(" MiB)"); } + + pw.println(); pw.println("Local unlocked users: " + Arrays.toString(mLocalUnlockedUsers)); pw.println("System unlocked users: " + Arrays.toString(mSystemUnlockedUsers)); + + final ContentResolver cr = mContext.getContentResolver(); + pw.println(); + pw.println("Isolated storage, local feature flag: " + + Settings.Global.getInt(cr, Settings.Global.ISOLATED_STORAGE_LOCAL, 0)); + pw.println("Isolated storage, remote feature flag: " + + Settings.Global.getInt(cr, Settings.Global.ISOLATED_STORAGE_REMOTE, 0)); + pw.println("Isolated storage, resolved: " + StorageManager.hasIsolatedStorage()); } synchronized (mObbMounts) { diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index ae29180a8043..2826e7b8fe2b 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -323,6 +323,7 @@ import dalvik.system.VMRuntime; import libcore.io.IoUtils; import libcore.util.EmptyArray; +import libcore.util.HexEncoding; import org.xmlpull.v1.XmlPullParser; import org.xmlpull.v1.XmlPullParserException; @@ -590,12 +591,6 @@ public class PackageManagerService extends IPackageManager.Stub public static final int REASON_LAST = REASON_SHARED; - /** - * Version number for the package parser cache. Increment this whenever the format or - * extent of cached data changes. See {@code PackageParser#setCacheDir}. - */ - private static final String PACKAGE_PARSER_CACHE_VERSION = "1"; - /** * Whether the package parser cache is enabled. */ @@ -2329,7 +2324,7 @@ public class PackageManagerService extends IPackageManager.Stub } } - mCacheDir = preparePackageParserCache(mIsUpgrade); + mCacheDir = preparePackageParserCache(); // Set flag to monitor and not change apk file paths when // scanning install directories. @@ -3196,7 +3191,7 @@ public class PackageManagerService extends IPackageManager.Stub setUpInstantAppInstallerActivityLP(getInstantAppInstallerLPr()); } - private static File preparePackageParserCache(boolean isUpgrade) { + private static @Nullable File preparePackageParserCache() { if (!DEFAULT_PACKAGE_PARSER_CACHE_ENABLED) { return null; } @@ -3217,17 +3212,25 @@ public class PackageManagerService extends IPackageManager.Stub return null; } - // If this is a system upgrade scenario, delete the contents of the package cache dir. - // This also serves to "GC" unused entries when the package cache version changes (which - // can only happen during upgrades). - if (isUpgrade) { - FileUtils.deleteContents(cacheBaseDir); - } + // There are several items that need to be combined together to safely + // identify cached items. In particular, changing the value of certain + // feature flags should cause us to invalidate any caches. + final String cacheName = SystemProperties.digestOf( + "ro.build.fingerprint", + "persist.sys.isolated_storage"); + // Reconcile cache directories, keeping only what we'd actually use. + for (File cacheDir : FileUtils.listFilesOrEmpty(cacheBaseDir)) { + if (Objects.equals(cacheName, cacheDir.getName())) { + Slog.d(TAG, "Keeping known cache " + cacheDir.getName()); + } else { + Slog.d(TAG, "Destroying unknown cache " + cacheDir.getName()); + FileUtils.deleteContentsAndDir(cacheDir); + } + } - // Return the versioned package cache directory. This is something like - // "/data/system/package_cache/1" - File cacheDir = FileUtils.createDir(cacheBaseDir, PACKAGE_PARSER_CACHE_VERSION); + // Return the versioned package cache directory. + File cacheDir = FileUtils.createDir(cacheBaseDir, cacheName); if (cacheDir == null) { // Something went wrong. Attempt to delete everything and return. @@ -3253,7 +3256,7 @@ public class PackageManagerService extends IPackageManager.Stub File frameworkDir = new File(Environment.getRootDirectory(), "framework"); if (cacheDir.lastModified() < frameworkDir.lastModified()) { FileUtils.deleteContents(cacheBaseDir); - cacheDir = FileUtils.createDir(cacheBaseDir, PACKAGE_PARSER_CACHE_VERSION); + cacheDir = FileUtils.createDir(cacheBaseDir, cacheName); } } -- cgit v1.2.3-59-g8ed1b