[DO NOT MERGE] Handle config override via settings correctly

Initialize persistence only after we can read the settings
provider as we need to take into account the current config
potentially set as an override in settings vs using the
hard coded defaults.

Make the last persist time more robust to ensure it always
termines to guard against other unforeseen cases wehere the
persistet files don't match what is expected under the current
config.

Test: atest CtsAppOpsTestCases
Test: atest android.appsecurity.cts.AppOpsTest

Bug: 134093967

Change-Id: I4bd272d3908eb8fc2afcdb7c1bd96b05a182d8c4

services/core/java/com/android/server/appop/AppOpsService.java

@@ -201,7 +201,7 @@ public class AppOpsService extends IAppOpsService.Stub {
     @VisibleForTesting
     final SparseArray<UidState> mUidStates = new SparseArray<>();
 
-    private final HistoricalRegistry mHistoricalRegistry = new HistoricalRegistry(this);
+    final HistoricalRegistry mHistoricalRegistry = new HistoricalRegistry(this);
 
     long mLastRealtime;
 

services/core/java/com/android/server/appop/HistoricalRegistry.java

@@ -108,6 +108,12 @@ import java.util.concurrent.TimeUnit;
  * must be called with the mInMemoryLock, xxxDMLocked suffix means the method
  * must be called with the mOnDiskLock and mInMemoryLock locks acquired in that
  * exact order.
+ * <p>
+ * INITIALIZATION: We can initialize persistence only after the system is ready
+ * as we need to check the optional configuration override from the settings
+ * database which is not initialized at the time the app ops service is created.
+ * This means that all entry points that touch persistence should be short
+ * circuited via isPersistenceInitialized() check.
  */
 // TODO (bug:122218838): Make sure we handle start of epoch time
 // TODO (bug:122218838): Validate changed time is handled correctly
@@ -177,14 +183,33 @@ final class HistoricalRegistry {
 
     // Object managing persistence (read/write)
     @GuardedBy("mOnDiskLock")
-    private Persistence mPersistence = new Persistence(mBaseSnapshotInterval,
+    private Persistence mPersistence;
-            mIntervalCompressionMultiplier);
 
     HistoricalRegistry(@NonNull Object lock) {
         mInMemoryLock = lock;
-        if (mMode != AppOpsManager.HISTORICAL_MODE_DISABLED) {
+    }
-            synchronized (mOnDiskLock) {
+
-                synchronized (mInMemoryLock) {
+    void systemReady(@NonNull ContentResolver resolver) {
+        final Uri uri = Settings.Global.getUriFor(Settings.Global.APPOP_HISTORY_PARAMETERS);
+        resolver.registerContentObserver(uri, false, new ContentObserver(
+                FgThread.getHandler()) {
+            @Override
+            public void onChange(boolean selfChange) {
+                updateParametersFromSetting(resolver);
+            }
+        });
+
+        updateParametersFromSetting(resolver);
+
+        synchronized (mOnDiskLock) {
+            synchronized (mInMemoryLock) {
+                if (mMode != AppOpsManager.HISTORICAL_MODE_DISABLED) {
+                    // Can be uninitialized if there is no config in the settings table.
+                    if (!isPersistenceInitializedMLocked()) {
+                        mPersistence = new Persistence(mBaseSnapshotInterval,
+                                mIntervalCompressionMultiplier);
+                    }
+
                     // When starting always adjust history to now.
                     final long lastPersistTimeMills =
                             mPersistence.getLastPersistTimeMillisDLocked();
@@ -197,16 +222,8 @@ final class HistoricalRegistry {
         }
     }
 
-    void systemReady(@NonNull ContentResolver resolver) {
+    private boolean isPersistenceInitializedMLocked() {
-        updateParametersFromSetting(resolver);
+        return mPersistence != null;
-        final Uri uri = Settings.Global.getUriFor(Settings.Global.APPOP_HISTORY_PARAMETERS);
-        resolver.registerContentObserver(uri, false, new ContentObserver(
-                FgThread.getHandler()) {
-            @Override
-            public void onChange(boolean selfChange) {
-                updateParametersFromSetting(resolver);
-            }
-        });
     }
 
     private void updateParametersFromSetting(@NonNull ContentResolver resolver) {
@@ -274,6 +291,11 @@ final class HistoricalRegistry {
                 makeRelativeToEpochStart(currentOps, nowMillis);
                 currentOps.accept(visitor);
 
+                if(isPersistenceInitializedMLocked()) {
+                    Slog.e(LOG_TAG, "Interaction before persistence initialized");
+                    return;
+                }
+
                 final List<HistoricalOps> ops = mPersistence.readHistoryDLocked();
                 if (ops != null) {
                     // TODO (bug:122218838): Make sure this is properly dumped
@@ -302,12 +324,21 @@ final class HistoricalRegistry {
     void getHistoricalOpsFromDiskRaw(int uid, @NonNull String packageName,
             @Nullable String[] opNames, long beginTimeMillis, long endTimeMillis,
             @OpFlags int flags, @NonNull RemoteCallback callback) {
-        final HistoricalOps result = new HistoricalOps(beginTimeMillis, endTimeMillis);
+        synchronized (mOnDiskLock) {
-        mPersistence.collectHistoricalOpsDLocked(result, uid, packageName, opNames,
+            synchronized (mInMemoryLock) {
-                beginTimeMillis, endTimeMillis, flags);
+                if (!isPersistenceInitializedMLocked()) {
-        final Bundle payload = new Bundle();
+                    Slog.e(LOG_TAG, "Interaction before persistence initialized");
-        payload.putParcelable(AppOpsManager.KEY_HISTORICAL_OPS, result);
+                    callback.sendResult(new Bundle());
-        callback.sendResult(payload);
+                    return;
+                }
+                final HistoricalOps result = new HistoricalOps(beginTimeMillis, endTimeMillis);
+                mPersistence.collectHistoricalOpsDLocked(result, uid, packageName, opNames,
+                        beginTimeMillis, endTimeMillis, flags);
+                final Bundle payload = new Bundle();
+                payload.putParcelable(AppOpsManager.KEY_HISTORICAL_OPS, result);
+                callback.sendResult(payload);
+            }
+        }
     }
 
     void getHistoricalOps(int uid, @NonNull String packageName,
@@ -331,6 +362,12 @@ final class HistoricalRegistry {
             boolean collectOpsFromDisk;
 
             synchronized (mInMemoryLock) {
+                if (!isPersistenceInitializedMLocked()) {
+                    Slog.e(LOG_TAG, "Interaction before persistence initialized");
+                    callback.sendResult(new Bundle());
+                    return;
+                }
+
                 currentOps = getUpdatedPendingHistoricalOpsMLocked(currentTimeMillis);
                 if (!(inMemoryAdjBeginTimeMillis >= currentOps.getEndTimeMillis()
                         || inMemoryAdjEndTimeMillis <= currentOps.getBeginTimeMillis())) {
@@ -374,6 +411,10 @@ final class HistoricalRegistry {
             @UidState int uidState, @OpFlags int flags) {
         synchronized (mInMemoryLock) {
             if (mMode == AppOpsManager.HISTORICAL_MODE_ENABLED_ACTIVE) {
+                if (!isPersistenceInitializedMLocked()) {
+                    Slog.e(LOG_TAG, "Interaction before persistence initialized");
+                    return;
+                }
                 getUpdatedPendingHistoricalOpsMLocked(System.currentTimeMillis())
                         .increaseAccessCount(op, uid, packageName, uidState, flags, 1);
             }
@@ -384,6 +425,10 @@ final class HistoricalRegistry {
             @UidState int uidState, @OpFlags int flags) {
         synchronized (mInMemoryLock) {
             if (mMode == AppOpsManager.HISTORICAL_MODE_ENABLED_ACTIVE) {
+                if (!isPersistenceInitializedMLocked()) {
+                    Slog.e(LOG_TAG, "Interaction before persistence initialized");
+                    return;
+                }
                 getUpdatedPendingHistoricalOpsMLocked(System.currentTimeMillis())
                         .increaseRejectCount(op, uid, packageName, uidState, flags, 1);
             }
@@ -394,6 +439,10 @@ final class HistoricalRegistry {
             @UidState int uidState, @OpFlags int flags, long increment) {
         synchronized (mInMemoryLock) {
             if (mMode == AppOpsManager.HISTORICAL_MODE_ENABLED_ACTIVE) {
+                if (!isPersistenceInitializedMLocked()) {
+                    Slog.e(LOG_TAG, "Interaction before persistence initialized");
+                    return;
+                }
                 getUpdatedPendingHistoricalOpsMLocked(System.currentTimeMillis())
                         .increaseAccessDuration(op, uid, packageName, uidState, flags, increment);
             }
@@ -404,6 +453,8 @@ final class HistoricalRegistry {
             long baseSnapshotInterval, long intervalCompressionMultiplier) {
         synchronized (mOnDiskLock) {
             synchronized (mInMemoryLock) {
+                // NOTE: We allow this call if persistence is not initialized as
+                // it is a part of the persistence initialization process.
                 boolean resampleHistory = false;
                 Slog.i(LOG_TAG, "New history parameters: mode:"
                         + AppOpsManager.historicalModeToString(mMode) + " baseSnapshotInterval:"
@@ -412,7 +463,7 @@ final class HistoricalRegistry {
                 if (mMode != mode) {
                     mMode = mode;
                     if (mMode == AppOpsManager.HISTORICAL_MODE_DISABLED) {
-                        clearHistoryOnDiskLocked();
+                        clearHistoryOnDiskDLocked();
                     }
                 }
                 if (mBaseSnapshotInterval != baseSnapshotInterval) {
@@ -433,6 +484,10 @@ final class HistoricalRegistry {
     void offsetHistory(long offsetMillis) {
         synchronized (mOnDiskLock) {
             synchronized (mInMemoryLock) {
+                if (!isPersistenceInitializedMLocked()) {
+                    Slog.e(LOG_TAG, "Interaction before persistence initialized");
+                    return;
+                }
                 final List<HistoricalOps> history = mPersistence.readHistoryDLocked();
                 clearHistory();
                 if (history != null) {
@@ -453,6 +508,10 @@ final class HistoricalRegistry {
     void addHistoricalOps(HistoricalOps ops) {
         final List<HistoricalOps> pendingWrites;
         synchronized (mInMemoryLock) {
+            if (!isPersistenceInitializedMLocked()) {
+                Slog.e(LOG_TAG, "Interaction before persistence initialized");
+                return;
+            }
             // The history files start from mBaseSnapshotInterval - take this into account.
             ops.offsetBeginAndEndTime(mBaseSnapshotInterval);
             mPendingWrites.offerFirst(ops);
@@ -468,6 +527,10 @@ final class HistoricalRegistry {
     }
 
     void resetHistoryParameters() {
+        if (!isPersistenceInitializedMLocked()) {
+            Slog.e(LOG_TAG, "Interaction before persistence initialized");
+            return;
+        }
         setHistoryParameters(DEFAULT_MODE, DEFAULT_SNAPSHOT_INTERVAL_MILLIS,
                 DEFAULT_COMPRESSION_STEP);
     }
@@ -475,6 +538,10 @@ final class HistoricalRegistry {
     void clearHistory(int uid, String packageName) {
         synchronized (mOnDiskLock) {
             synchronized (mInMemoryLock) {
+                if (!isPersistenceInitializedMLocked()) {
+                    Slog.e(LOG_TAG, "Interaction before persistence initialized");
+                    return;
+                }
                 if (mMode != AppOpsManager.HISTORICAL_MODE_ENABLED_ACTIVE) {
                     return;
                 }
@@ -493,18 +560,24 @@ final class HistoricalRegistry {
 
     void clearHistory() {
         synchronized (mOnDiskLock) {
-            clearHistoryOnDiskLocked();
+            synchronized (mInMemoryLock) {
+                if (!isPersistenceInitializedMLocked()) {
+                    Slog.e(LOG_TAG, "Interaction before persistence initialized");
+                    return;
+                }
+                clearHistoryOnDiskDLocked();
+            }
         }
     }
 
-    private void clearHistoryOnDiskLocked() {
+    private void clearHistoryOnDiskDLocked() {
         BackgroundThread.getHandler().removeMessages(MSG_WRITE_PENDING_HISTORY);
         synchronized (mInMemoryLock) {
             mCurrentHistoricalOps = null;
             mNextPersistDueTimeMillis = System.currentTimeMillis();
             mPendingWrites.clear();
         }
-        mPersistence.clearHistoryDLocked();
+        Persistence.clearHistoryDLocked();
     }
 
     private @NonNull HistoricalOps getUpdatedPendingHistoricalOpsMLocked(long now) {
@@ -639,7 +712,7 @@ final class HistoricalRegistry {
             mIntervalCompressionMultiplier = intervalCompressionMultiplier;
         }
 
-        private final AtomicDirectory mHistoricalAppOpsDir = new AtomicDirectory(
+        private static final AtomicDirectory sHistoricalAppOpsDir = new AtomicDirectory(
                 new File(new File(Environment.getDataSystemDirectory(), "appops"), "history"));
 
         private File generateFile(@NonNull File baseDir, int depth) {
@@ -663,8 +736,8 @@ final class HistoricalRegistry {
             persistHistoricalOpsDLocked(historicalOps);
         }
 
-        void clearHistoryDLocked() {
+        static void clearHistoryDLocked() {
-            mHistoricalAppOpsDir.delete();
+            sHistoricalAppOpsDir.delete();
         }
 
         void persistHistoricalOpsDLocked(@NonNull List<HistoricalOps> ops) {
@@ -673,8 +746,8 @@ final class HistoricalRegistry {
                 enforceOpsWellFormed(ops);
             }
             try {
-                final File newBaseDir = mHistoricalAppOpsDir.startWrite();
+                final File newBaseDir = sHistoricalAppOpsDir.startWrite();
-                final File oldBaseDir = mHistoricalAppOpsDir.getBackupDirectory();
+                final File oldBaseDir = sHistoricalAppOpsDir.getBackupDirectory();
                 final HistoricalFilesInvariant filesInvariant;
                 if (DEBUG) {
                     filesInvariant = new HistoricalFilesInvariant();
@@ -686,10 +759,10 @@ final class HistoricalRegistry {
                 if (DEBUG) {
                     filesInvariant.stopTracking(newBaseDir);
                 }
-                mHistoricalAppOpsDir.finishWrite();
+                sHistoricalAppOpsDir.finishWrite();
             } catch (Throwable t) {
                 wtf("Failed to write historical app ops, restoring backup", t, null);
-                mHistoricalAppOpsDir.failWrite();
+                sHistoricalAppOpsDir.failWrite();
             }
         }
 
@@ -715,22 +788,36 @@ final class HistoricalRegistry {
         long getLastPersistTimeMillisDLocked() {
             File baseDir = null;
             try {
-                baseDir = mHistoricalAppOpsDir.startRead();
+                baseDir = sHistoricalAppOpsDir.startRead();
                 final File[] files = baseDir.listFiles();
                 if (files != null && files.length > 0) {
-                    final Set<File> historyFiles = new ArraySet<>();
+                    File shortestFile = null;
-                    Collections.addAll(historyFiles, files);
+                    for (File candidate : files) {
-                    for (int i = 0;; i++) {
+                        final String candidateName = candidate.getName();
-                        final File file = generateFile(baseDir, i);
+                        if (!candidateName.endsWith(HISTORY_FILE_SUFFIX)) {
-                        if (historyFiles.contains(file)) {
+                            continue;
-                            return file.lastModified();
+                        }
+                        if (shortestFile == null) {
+                            shortestFile = candidate;
+                        } else if (candidateName.length() < shortestFile.getName().length()) {
+                            shortestFile = candidate;
                         }
                     }
+                    if (shortestFile == null) {
+                        return 0;
+                    }
+                    final String shortestNameNoExtension = shortestFile.getName()
+                            .replace(HISTORY_FILE_SUFFIX, "");
+                    try {
+                        return Long.parseLong(shortestNameNoExtension);
+                    } catch (NumberFormatException e) {
+                        return 0;
+                    }
                 }
-                mHistoricalAppOpsDir.finishRead();
+                sHistoricalAppOpsDir.finishRead();
             } catch (Throwable e) {
                 wtf("Error reading historical app ops. Deleting history.", e, baseDir);
-                mHistoricalAppOpsDir.delete();
+                sHistoricalAppOpsDir.delete();
             }
             return 0;
         }
@@ -755,7 +842,7 @@ final class HistoricalRegistry {
                 long filterBeginTimeMillis, long filterEndTimeMillis, @OpFlags int filterFlags) {
             File baseDir = null;
             try {
-                baseDir = mHistoricalAppOpsDir.startRead();
+                baseDir = sHistoricalAppOpsDir.startRead();
                 final HistoricalFilesInvariant filesInvariant;
                 if (DEBUG) {
                     filesInvariant = new HistoricalFilesInvariant();
@@ -770,11 +857,11 @@ final class HistoricalRegistry {
                 if (DEBUG) {
                     filesInvariant.stopTracking(baseDir);
                 }
-                mHistoricalAppOpsDir.finishRead();
+                sHistoricalAppOpsDir.finishRead();
                 return ops;
             } catch (Throwable t) {
                 wtf("Error reading historical app ops. Deleting history.", t, baseDir);
-                mHistoricalAppOpsDir.delete();
+                sHistoricalAppOpsDir.delete();
             }
             return null;
         }
@@ -1241,7 +1328,7 @@ final class HistoricalRegistry {
 
         private void writeHistoricalOpsDLocked(@Nullable List<HistoricalOps> allOps,
                 long intervalOverflowMillis, @NonNull File file) throws IOException {
-            final FileOutputStream output = mHistoricalAppOpsDir.openWrite(file);
+            final FileOutputStream output = sHistoricalAppOpsDir.openWrite(file);
             try {
                 final XmlSerializer serializer = Xml.newSerializer();
                 serializer.setOutput(output, StandardCharsets.UTF_8.name());
@@ -1263,9 +1350,9 @@ final class HistoricalRegistry {
                 }
                 serializer.endTag(null, TAG_HISTORY);
                 serializer.endDocument();
-                mHistoricalAppOpsDir.closeWrite(output);
+                sHistoricalAppOpsDir.closeWrite(output);
             } catch (IOException e) {
-                mHistoricalAppOpsDir.failWrite(output);
+                sHistoricalAppOpsDir.failWrite(output);
                 throw e;
             }
         }

services/tests/servicestests/src/com/android/server/appop/AppOpsServiceTest.java

@@ -37,12 +37,15 @@ import android.os.Handler;
 import android.os.HandlerThread;
 import android.os.Process;
 import android.os.RemoteCallback;
+import android.provider.Settings;
 
 import androidx.test.InstrumentationRegistry;
 import androidx.test.filters.SmallTest;
 import androidx.test.runner.AndroidJUnit4;
 
+import org.junit.AfterClass;
 import org.junit.Before;
+import org.junit.BeforeClass;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
@@ -68,11 +71,14 @@ public class AppOpsServiceTest {
     private File mAppOpsFile;
     private Context mContext;
     private Handler mHandler;
+    private AppOpsManager mAppOpsManager;
     private AppOpsService mAppOpsService;
     private String mMyPackageName;
     private int mMyUid;
     private long mTestStartMillis;
 
+    private static String sDefaultAppopHistoryParameters;
+
     @Before
     public void setUp() {
         mContext = InstrumentationRegistry.getTargetContext();
@@ -88,11 +94,29 @@ public class AppOpsServiceTest {
         mMyPackageName = mContext.getOpPackageName();
         mMyUid = Process.myUid();
 
+        mAppOpsManager = mContext.getSystemService(AppOpsManager.class);
         mAppOpsService = new AppOpsService(mAppOpsFile, mHandler);
+        mAppOpsService.mHistoricalRegistry.systemReady(mContext.getContentResolver());
         mAppOpsService.mContext = mContext;
         mTestStartMillis = System.currentTimeMillis();
     }
 
+    @BeforeClass
+    public static void configureDesiredAppopHistoryParameters() {
+        final Context context = InstrumentationRegistry.getTargetContext();
+        sDefaultAppopHistoryParameters = Settings.Global.getString(context.getContentResolver(),
+                Settings.Global.APPOP_HISTORY_PARAMETERS);
+        Settings.Global.putString(InstrumentationRegistry.getTargetContext().getContentResolver(),
+                Settings.Global.APPOP_HISTORY_PARAMETERS, null);
+    }
+
+    @AfterClass
+    public static void restoreDefaultAppopHistoryParameters() {
+        Settings.Global.putString(InstrumentationRegistry.getTargetContext().getContentResolver(),
+                Settings.Global.APPOP_HISTORY_PARAMETERS,
+                sDefaultAppopHistoryParameters);
+    }
+
     @Test
     public void testGetOpsForPackage_noOpsLogged() {
         assertThat(getLoggedOps()).isNull();