From ce8e3fc1c3bb63771b6448911cfe571fb1ab1d40 Mon Sep 17 00:00:00 2001 From: Songchun Fan Date: Mon, 1 May 2023 16:18:48 -0700 Subject: [PATCH] [SettingsProvider] skip caching in system server The old implementation avoids closing the memory int array if the process is the system process. However, this doesn't prevent the object to be garbage collected. As a result, the array may be left open without being properly closed. This change skips sending the memory int array and index if the query is from inside system server; should also addresses StrictMode complaints. BUG: 277448672 Test: boots Change-Id: I309ad3558a77d8e7d0c8a8d2c8f45db5a316c976 --- core/java/android/provider/Settings.java | 104 +++++++++++++---------- 1 file changed, 58 insertions(+), 46 deletions(-) diff --git a/core/java/android/provider/Settings.java b/core/java/android/provider/Settings.java index 2efb265937807..49425452c3aa0 100644 --- a/core/java/android/provider/Settings.java +++ b/core/java/android/provider/Settings.java @@ -3036,9 +3036,7 @@ public final class Settings { public void destroy() { try { - // If this process is the system server process, mArray is the same object as - // the memory int array kept inside SettingsProvider, so skipping the close() - if (!Settings.isInSystemServer() && !mArray.isClosed()) { + if (!mArray.isClosed()) { mArray.close(); } } catch (IOException e) { @@ -3218,8 +3216,9 @@ public final class Settings { @UnsupportedAppUsage public String getStringForUser(ContentResolver cr, String name, final int userHandle) { final boolean isSelf = (userHandle == UserHandle.myUserId()); + final boolean useCache = isSelf && !isInSystemServer(); boolean needsGenerationTracker = false; - if (isSelf) { + if (useCache) { synchronized (NameValueCache.this) { final GenerationTracker generationTracker = mGenerationTrackers.get(name); if (generationTracker != null) { @@ -3251,8 +3250,10 @@ public final class Settings { needsGenerationTracker = true; } else { if (DEBUG || LOCAL_LOGV) { - Log.v(TAG, "get setting for user " + userHandle - + " by user " + UserHandle.myUserId() + " so skipping cache"); + Log.v(TAG, "get setting " + name + " for user " + userHandle + " by user " + + UserHandle.myUserId() + + (isInSystemServer() ? " in system_server" : "") + + " so skipping cache"); } } @@ -3365,9 +3366,12 @@ public final class Settings { } } } else { - if (LOCAL_LOGV) Log.i(TAG, "call-query of user " + userHandle - + " by " + UserHandle.myUserId() - + " so not updating cache"); + if (DEBUG || LOCAL_LOGV) { + Log.i(TAG, "call-query of user " + userHandle + + " by " + UserHandle.myUserId() + + (isInSystemServer() ? " in system_server" : "") + + " so not updating cache"); + } } return value; } @@ -3440,55 +3444,63 @@ public final class Settings { public ArrayMap getStringsForPrefix(ContentResolver cr, String prefix, List names) { + final boolean useCache = !isInSystemServer(); String namespace = prefix.substring(0, prefix.length() - 1); Config.enforceReadPermission(namespace); ArrayMap keyValues = new ArrayMap<>(); int currentGeneration = -1; boolean needsGenerationTracker = false; - synchronized (NameValueCache.this) { - final GenerationTracker generationTracker = mGenerationTrackers.get(prefix); - if (generationTracker != null) { - if (generationTracker.isGenerationChanged()) { - if (DEBUG) { - Log.i(TAG, "Generation changed for prefix:" + prefix - + " type:" + mUri.getPath() - + " in package:" + cr.getPackageName()); - } - for (int i = mValues.size() - 1; i >= 0; i--) { - String key = mValues.keyAt(i); - if (key.startsWith(prefix)) { - mValues.remove(key); - } - } - } else { - boolean prefixCached = mValues.containsKey(prefix); - if (prefixCached) { + if (useCache) { + synchronized (NameValueCache.this) { + final GenerationTracker generationTracker = mGenerationTrackers.get(prefix); + if (generationTracker != null) { + if (generationTracker.isGenerationChanged()) { if (DEBUG) { - Log.i(TAG, "Cache hit for prefix:" + prefix); + Log.i(TAG, "Generation changed for prefix:" + prefix + + " type:" + mUri.getPath() + + " in package:" + cr.getPackageName()); } - if (!names.isEmpty()) { - for (String name : names) { - if (mValues.containsKey(name)) { - keyValues.put(name, mValues.get(name)); - } - } - } else { - for (int i = 0; i < mValues.size(); ++i) { - String key = mValues.keyAt(i); - // Explicitly exclude the prefix as it is only there to - // signal that the prefix has been cached. - if (key.startsWith(prefix) && !key.equals(prefix)) { - keyValues.put(key, mValues.get(key)); - } + for (int i = mValues.size() - 1; i >= 0; i--) { + String key = mValues.keyAt(i); + if (key.startsWith(prefix)) { + mValues.remove(key); } } - return keyValues; + } else { + boolean prefixCached = mValues.containsKey(prefix); + if (prefixCached) { + if (DEBUG) { + Log.i(TAG, "Cache hit for prefix:" + prefix); + } + if (!names.isEmpty()) { + for (String name : names) { + if (mValues.containsKey(name)) { + keyValues.put(name, mValues.get(name)); + } + } + } else { + for (int i = 0; i < mValues.size(); ++i) { + String key = mValues.keyAt(i); + // Explicitly exclude the prefix as it is only there to + // signal that the prefix has been cached. + if (key.startsWith(prefix) && !key.equals(prefix)) { + keyValues.put(key, mValues.get(key)); + } + } + } + return keyValues; + } } + currentGeneration = generationTracker.getCurrentGeneration(); + } else { + needsGenerationTracker = true; } - currentGeneration = generationTracker.getCurrentGeneration(); - } else { - needsGenerationTracker = true; + } + } else { + if (DEBUG || LOCAL_LOGV) { + Log.v(TAG, "getting settings for prefix " + prefix + " in system_server" + + " so skipping cache"); } }