Fix Settings writes to a different user

Oops.  Stacked bugs:  first, the desired user handle was not properly
being passed from the call() entry point to the database operations;
then on top of that, the client-side cache management was still
looking in the local user's cache for the data, so a request to read
a different user's settings would return the local user's instead if
that key was already known to the local user's cache.

Reads and writes of a different user's settings are now uncached,
so they're relatively much slower.  They're rare, however, so this
is not something to worry about unless we encounter a real world
case where it's a significant factor.

This CL also adds a bit of cross-user settings read/write testing
to the existing provider suite.  These new tests caught both the
known wrong-user-write bug and discovered the client-side cache
bug, so yay.

Finally, the existing wholesale mutual-exclusion approach would
deadlock in certain circumstances due to the fact that the
settings database creation code might have to call out to the
Package Manager while populating the bookmark/shortcut table,
and the Package Manager would then call back into the settings
provider in the course of handling that request.  The synchronization
regime has been significantly tightened up now: now the database
code [which is known to deal with concurrency itself] is allowed
to cope with multiple parallel openers of the same db; this
allows the settings provider to avoid calling out to other parts
of the system even implicitly while its internal lock is held.

Change-Id: Ib77d445b4a2ec658cc5c210830f6977c981f87ed

4 files changed, 153 insertions, 88 deletions

diff --git a/core/java/android/provider/Settings.java b/core/java/android/provider/Settings.java
index 9ea523de7191..c3d3c26fda69 100644
--- a/core/java/android/provider/Settings.java
+++ b/core/java/android/provider/Settings.java
@@ -754,22 +754,29 @@ public final class Settings {
         }
 
         public String getStringForUser(ContentResolver cr, String name, final int userHandle) {
-            long newValuesVersion = SystemProperties.getLong(mVersionSystemProperty, 0);
+            final boolean isSelf = (userHandle == UserHandle.myUserId());
+            if (isSelf) {
+                long newValuesVersion = SystemProperties.getLong(mVersionSystemProperty, 0);
 
-            synchronized (this) {
-                if (mValuesVersion != newValuesVersion) {
-                    if (LOCAL_LOGV || false) {
-                        Log.v(TAG, "invalidate [" + mUri.getLastPathSegment() + "]: current " +
-                                newValuesVersion + " != cached " + mValuesVersion);
-                    }
+                // Our own user's settings data uses a client-side cache
+                synchronized (this) {
+                    if (mValuesVersion != newValuesVersion) {
+                        if (LOCAL_LOGV || false) {
+                            Log.v(TAG, "invalidate [" + mUri.getLastPathSegment() + "]: current "
+                                    + newValuesVersion + " != cached " + mValuesVersion);
+                        }
 
-                    mValues.clear();
-                    mValuesVersion = newValuesVersion;
-                }
+                        mValues.clear();
+                        mValuesVersion = newValuesVersion;
+                    }
 
-                if (mValues.containsKey(name)) {
-                    return mValues.get(name);  // Could be null, that's OK -- negative caching
+                    if (mValues.containsKey(name)) {
+                        return mValues.get(name);  // Could be null, that's OK -- negative caching
+                    }
                 }
+            } else {
+                if (LOCAL_LOGV) Log.v(TAG, "get setting for user " + userHandle
+                        + " by user " + UserHandle.myUserId() + " so skipping cache");
             }
 
             IContentProvider cp = lazyGetProvider(cr);
@@ -788,8 +795,15 @@ public final class Settings {
                     Bundle b = cp.call(mCallGetCommand, name, args);
                     if (b != null) {
                         String value = b.getPairValue();
-                        synchronized (this) {
-                            mValues.put(name, value);
+                        // Don't update our cache for reads of other users' data
+                        if (isSelf) {
+                            synchronized (this) {
+                                mValues.put(name, value);
+                            }
+                        } else {
+                            if (LOCAL_LOGV) Log.i(TAG, "call-query of user " + userHandle
+                                    + " by " + UserHandle.myUserId()
+                                    + " so not updating cache");
                         }
                         return value;
                     }
diff --git a/core/tests/coretests/AndroidManifest.xml b/core/tests/coretests/AndroidManifest.xml
index dcd1bab37c05..41f8536d5244 100644
--- a/core/tests/coretests/AndroidManifest.xml
+++ b/core/tests/coretests/AndroidManifest.xml
@@ -69,6 +69,10 @@
     <uses-permission android:name="com.google.android.googleapps.permission.GOOGLE_AUTH" />
     <uses-permission android:name="com.google.android.googleapps.permission.GOOGLE_AUTH.ALL_SERVICES" />
 
+    <uses-permission android:name="android.permission.MANAGE_USERS" />
+    <uses-permission android:name="android.permission.INTERACT_ACROSS_USERS" />
+    <uses-permission android:name="android.permission.INTERACT_ACROSS_USERS_FULL" />
+
     <uses-permission android:name="android.permission.RECEIVE_SMS"/>
     <uses-permission android:name="android.permission.INTERNET" />
     <uses-permission android:name="android.permission.READ_CONTACTS" />
diff --git a/core/tests/coretests/src/android/provider/SettingsProviderTest.java b/core/tests/coretests/src/android/provider/SettingsProviderTest.java
index 3e96dc45e9c1..6edd2dc361cd 100644
--- a/core/tests/coretests/src/android/provider/SettingsProviderTest.java
+++ b/core/tests/coretests/src/android/provider/SettingsProviderTest.java
@@ -19,11 +19,15 @@ package android.provider;
 import android.content.ContentResolver;
 import android.content.ContentUris;
 import android.content.ContentValues;
+import android.content.Context;
 import android.content.Intent;
 import android.content.pm.PackageManager;
 import android.content.pm.ResolveInfo;
+import android.content.pm.UserInfo;
 import android.database.Cursor;
 import android.net.Uri;
+import android.os.UserHandle;
+import android.os.UserManager;
 import android.provider.Settings;
 import android.test.AndroidTestCase;
 import android.test.suitebuilder.annotation.MediumTest;
@@ -197,6 +201,53 @@ public class SettingsProviderTest extends AndroidTestCase {
                 Settings.Secure.getString(r, Settings.Secure.LOCATION_PROVIDERS_ALLOWED));
      }
 
+    private boolean findUser(UserManager um, int userHandle) {
+        for (UserInfo user : um.getUsers()) {
+            if (user.id == userHandle) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    @MediumTest
+    public void testPerUserSettings() {
+        UserManager um = (UserManager) getContext().getSystemService(Context.USER_SERVICE);
+        ContentResolver r = getContext().getContentResolver();
+
+        // Make sure there's an owner
+        assertTrue(findUser(um, UserHandle.USER_OWNER));
+
+        // create a new user to use for testing
+        UserInfo user = um.createUser("TestUser1", UserInfo.FLAG_GUEST);
+        assertTrue(user != null);
+
+        try {
+            // Write some settings for that user as well as the current user
+            final String TEST_KEY = "test_setting";
+            final int SELF_VALUE = 40;
+            final int OTHER_VALUE = 27;
+
+            Settings.System.putInt(r, TEST_KEY, SELF_VALUE);
+            Settings.System.putIntForUser(r, TEST_KEY, OTHER_VALUE, user.id);
+
+            // Verify that they read back as intended
+            int myValue = Settings.System.getInt(r, TEST_KEY, 0);
+            int otherValue = Settings.System.getIntForUser(r, TEST_KEY, 0, user.id);
+            assertTrue("Running as user " + UserHandle.myUserId()
+                    + " and reading/writing as user " + user.id
+                    + ", expected to read " + SELF_VALUE + " but got " + myValue,
+                    myValue == SELF_VALUE);
+            assertTrue("Running as user " + UserHandle.myUserId()
+                    + " and reading/writing as user " + user.id
+                    + ", expected to read " + OTHER_VALUE + " but got " + otherValue,
+                    otherValue == OTHER_VALUE);
+        } finally {
+            // Tidy up
+            um.removeUser(user.id);
+        }
+    }
+
      @SmallTest
      public void testSettings() {
         assertCanBeHandled(new Intent(Settings.ACTION_ACCESSIBILITY_SETTINGS));
diff --git a/packages/SettingsProvider/src/com/android/providers/settings/SettingsProvider.java b/packages/SettingsProvider/src/com/android/providers/settings/SettingsProvider.java
index e1a5b52510f7..7cc5decdd7ae 100644
--- a/packages/SettingsProvider/src/com/android/providers/settings/SettingsProvider.java
+++ b/packages/SettingsProvider/src/com/android/providers/settings/SettingsProvider.java
@@ -87,6 +87,9 @@ public class SettingsProvider extends ContentProvider {
     private static final SparseArray<AtomicInteger> sKnownMutationsInFlight
             = new SparseArray<AtomicInteger>();
 
+    // Each defined user has their own settings
+    protected final SparseArray<DatabaseHelper> mOpenHelpers = new SparseArray<DatabaseHelper>();
+
     // Over this size we don't reject loading or saving settings but
     // we do consider them broken/malicious and don't keep them in
     // memory at least:
@@ -98,9 +101,6 @@ public class SettingsProvider extends ContentProvider {
     // want to cache the existence of a key, but not store its value.
     private static final Bundle TOO_LARGE_TO_CACHE_MARKER = Bundle.forPair("_dummy", null);
 
-    // Each defined user has their own settings
-    protected final SparseArray<DatabaseHelper> mOpenHelpers = new SparseArray<DatabaseHelper>();
-    //protected DatabaseHelper mOpenHelper;
     private UserManager mUserManager;
     private BackupManager mBackupManager;
 
@@ -411,32 +411,29 @@ public class SettingsProvider extends ContentProvider {
         mBackupManager = new BackupManager(getContext());
         mUserManager = (UserManager) getContext().getSystemService(Context.USER_SERVICE);
 
-        synchronized (this) {
-            establishDbTrackingLocked(UserHandle.USER_OWNER);
-
-            IntentFilter userFilter = new IntentFilter();
-            userFilter.addAction(Intent.ACTION_USER_REMOVED);
-            getContext().registerReceiver(new BroadcastReceiver() {
-                @Override
-                public void onReceive(Context context, Intent intent) {
-                    if (intent.getAction().equals(Intent.ACTION_USER_REMOVED)) {
-                        final int userHandle = intent.getIntExtra(Intent.EXTRA_USER_HANDLE,
-                                UserHandle.USER_OWNER);
-                        if (userHandle != UserHandle.USER_OWNER) {
-                            onUserRemoved(userHandle);
-                        }
+        establishDbTracking(UserHandle.USER_OWNER);
+
+        IntentFilter userFilter = new IntentFilter();
+        userFilter.addAction(Intent.ACTION_USER_REMOVED);
+        getContext().registerReceiver(new BroadcastReceiver() {
+            @Override
+            public void onReceive(Context context, Intent intent) {
+                if (intent.getAction().equals(Intent.ACTION_USER_REMOVED)) {
+                    final int userHandle = intent.getIntExtra(Intent.EXTRA_USER_HANDLE,
+                            UserHandle.USER_OWNER);
+                    if (userHandle != UserHandle.USER_OWNER) {
+                        onUserRemoved(userHandle);
                     }
                 }
-            }, userFilter);
-        }
+            }
+        }, userFilter);
         return true;
     }
 
     void onUserRemoved(int userHandle) {
-        // the db file itself will be deleted automatically, but we need to tear down
-        // our caches and other internal bookkeeping.  Creation/deletion of a user's
-        // settings db infrastructure is synchronized on 'this'
         synchronized (this) {
+            // the db file itself will be deleted automatically, but we need to tear down
+            // our caches and other internal bookkeeping.
             FileObserver observer = sObserverInstances.get(userHandle);
             if (observer != null) {
                 observer.stopWatching();
@@ -455,25 +452,43 @@ public class SettingsProvider extends ContentProvider {
         }
     }
 
-    private void establishDbTrackingLocked(int userHandle) {
+    private void establishDbTracking(int userHandle) {
         if (LOCAL_LOGV) {
             Slog.i(TAG, "Installing settings db helper and caches for user " + userHandle);
         }
 
-        DatabaseHelper dbhelper = new DatabaseHelper(getContext(), userHandle);
-        mOpenHelpers.append(userHandle, dbhelper);
+        DatabaseHelper dbhelper;
 
-        // Watch for external modifications to the database files,
-        // keeping our caches in sync.
-        sSystemCaches.append(userHandle, new SettingsCache(TABLE_SYSTEM));
-        sSecureCaches.append(userHandle, new SettingsCache(TABLE_SECURE));
-        sKnownMutationsInFlight.append(userHandle, new AtomicInteger(0));
+        synchronized (this) {
+            dbhelper = mOpenHelpers.get(userHandle);
+            if (dbhelper == null) {
+                dbhelper = new DatabaseHelper(getContext(), userHandle);
+                mOpenHelpers.append(userHandle, dbhelper);
+
+                sSystemCaches.append(userHandle, new SettingsCache(TABLE_SYSTEM));
+                sSecureCaches.append(userHandle, new SettingsCache(TABLE_SECURE));
+                sKnownMutationsInFlight.append(userHandle, new AtomicInteger(0));
+            }
+        }
+
+        // Initialization of the db *outside* the locks.  It's possible that racing
+        // threads might wind up here, the second having read the cache entries
+        // written by the first, but that's benign: the SQLite helper implementation
+        // manages concurrency itself, and it's important that we not run the db
+        // initialization with any of our own locks held, so we're fine.
         SQLiteDatabase db = dbhelper.getWritableDatabase();
 
-        // Now we can start observing it for changes
-        SettingsFileObserver observer = new SettingsFileObserver(userHandle, db.getPath());
-        sObserverInstances.append(userHandle, observer);
-        observer.startWatching();
+        // Watch for external modifications to the database files,
+        // keeping our caches in sync.  We synchronize the observer set
+        // separately, and of course it has to run after the db file
+        // itself was set up by the DatabaseHelper.
+        synchronized (sObserverInstances) {
+            if (sObserverInstances.get(userHandle) == null) {
+                SettingsFileObserver observer = new SettingsFileObserver(userHandle, db.getPath());
+                sObserverInstances.append(userHandle, observer);
+                observer.startWatching();
+            }
+        }
 
         ensureAndroidIdIsSet(userHandle);
 
@@ -571,19 +586,17 @@ public class SettingsProvider extends ContentProvider {
 
     // Lazy-initialize the settings caches for non-primary users
     private SettingsCache getOrConstructCache(int callingUser, SparseArray<SettingsCache> which) {
-        synchronized (this) {
-            getOrEstablishDatabaseLocked(callingUser); // ignore return value; we don't need it
-            return which.get(callingUser);
-        }
+        getOrEstablishDatabase(callingUser); // ignore return value; we don't need it
+        return which.get(callingUser);
     }
 
     // Lazy initialize the database helper and caches for this user, if necessary
-    private DatabaseHelper getOrEstablishDatabaseLocked(int callingUser) {
+    private DatabaseHelper getOrEstablishDatabase(int callingUser) {
         long oldId = Binder.clearCallingIdentity();
         try {
             DatabaseHelper dbHelper = mOpenHelpers.get(callingUser);
             if (null == dbHelper) {
-                establishDbTrackingLocked(callingUser);
+                establishDbTracking(callingUser);
                 dbHelper = mOpenHelpers.get(callingUser);
             }
             return dbHelper;
@@ -646,25 +659,21 @@ public class SettingsProvider extends ContentProvider {
         // Get methods
         if (Settings.CALL_METHOD_GET_SYSTEM.equals(method)) {
             if (LOCAL_LOGV) Slog.v(TAG, "call(system:" + request + ") for " + callingUser);
-            synchronized (this) {
-                dbHelper = getOrEstablishDatabaseLocked(callingUser);
-                cache = sSystemCaches.get(callingUser);
-            }
+            dbHelper = getOrEstablishDatabase(callingUser);
+            cache = sSystemCaches.get(callingUser);
             return lookupValue(dbHelper, TABLE_SYSTEM, cache, request);
         }
         if (Settings.CALL_METHOD_GET_SECURE.equals(method)) {
             if (LOCAL_LOGV) Slog.v(TAG, "call(secure:" + request + ") for " + callingUser);
-            synchronized (this) {
-                dbHelper = getOrEstablishDatabaseLocked(callingUser);
-                cache = sSecureCaches.get(callingUser);
-            }
+            dbHelper = getOrEstablishDatabase(callingUser);
+            cache = sSecureCaches.get(callingUser);
             return lookupValue(dbHelper, TABLE_SECURE, cache, request);
         }
         if (Settings.CALL_METHOD_GET_GLOBAL.equals(method)) {
             if (LOCAL_LOGV) Slog.v(TAG, "call(global:" + request + ") for " + callingUser);
             // fast path: owner db & cache are immutable after onCreate() so we need not
             // guard on the attempt to look them up
-            return lookupValue(getOrEstablishDatabaseLocked(UserHandle.USER_OWNER), TABLE_GLOBAL,
+            return lookupValue(getOrEstablishDatabase(UserHandle.USER_OWNER), TABLE_GLOBAL,
                     sGlobalCache, request);
         }
 
@@ -678,13 +687,13 @@ public class SettingsProvider extends ContentProvider {
         values.put(Settings.NameValueTable.VALUE, newValue);
         if (Settings.CALL_METHOD_PUT_SYSTEM.equals(method)) {
             if (LOCAL_LOGV) Slog.v(TAG, "call_put(system:" + request + "=" + newValue + ") for " + callingUser);
-            insert(Settings.System.CONTENT_URI, values);
+            insertForUser(Settings.System.CONTENT_URI, values, callingUser);
         } else if (Settings.CALL_METHOD_PUT_SECURE.equals(method)) {
             if (LOCAL_LOGV) Slog.v(TAG, "call_put(secure:" + request + "=" + newValue + ") for " + callingUser);
-            insert(Settings.Secure.CONTENT_URI, values);
+            insertForUser(Settings.Secure.CONTENT_URI, values, callingUser);
         } else if (Settings.CALL_METHOD_PUT_GLOBAL.equals(method)) {
             if (LOCAL_LOGV) Slog.v(TAG, "call_put(global:" + request + "=" + newValue + ") for " + callingUser);
-            insert(Settings.Global.CONTENT_URI, values);
+            insertForUser(Settings.Global.CONTENT_URI, values, callingUser);
         } else {
             Slog.w(TAG, "call() with invalid method: " + method);
         }
@@ -747,10 +756,8 @@ public class SettingsProvider extends ContentProvider {
         if (LOCAL_LOGV) Slog.v(TAG, "query(" + url + ") for user " + forUser);
         SqlArguments args = new SqlArguments(url, where, whereArgs);
         DatabaseHelper dbH;
-        synchronized (this) {
-            dbH = getOrEstablishDatabaseLocked(
-                    TABLE_GLOBAL.equals(args.table) ? UserHandle.USER_OWNER : forUser);
-        }
+        dbH = getOrEstablishDatabase(
+                TABLE_GLOBAL.equals(args.table) ? UserHandle.USER_OWNER : forUser);
         SQLiteDatabase db = dbH.getReadableDatabase();
 
         // The favorites table was moved from this provider to a provider inside Home
@@ -805,11 +812,8 @@ public class SettingsProvider extends ContentProvider {
 
         final AtomicInteger mutationCount = sKnownMutationsInFlight.get(callingUser);
         mutationCount.incrementAndGet();
-        DatabaseHelper dbH;
-        synchronized (this) {
-            dbH = getOrEstablishDatabaseLocked(
-                    TABLE_GLOBAL.equals(args.table) ? UserHandle.USER_OWNER : callingUser);
-        }
+        DatabaseHelper dbH = getOrEstablishDatabase(
+                TABLE_GLOBAL.equals(args.table) ? UserHandle.USER_OWNER : callingUser);
         SQLiteDatabase db = dbH.getWritableDatabase();
         db.beginTransaction();
         try {
@@ -945,10 +949,7 @@ public class SettingsProvider extends ContentProvider {
 
         final AtomicInteger mutationCount = sKnownMutationsInFlight.get(desiredUserHandle);
         mutationCount.incrementAndGet();
-        DatabaseHelper dbH;
-        synchronized (this) {
-            dbH = getOrEstablishDatabaseLocked(desiredUserHandle);
-        }
+        DatabaseHelper dbH = getOrEstablishDatabase(desiredUserHandle);
         SQLiteDatabase db = dbH.getWritableDatabase();
         final long rowId = db.insert(args.table, null, initialValues);
         mutationCount.decrementAndGet();
@@ -956,7 +957,8 @@ public class SettingsProvider extends ContentProvider {
 
         SettingsCache.populate(cache, initialValues);  // before we notify
 
-        if (LOCAL_LOGV) Log.v(TAG, args.table + " <- " + initialValues);
+        if (LOCAL_LOGV) Log.v(TAG, args.table + " <- " + initialValues
+                + " for user " + desiredUserHandle);
         // Note that we use the original url here, not the potentially-rewritten table name
         url = getUriFor(url, initialValues, rowId);
         sendNotify(url, desiredUserHandle);
@@ -979,10 +981,7 @@ public class SettingsProvider extends ContentProvider {
 
         final AtomicInteger mutationCount = sKnownMutationsInFlight.get(callingUser);
         mutationCount.incrementAndGet();
-        DatabaseHelper dbH;
-        synchronized (this) {
-            dbH = getOrEstablishDatabaseLocked(callingUser);
-        }
+        DatabaseHelper dbH = getOrEstablishDatabase(callingUser);
         SQLiteDatabase db = dbH.getWritableDatabase();
         int count = db.delete(args.table, args.where, args.args);
         mutationCount.decrementAndGet();
@@ -1014,10 +1013,7 @@ public class SettingsProvider extends ContentProvider {
 
         final AtomicInteger mutationCount = sKnownMutationsInFlight.get(callingUser);
         mutationCount.incrementAndGet();
-        DatabaseHelper dbH;
-        synchronized (this) {
-            dbH = getOrEstablishDatabaseLocked(callingUser);
-        }
+        DatabaseHelper dbH = getOrEstablishDatabase(callingUser);
         SQLiteDatabase db = dbH.getWritableDatabase();
         int count = db.update(args.table, initialValues, args.where, args.args);
         mutationCount.decrementAndGet();