From c3c67f1c50f1103da46629066b5ce1c25f81d715 Mon Sep 17 00:00:00 2001 From: Evan Severson Date: Tue, 21 Mar 2023 14:08:17 -0700 Subject: [PATCH] Only add packageOps if they don't already exist in systemReady We were overwriting them when we try to initialize for all remaining packages that didn't have state from the persistence read. Also we need to be sure to add Op objects for codes that weren't added during the earlier recent_accesses round. Test: Boot and dumpsys Verify new testcase with and without fix Verify persisted files in upgrade cases and normal boot cases Fixes: 273813938 Bug: 272810483 Change-Id: Ibf814b9a3318420d49a77c35f0bab7e6763f8953 --- .../android/server/appop/AppOpsService.java | 83 ++++++++++++++----- .../server/appop/AppOpsServiceTest.java | 48 +++++++++++ 2 files changed, 109 insertions(+), 22 deletions(-) diff --git a/services/core/java/com/android/server/appop/AppOpsService.java b/services/core/java/com/android/server/appop/AppOpsService.java index fc22935736a35..965a07b51e123 100644 --- a/services/core/java/com/android/server/appop/AppOpsService.java +++ b/services/core/java/com/android/server/appop/AppOpsService.java @@ -157,10 +157,10 @@ import com.android.server.LockGuard; import com.android.server.SystemServerInitThreadPool; import com.android.server.SystemServiceManager; import com.android.server.pm.PackageList; +import com.android.server.pm.PackageManagerLocal; import com.android.server.pm.UserManagerInternal; import com.android.server.pm.pkg.AndroidPackage; import com.android.server.pm.pkg.PackageState; -import com.android.server.pm.pkg.PackageStateInternal; import com.android.server.pm.pkg.component.ParsedAttribution; import com.android.server.policy.AppOpsPolicy; @@ -367,6 +367,9 @@ public class AppOpsService extends IAppOpsService.Stub { /** Package Manager internal. Access via {@link #getPackageManagerInternal()} */ private @Nullable PackageManagerInternal mPackageManagerInternal; + /** Package Manager local. Access via {@link #getPackageManagerLocal()} */ + private @Nullable PackageManagerLocal mPackageManagerLocal; + /** User Manager internal. Access via {@link #getUserManagerInternal()} */ private @Nullable UserManagerInternal mUserManagerInternal; @@ -1189,42 +1192,64 @@ public class AppOpsService extends IAppOpsService.Stub { /** * Initialize uid state objects for state contained in the checking service. */ - private void initializeUidStates() { + @VisibleForTesting + void initializeUidStates() { UserManagerInternal umi = getUserManagerInternal(); - int[] userIds = umi.getUserIds(); synchronized (this) { - for (int i = 0; i < userIds.length; i++) { - int userId = userIds[i]; - initializeUserUidStatesLocked(userId); + int[] userIds = umi.getUserIds(); + try (PackageManagerLocal.UnfilteredSnapshot snapshot = + getPackageManagerLocal().withUnfilteredSnapshot()) { + Map packageStates = snapshot.getPackageStates(); + for (int i = 0; i < userIds.length; i++) { + int userId = userIds[i]; + initializeUserUidStatesLocked(userId, packageStates); + } } } } private void initializeUserUidStates(int userId) { synchronized (this) { - initializeUserUidStatesLocked(userId); + try (PackageManagerLocal.UnfilteredSnapshot snapshot = + getPackageManagerLocal().withUnfilteredSnapshot()) { + initializeUserUidStatesLocked(userId, snapshot.getPackageStates()); + } } } - private void initializeUserUidStatesLocked(int userId) { - ArrayMap packageStates = - getPackageManagerInternal().getPackageStates(); - for (int j = 0; j < packageStates.size(); j++) { - PackageStateInternal packageState = packageStates.valueAt(j); - int uid = UserHandle.getUid(userId, packageState.getAppId()); - UidState uidState = getUidStateLocked(uid, true); - String packageName = packageStates.keyAt(j); - Ops ops = new Ops(packageName, uidState); - uidState.pkgOps.put(packageName, ops); + private void initializeUserUidStatesLocked(int userId, Map packageStates) { + for (Map.Entry entry : packageStates.entrySet()) { + int appId = entry.getValue().getAppId(); + String packageName = entry.getKey(); - SparseIntArray packageModes = - mAppOpsCheckingService.getNonDefaultPackageModes(packageName, userId); - for (int k = 0; k < packageModes.size(); k++) { - int code = packageModes.get(k); + initializePackageUidStateLocked(userId, appId, packageName); + } + } + + /* + Be careful not to clear any existing data; only want to add objects that don't already exist. + */ + private void initializePackageUidStateLocked(int userId, int appId, String packageName) { + int uid = UserHandle.getUid(userId, appId); + UidState uidState = getUidStateLocked(uid, true); + Ops ops = uidState.pkgOps.get(packageName); + if (ops == null) { + ops = new Ops(packageName, uidState); + uidState.pkgOps.put(packageName, ops); + } + + SparseIntArray packageModes = + mAppOpsCheckingService.getNonDefaultPackageModes(packageName, userId); + for (int k = 0; k < packageModes.size(); k++) { + int code = packageModes.keyAt(k); + + if (ops.indexOfKey(code) < 0) { ops.put(code, new Op(uidState, packageName, code, uid)); } - uidState.evalForegroundOps(); } + + uidState.evalForegroundOps(); } /** @@ -3648,6 +3673,20 @@ public class AppOpsService extends IAppOpsService.Stub { return mPackageManagerInternal; } + /** + * @return {@link PackageManagerLocal} + */ + private @NonNull PackageManagerLocal getPackageManagerLocal() { + if (mPackageManagerLocal == null) { + mPackageManagerLocal = LocalManagerRegistry.getManager(PackageManagerLocal.class); + } + if (mPackageManagerLocal == null) { + throw new IllegalStateException("PackageManagerLocal not loaded"); + } + + return mPackageManagerLocal; + } + /** * @return {@link UserManagerInternal} */ diff --git a/services/tests/mockingservicestests/src/com/android/server/appop/AppOpsServiceTest.java b/services/tests/mockingservicestests/src/com/android/server/appop/AppOpsServiceTest.java index f86e4644d8b91..44ec26ea65e0c 100644 --- a/services/tests/mockingservicestests/src/com/android/server/appop/AppOpsServiceTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/appop/AppOpsServiceTest.java @@ -19,9 +19,11 @@ import static android.app.AppOpsManager.MODE_ALLOWED; import static android.app.AppOpsManager.MODE_ERRORED; import static android.app.AppOpsManager.OP_COARSE_LOCATION; import static android.app.AppOpsManager.OP_FLAGS_ALL; +import static android.app.AppOpsManager.OP_FLAG_SELF; import static android.app.AppOpsManager.OP_READ_SMS; import static android.app.AppOpsManager.OP_WIFI_SCAN; import static android.app.AppOpsManager.OP_WRITE_SMS; +import static android.os.UserHandle.getUserId; import static com.android.dx.mockito.inline.extended.ExtendedMockito.doNothing; import static com.android.dx.mockito.inline.extended.ExtendedMockito.doReturn; @@ -33,12 +35,15 @@ import static com.android.dx.mockito.inline.extended.ExtendedMockito.when; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNotNull; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.nullable; +import android.app.AppOpsManager; import android.app.AppOpsManager.OpEntry; import android.app.AppOpsManager.PackageOps; import android.content.ContentResolver; @@ -48,14 +53,19 @@ import android.os.Handler; import android.os.HandlerThread; import android.os.Process; import android.provider.Settings; +import android.util.ArrayMap; import androidx.test.InstrumentationRegistry; import androidx.test.filters.SmallTest; import androidx.test.runner.AndroidJUnit4; import com.android.dx.mockito.inline.extended.StaticMockitoSession; +import com.android.server.LocalManagerRegistry; import com.android.server.LocalServices; +import com.android.server.pm.PackageManagerLocal; +import com.android.server.pm.UserManagerInternal; import com.android.server.pm.pkg.AndroidPackage; +import com.android.server.pm.pkg.PackageState; import com.android.server.pm.pkg.PackageStateInternal; import org.junit.After; @@ -67,6 +77,7 @@ import org.mockito.quality.Strictness; import java.io.File; import java.util.Collections; import java.util.List; +import java.util.Map; /** * Unit tests for AppOpsService. Covers functionality that is difficult to test using CTS tests @@ -133,6 +144,7 @@ public class AppOpsServiceTest { mMockingSession = mockitoSession() .strictness(Strictness.LENIENT) .spyStatic(LocalServices.class) + .spyStatic(LocalManagerRegistry.class) .spyStatic(Settings.Global.class) .startMocking(); @@ -152,6 +164,23 @@ public class AppOpsServiceTest { doReturn(mockPackageManagerInternal).when( () -> LocalServices.getService(PackageManagerInternal.class)); + PackageManagerLocal mockPackageManagerLocal = mock(PackageManagerLocal.class); + PackageManagerLocal.UnfilteredSnapshot mockUnfilteredSnapshot = + mock(PackageManagerLocal.UnfilteredSnapshot.class); + PackageState mockMyPS = mock(PackageState.class); + ArrayMap packageStates = new ArrayMap<>(); + packageStates.put(sMyPackageName, mockMyPS); + when(mockMyPS.getAppId()).thenReturn(mMyUid); + when(mockUnfilteredSnapshot.getPackageStates()).thenReturn(packageStates); + when(mockPackageManagerLocal.withUnfilteredSnapshot()).thenReturn(mockUnfilteredSnapshot); + doReturn(mockPackageManagerLocal).when( + () -> LocalManagerRegistry.getManager(PackageManagerLocal.class)); + + UserManagerInternal mockUserManagerInternal = mock(UserManagerInternal.class); + when(mockUserManagerInternal.getUserIds()).thenReturn(new int[] {getUserId(mMyUid)}); + doReturn(mockUserManagerInternal).when( + () -> LocalServices.getService(UserManagerInternal.class)); + // Mock behavior to use specific Settings.Global.APPOP_HISTORY_PARAMETERS doReturn(null).when(() -> Settings.Global.getString(any(ContentResolver.class), eq(Settings.Global.APPOP_HISTORY_PARAMETERS))); @@ -337,6 +366,25 @@ public class AppOpsServiceTest { assertThat(getLoggedOps()).isNull(); } + @Test + public void testUidStateInitializationDoesntClearState() throws InterruptedException { + mAppOpsService.setMode(OP_READ_SMS, mMyUid, sMyPackageName, MODE_ALLOWED); + mAppOpsService.noteOperation(OP_READ_SMS, mMyUid, sMyPackageName, null, false, null, false); + mAppOpsService.initializeUidStates(); + List ops = mAppOpsService.getOpsForPackage(mMyUid, sMyPackageName, + new int[]{OP_READ_SMS}); + assertNotNull(ops); + for (int i = 0; i < ops.size(); i++) { + List opEntries = ops.get(i).getOps(); + for (int j = 0; j < opEntries.size(); j++) { + Map attributedOpEntries = opEntries.get( + j).getAttributedOpEntries(); + assertNotEquals(-1, attributedOpEntries.get(null) + .getLastAccessTime(OP_FLAG_SELF)); + } + } + } + private List getLoggedOps() { return mAppOpsService.getOpsForPackage(mMyUid, sMyPackageName, null /* all ops */); }