From 3c8c81011048a4147aa399a8f89e1bdd81047ae0 Mon Sep 17 00:00:00 2001 From: Paul Hu Date: Thu, 25 Aug 2022 07:06:16 +0000 Subject: [PATCH 1/3] Running PermissionMonitor#startMonitoring on handler thread Most permission update functions are running on the handler thread. So the startMonitoring() should be running on handler thread as well to ensure the permission won't change while processing CS's works. However, startMonitoring() has a dependency with MultipathPolicyTracker. It needs to call and complete the permission update before calling MultipathPolicyTracker#start. Thus, we need to check the startMonitoring done in the end of systemReadyInternal because we are processing it on the handler thread. Bug: 232048835 Test: atest FrameworksNetTests CtsNetTestCases Change-Id: I41fef14d6c163eb5676eab3ccd43416ea1fe1414 --- .../src/com/android/server/ConnectivityService.java | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index 2535974c19..dffe11d703 100755 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -211,6 +211,7 @@ import android.os.BatteryStatsManager; import android.os.Binder; import android.os.Build; import android.os.Bundle; +import android.os.ConditionVariable; import android.os.Handler; import android.os.HandlerThread; import android.os.IBinder; @@ -3040,7 +3041,11 @@ public class ConnectivityService extends IConnectivityManager.Stub // Calling PermissionMonitor#startMonitoring() in systemReadyInternal() and the // MultipathPolicyTracker.start() is called in NetworkPolicyManagerService#systemReady() // to ensure the tracking will be initialized correctly. - mPermissionMonitor.startMonitoring(); + final ConditionVariable startMonitoringDone = new ConditionVariable(); + mHandler.post(() -> { + mPermissionMonitor.startMonitoring(); + startMonitoringDone.open(); + }); mProxyTracker.loadGlobalProxy(); registerDnsResolverUnsolicitedEventListener(); @@ -3067,6 +3072,11 @@ public class ConnectivityService extends IConnectivityManager.Stub if (SdkLevel.isAtLeastT()) { mBpfNetMaps.setPullAtomCallback(mContext); } + // Wait PermissionMonitor to finish the permission update. Then MultipathPolicyTracker won't + // have permission problem. While CV#block() is unbounded in time and can in principle block + // forever, this replaces a synchronous call to PermissionMonitor#startMonitoring, which + // could have blocked forever too. + startMonitoringDone.block(); } /** From 51f816be4abee6f5ffcedc859deb19ce85ddaa39 Mon Sep 17 00:00:00 2001 From: Paul Hu Date: Thu, 11 Aug 2022 14:43:47 +0000 Subject: [PATCH 2/3] Deal with permission update on handler thread To ensure that permission cannot change while CS is doing works, scoring networks, sending callbacks, etc. So making all permission update are running on handler thread. Bug: 232048835 Test: FrameworksNetTests CtsNetTestCases Change-Id: I5380ec8ff1282a1056d9063848e0fff8b3a570ec --- .../android/server/ConnectivityService.java | 8 +- .../connectivity/PermissionMonitor.java | 32 ++-- .../connectivity/PermissionMonitorTest.java | 157 ++++++++++++------ 3 files changed, 134 insertions(+), 63 deletions(-) diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index dffe11d703..9b233953d2 100755 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -1451,7 +1451,11 @@ public class ConnectivityService extends IConnectivityManager.Stub mCellularRadioTimesharingCapable = mResources.get().getBoolean(R.bool.config_cellular_radio_timesharing_capable); + mNetd = netd; + mBpfNetMaps = mDeps.getBpfNetMaps(mContext, netd); mHandlerThread = mDeps.makeHandlerThread(); + mPermissionMonitor = + new PermissionMonitor(mContext, mNetd, mBpfNetMaps, mHandlerThread); mHandlerThread.start(); mHandler = new InternalHandler(mHandlerThread.getLooper()); mTrackerHandler = new NetworkStateTrackerHandler(mHandlerThread.getLooper()); @@ -1466,8 +1470,6 @@ public class ConnectivityService extends IConnectivityManager.Stub mDnsResolver = Objects.requireNonNull(dnsresolver, "missing IDnsResolver"); mProxyTracker = mDeps.makeProxyTracker(mContext, mHandler); - mNetd = netd; - mBpfNetMaps = mDeps.getBpfNetMaps(mContext, netd); mTelephonyManager = (TelephonyManager) mContext.getSystemService(Context.TELEPHONY_SERVICE); mAppOpsManager = (AppOpsManager) mContext.getSystemService(Context.APP_OPS_SERVICE); mLocationPermissionChecker = mDeps.makeLocationPermissionChecker(mContext); @@ -1497,8 +1499,6 @@ public class ConnectivityService extends IConnectivityManager.Stub mUserManager = (UserManager) context.getSystemService(Context.USER_SERVICE); - mPermissionMonitor = new PermissionMonitor(mContext, mNetd, mBpfNetMaps); - mUserAllContext = mContext.createContextAsUser(UserHandle.ALL, 0 /* flags */); // Listen for user add/removes to inform PermissionMonitor. // Should run on mHandler to avoid any races. diff --git a/service/src/com/android/server/connectivity/PermissionMonitor.java b/service/src/com/android/server/connectivity/PermissionMonitor.java index eee7f3a107..9f1613f5bc 100755 --- a/service/src/com/android/server/connectivity/PermissionMonitor.java +++ b/service/src/com/android/server/connectivity/PermissionMonitor.java @@ -52,6 +52,8 @@ import android.net.INetd; import android.net.UidRange; import android.net.Uri; import android.os.Build; +import android.os.Handler; +import android.os.HandlerThread; import android.os.Process; import android.os.RemoteException; import android.os.ServiceSpecificException; @@ -98,6 +100,7 @@ public class PermissionMonitor { private final Dependencies mDeps; private final Context mContext; private final BpfNetMaps mBpfNetMaps; + private final HandlerThread mThread; private static final ProcessShim sProcessShim = ProcessShimImpl.newInstance(); @@ -259,14 +262,15 @@ public class PermissionMonitor { } public PermissionMonitor(@NonNull final Context context, @NonNull final INetd netd, - @NonNull final BpfNetMaps bpfNetMaps) { - this(context, netd, bpfNetMaps, new Dependencies()); + @NonNull final BpfNetMaps bpfNetMaps, @NonNull final HandlerThread thread) { + this(context, netd, bpfNetMaps, new Dependencies(), thread); } @VisibleForTesting PermissionMonitor(@NonNull final Context context, @NonNull final INetd netd, @NonNull final BpfNetMaps bpfNetMaps, - @NonNull final Dependencies deps) { + @NonNull final Dependencies deps, + @NonNull final HandlerThread thread) { mPackageManager = context.getPackageManager(); mUserManager = (UserManager) context.getSystemService(Context.USER_SERVICE); mSystemConfigManager = context.getSystemService(SystemConfigManager.class); @@ -274,6 +278,14 @@ public class PermissionMonitor { mDeps = deps; mContext = context; mBpfNetMaps = bpfNetMaps; + mThread = thread; + } + + private void ensureRunningOnHandlerThread() { + if (mThread.getLooper().getThread() != Thread.currentThread()) { + throw new IllegalStateException( + "Not running on Handler thread: " + Thread.currentThread().getName()); + } } private int getPackageNetdNetworkPermission(@NonNull final PackageInfo app) { @@ -405,14 +417,14 @@ public class PermissionMonitor { public synchronized void startMonitoring() { log("Monitoring"); + final Handler handler = new Handler(mThread.getLooper()); final Context userAllContext = mContext.createContextAsUser(UserHandle.ALL, 0 /* flags */); final IntentFilter intentFilter = new IntentFilter(); intentFilter.addAction(Intent.ACTION_PACKAGE_ADDED); intentFilter.addAction(Intent.ACTION_PACKAGE_REMOVED); intentFilter.addDataScheme("package"); userAllContext.registerReceiver( - mIntentReceiver, intentFilter, null /* broadcastPermission */, - null /* scheduler */); + mIntentReceiver, intentFilter, null /* broadcastPermission */, handler); // Listen to EXTERNAL_APPLICATIONS_AVAILABLE is that an app becoming available means it may // need to gain a permission. But an app that becomes unavailable can neither gain nor lose @@ -421,23 +433,21 @@ public class PermissionMonitor { final IntentFilter externalIntentFilter = new IntentFilter(Intent.ACTION_EXTERNAL_APPLICATIONS_AVAILABLE); userAllContext.registerReceiver( - mIntentReceiver, externalIntentFilter, null /* broadcastPermission */, - null /* scheduler */); + mIntentReceiver, externalIntentFilter, null /* broadcastPermission */, handler); // Listen for user add/remove. final IntentFilter userIntentFilter = new IntentFilter(); userIntentFilter.addAction(Intent.ACTION_USER_ADDED); userIntentFilter.addAction(Intent.ACTION_USER_REMOVED); userAllContext.registerReceiver( - mIntentReceiver, userIntentFilter, null /* broadcastPermission */, - null /* scheduler */); + mIntentReceiver, userIntentFilter, null /* broadcastPermission */, handler); // Register UIDS_ALLOWED_ON_RESTRICTED_NETWORKS setting observer mDeps.registerContentObserver( userAllContext, Settings.Global.getUriFor(UIDS_ALLOWED_ON_RESTRICTED_NETWORKS), false /* notifyForDescendants */, - new ContentObserver(null) { + new ContentObserver(handler) { @Override public void onChange(boolean selfChange) { onSettingChanged(); @@ -541,6 +551,7 @@ public class PermissionMonitor { } private void sendUidsNetworkPermission(SparseIntArray uids, boolean add) { + ensureRunningOnHandlerThread(); List network = new ArrayList<>(); List system = new ArrayList<>(); for (int i = 0; i < uids.size(); i++) { @@ -1143,6 +1154,7 @@ public class PermissionMonitor { */ @VisibleForTesting void sendAppIdsTrafficPermission(SparseIntArray netdPermissionsAppIds) { + ensureRunningOnHandlerThread(); final ArrayList allPermissionAppIds = new ArrayList<>(); final ArrayList internetPermissionAppIds = new ArrayList<>(); final ArrayList updateStatsPermissionAppIds = new ArrayList<>(); diff --git a/tests/unit/java/com/android/server/connectivity/PermissionMonitorTest.java b/tests/unit/java/com/android/server/connectivity/PermissionMonitorTest.java index 354e79a4a1..8076edbcfd 100644 --- a/tests/unit/java/com/android/server/connectivity/PermissionMonitorTest.java +++ b/tests/unit/java/com/android/server/connectivity/PermissionMonitorTest.java @@ -77,6 +77,8 @@ import android.net.INetd; import android.net.UidRange; import android.net.Uri; import android.os.Build; +import android.os.Handler; +import android.os.HandlerThread; import android.os.Process; import android.os.SystemConfigManager; import android.os.UserHandle; @@ -96,6 +98,7 @@ import com.android.networkstack.apishim.common.ProcessShim; import com.android.server.BpfNetMaps; import com.android.testutils.DevSdkIgnoreRule; import com.android.testutils.DevSdkIgnoreRunner; +import com.android.testutils.HandlerUtils; import org.junit.Before; import org.junit.Test; @@ -153,6 +156,7 @@ public class PermissionMonitorTest { private static final int VERSION_Q = Build.VERSION_CODES.Q; private static final int PERMISSION_TRAFFIC_ALL = PERMISSION_INTERNET | PERMISSION_UPDATE_DEVICE_STATS; + private static final int TIMEOUT_MS = 2_000; @Mock private Context mContext; @Mock private PackageManager mPackageManager; @@ -165,7 +169,7 @@ public class PermissionMonitorTest { private PermissionMonitor mPermissionMonitor; private NetdMonitor mNetdMonitor; private BpfMapMonitor mBpfMapMonitor; - + private HandlerThread mHandlerThread; private ProcessShim mProcessShim = ProcessShimImpl.newInstance(); @Before @@ -195,12 +199,17 @@ public class PermissionMonitorTest { // by default. doReturn(VERSION_Q).when(mDeps).getDeviceFirstSdkInt(); - mPermissionMonitor = new PermissionMonitor(mContext, mNetdService, mBpfNetMaps, mDeps); + mHandlerThread = new HandlerThread("PermissionMonitorTest"); + mPermissionMonitor = new PermissionMonitor( + mContext, mNetdService, mBpfNetMaps, mDeps, mHandlerThread); mNetdMonitor = new NetdMonitor(mNetdService); mBpfMapMonitor = new BpfMapMonitor(mBpfNetMaps); + // Start the HandlerThread after PermissionMonitor created as CS current behavior. + mHandlerThread.start(); + doReturn(List.of()).when(mPackageManager).getInstalledPackagesAsUser(anyInt(), anyInt()); - mPermissionMonitor.onUserAdded(MOCK_USER1); + onUserAdded(MOCK_USER1); } private boolean hasRestrictedNetworkPermission(String partition, int targetSdkVersion, @@ -288,9 +297,39 @@ public class PermissionMonitorTest { doReturn(newPackages).when(mPackageManager).getPackagesForUid(eq(uid)); } + private void startMonitoring() { + processOnHandlerThread(() -> mPermissionMonitor.startMonitoring()); + } + + private void onUserAdded(UserHandle user) { + processOnHandlerThread(() -> mPermissionMonitor.onUserAdded(user)); + } + + private void onUserRemoved(UserHandle user) { + processOnHandlerThread(() -> mPermissionMonitor.onUserRemoved(user)); + } + + private void onPackageAdded(String packageName, int uid) { + processOnHandlerThread(() -> mPermissionMonitor.onPackageAdded(packageName, uid)); + } + + private void onPackageRemoved(String packageName, int uid) { + processOnHandlerThread(() -> mPermissionMonitor.onPackageRemoved(packageName, uid)); + } + + private void sendAppIdsTrafficPermission(SparseIntArray netdPermissionsAppIds) { + processOnHandlerThread(() -> + mPermissionMonitor.sendAppIdsTrafficPermission(netdPermissionsAppIds)); + } + + private void sendPackagePermissionsForAppId(int appId, int permissions) { + processOnHandlerThread(() -> + mPermissionMonitor.sendPackagePermissionsForAppId(appId, permissions)); + } + private void addPackage(String packageName, int uid, String... permissions) throws Exception { buildAndMockPackageInfoWithPermissions(packageName, uid, permissions); - mPermissionMonitor.onPackageAdded(packageName, uid); + processOnHandlerThread(() -> mPermissionMonitor.onPackageAdded(packageName, uid)); } private void removePackage(String packageName, int uid) { @@ -302,7 +341,7 @@ public class PermissionMonitorTest { final String[] newPackages = Arrays.stream(oldPackages).filter(e -> !e.equals(packageName)) .toArray(String[]::new); doReturn(newPackages).when(mPackageManager).getPackagesForUid(eq(uid)); - mPermissionMonitor.onPackageRemoved(packageName, uid); + processOnHandlerThread(() -> mPermissionMonitor.onPackageRemoved(packageName, uid)); } @Test @@ -684,7 +723,7 @@ public class PermissionMonitorTest { CHANGE_NETWORK_STATE); // Add user MOCK_USER1. - mPermissionMonitor.onUserAdded(MOCK_USER1); + onUserAdded(MOCK_USER1); // Add SYSTEM_PACKAGE2, expect only have network permission. addPackageForUsers(new UserHandle[]{MOCK_USER1}, SYSTEM_PACKAGE2, SYSTEM_APPID1); mNetdMonitor.expectNetworkPerm(PERMISSION_NETWORK, new UserHandle[]{MOCK_USER1}, @@ -702,7 +741,7 @@ public class PermissionMonitorTest { doReturn(pkgs).when(mPackageManager).getInstalledPackagesAsUser(eq(GET_PERMISSIONS), eq(MOCK_USER_ID2)); // Add user MOCK_USER2. - mPermissionMonitor.onUserAdded(MOCK_USER2); + onUserAdded(MOCK_USER2); mNetdMonitor.expectNetworkPerm(PERMISSION_SYSTEM, new UserHandle[]{MOCK_USER1, MOCK_USER2}, SYSTEM_APPID1); @@ -743,7 +782,7 @@ public class PermissionMonitorTest { mNetdMonitor.expectNetworkPerm(PERMISSION_NETWORK, new UserHandle[]{MOCK_USER1, MOCK_USER2}, SYSTEM_APPID1); - mPermissionMonitor.onUserRemoved(MOCK_USER1); + onUserRemoved(MOCK_USER1); mNetdMonitor.expectNetworkPerm(PERMISSION_NETWORK, new UserHandle[]{MOCK_USER2}, SYSTEM_APPID1); mNetdMonitor.expectNoNetworkPerm(new UserHandle[]{MOCK_USER1}, SYSTEM_APPID1); @@ -757,7 +796,7 @@ public class PermissionMonitorTest { MOCK_APPID1); // Remove last user, expect no permission change. - mPermissionMonitor.onUserRemoved(MOCK_USER2); + onUserRemoved(MOCK_USER2); mNetdMonitor.expectNoNetworkPerm(new UserHandle[]{MOCK_USER1, MOCK_USER2}, SYSTEM_APPID1, MOCK_APPID1); } @@ -772,7 +811,7 @@ public class PermissionMonitorTest { buildPackageInfo(SYSTEM_PACKAGE2, VPN_UID))) .when(mPackageManager).getInstalledPackagesAsUser(eq(GET_PERMISSIONS), anyInt()); buildAndMockPackageInfoWithPermissions(MOCK_PACKAGE1, MOCK_UID11); - mPermissionMonitor.startMonitoring(); + startMonitoring(); // Every app on user 0 except MOCK_UID12 is subject to the VPN. final Set vpnRange1 = Set.of( new UidRange(0, MOCK_UID12 - 1), @@ -786,9 +825,9 @@ public class PermissionMonitorTest { reset(mBpfNetMaps); // When MOCK_UID11 package is uninstalled and reinstalled, expect Netd to be updated - mPermissionMonitor.onPackageRemoved(MOCK_PACKAGE1, MOCK_UID11); + onPackageRemoved(MOCK_PACKAGE1, MOCK_UID11); verify(mBpfNetMaps).removeUidInterfaceRules(aryEq(new int[]{MOCK_UID11})); - mPermissionMonitor.onPackageAdded(MOCK_PACKAGE1, MOCK_UID11); + onPackageAdded(MOCK_PACKAGE1, MOCK_UID11); verify(mBpfNetMaps).addUidInterfaceRules(eq(ifName), aryEq(new int[]{MOCK_UID11})); reset(mBpfNetMaps); @@ -829,7 +868,7 @@ public class PermissionMonitorTest { buildAndMockPackageInfoWithPermissions(MOCK_PACKAGE1, MOCK_UID11); doReturn(List.of(MOCK_USER1, MOCK_USER2)).when(mUserManager).getUserHandles(eq(true)); - mPermissionMonitor.startMonitoring(); + startMonitoring(); final Set vpnRange = Set.of(UidRange.createForUser(MOCK_USER1), UidRange.createForUser(MOCK_USER2)); mPermissionMonitor.onVpnUidRangesAdded(ifName, vpnRange, VPN_UID); @@ -840,7 +879,7 @@ public class PermissionMonitorTest { verify(mBpfNetMaps).addUidInterfaceRules(eq(ifName), aryEq(new int[]{MOCK_UID21})); // Removed package should have its uid rules removed - mPermissionMonitor.onPackageRemoved(MOCK_PACKAGE1, MOCK_UID11); + onPackageRemoved(MOCK_PACKAGE1, MOCK_UID11); verify(mBpfNetMaps).removeUidInterfaceRules(aryEq(new int[]{MOCK_UID11})); verify(mBpfNetMaps, never()).removeUidInterfaceRules(aryEq(new int[]{MOCK_UID21})); } @@ -864,7 +903,7 @@ public class PermissionMonitorTest { buildPackageInfo(MOCK_PACKAGE2, MOCK_UID12), buildPackageInfo(SYSTEM_PACKAGE2, VPN_UID))) .when(mPackageManager).getInstalledPackagesAsUser(eq(GET_PERMISSIONS), anyInt()); - mPermissionMonitor.startMonitoring(); + startMonitoring(); // Every app on user 0 except MOCK_UID12 is subject to the VPN. final UidRange[] lockdownRange = { new UidRange(0, MOCK_UID12 - 1), @@ -896,7 +935,7 @@ public class PermissionMonitorTest { buildPackageInfo(MOCK_PACKAGE1, MOCK_UID11), buildPackageInfo(SYSTEM_PACKAGE2, VPN_UID))) .when(mPackageManager).getInstalledPackagesAsUser(eq(GET_PERMISSIONS), anyInt()); - mPermissionMonitor.startMonitoring(); + startMonitoring(); // MOCK_UID11 is subject to the VPN. final UidRange range = new UidRange(MOCK_UID11, MOCK_UID11); final UidRange[] lockdownRange = {range}; @@ -941,7 +980,7 @@ public class PermissionMonitorTest { buildPackageInfo(MOCK_PACKAGE1, MOCK_UID11), buildPackageInfo(SYSTEM_PACKAGE2, VPN_UID))) .when(mPackageManager).getInstalledPackagesAsUser(eq(GET_PERMISSIONS), anyInt()); - mPermissionMonitor.startMonitoring(); + startMonitoring(); // MOCK_UID11 is subject to the VPN. final UidRange range = new UidRange(MOCK_UID11, MOCK_UID11); final UidRange[] lockdownRangeDuplicates = {range, range}; @@ -979,7 +1018,7 @@ public class PermissionMonitorTest { .when(mPackageManager).getInstalledPackagesAsUser(eq(GET_PERMISSIONS), anyInt()); doReturn(List.of(MOCK_USER1, MOCK_USER2)).when(mUserManager).getUserHandles(eq(true)); - mPermissionMonitor.startMonitoring(); + startMonitoring(); final UidRange[] lockdownRange = { UidRange.createForUser(MOCK_USER1), UidRange.createForUser(MOCK_USER2) @@ -997,7 +1036,7 @@ public class PermissionMonitorTest { reset(mBpfNetMaps); // Uninstalling package should remove Lockdown rules - mPermissionMonitor.onPackageRemoved(MOCK_PACKAGE1, MOCK_UID11); + onPackageRemoved(MOCK_PACKAGE1, MOCK_UID11); verify(mBpfNetMaps).updateUidLockdownRule(anyInt(), eq(false) /* add */); verify(mBpfNetMaps).updateUidLockdownRule(MOCK_UID11, false /* add */); } @@ -1007,13 +1046,15 @@ public class PermissionMonitorTest { // called multiple times with the uid corresponding to each user. private void addPackageForUsers(UserHandle[] users, String packageName, int appId) { for (final UserHandle user : users) { - mPermissionMonitor.onPackageAdded(packageName, user.getUid(appId)); + processOnHandlerThread(() -> + mPermissionMonitor.onPackageAdded(packageName, user.getUid(appId))); } } private void removePackageForUsers(UserHandle[] users, String packageName, int appId) { for (final UserHandle user : users) { - mPermissionMonitor.onPackageRemoved(packageName, user.getUid(appId)); + processOnHandlerThread(() -> + mPermissionMonitor.onPackageRemoved(packageName, user.getUid(appId))); } } @@ -1039,7 +1080,7 @@ public class PermissionMonitorTest { netdPermissionsAppIds.put(SYSTEM_APPID2, PERMISSION_UPDATE_DEVICE_STATS); // Send the permission information to netd, expect permission updated. - mPermissionMonitor.sendAppIdsTrafficPermission(netdPermissionsAppIds); + sendAppIdsTrafficPermission(netdPermissionsAppIds); mBpfMapMonitor.expectTrafficPerm(PERMISSION_INTERNET, MOCK_APPID1); mBpfMapMonitor.expectTrafficPerm(PERMISSION_NONE, MOCK_APPID2); @@ -1047,16 +1088,16 @@ public class PermissionMonitorTest { mBpfMapMonitor.expectTrafficPerm(PERMISSION_UPDATE_DEVICE_STATS, SYSTEM_APPID2); // Update permission of MOCK_APPID1, expect new permission show up. - mPermissionMonitor.sendPackagePermissionsForAppId(MOCK_APPID1, PERMISSION_TRAFFIC_ALL); + sendPackagePermissionsForAppId(MOCK_APPID1, PERMISSION_TRAFFIC_ALL); mBpfMapMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_APPID1); // Change permissions of SYSTEM_APPID2, expect new permission show up and old permission // revoked. - mPermissionMonitor.sendPackagePermissionsForAppId(SYSTEM_APPID2, PERMISSION_INTERNET); + sendPackagePermissionsForAppId(SYSTEM_APPID2, PERMISSION_INTERNET); mBpfMapMonitor.expectTrafficPerm(PERMISSION_INTERNET, SYSTEM_APPID2); // Revoke permission from SYSTEM_APPID1, expect no permission stored. - mPermissionMonitor.sendPackagePermissionsForAppId(SYSTEM_APPID1, PERMISSION_NONE); + sendPackagePermissionsForAppId(SYSTEM_APPID1, PERMISSION_NONE); mBpfMapMonitor.expectTrafficPerm(PERMISSION_NONE, SYSTEM_APPID1); } @@ -1086,7 +1127,7 @@ public class PermissionMonitorTest { mBpfMapMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_APPID1); when(mPackageManager.getPackagesForUid(MOCK_UID11)).thenReturn(new String[]{}); - mPermissionMonitor.onPackageRemoved(MOCK_PACKAGE1, MOCK_UID11); + onPackageRemoved(MOCK_PACKAGE1, MOCK_UID11); mBpfMapMonitor.expectTrafficPerm(PERMISSION_UNINSTALLED, MOCK_APPID1); } @@ -1096,7 +1137,7 @@ public class PermissionMonitorTest { mBpfMapMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_APPID1); when(mPackageManager.getPackagesForUid(MOCK_UID11)).thenReturn(new String[]{}); - mPermissionMonitor.onPackageRemoved(MOCK_PACKAGE1, MOCK_UID11); + onPackageRemoved(MOCK_PACKAGE1, MOCK_UID11); mBpfMapMonitor.expectTrafficPerm(PERMISSION_UNINSTALLED, MOCK_APPID1); addPackage(MOCK_PACKAGE1, MOCK_UID11, INTERNET); @@ -1124,7 +1165,7 @@ public class PermissionMonitorTest { // Uninstall MOCK_PACKAGE1 and expect only INTERNET permission left. when(mPackageManager.getPackagesForUid(eq(MOCK_UID11))) .thenReturn(new String[]{MOCK_PACKAGE2}); - mPermissionMonitor.onPackageRemoved(MOCK_PACKAGE1, MOCK_UID11); + onPackageRemoved(MOCK_PACKAGE1, MOCK_UID11); mBpfMapMonitor.expectTrafficPerm(PERMISSION_INTERNET, MOCK_APPID1); } @@ -1133,8 +1174,8 @@ public class PermissionMonitorTest { // Use the real context as this test must ensure the *real* system package holds the // necessary permission. final Context realContext = InstrumentationRegistry.getContext(); - final PermissionMonitor monitor = new PermissionMonitor(realContext, mNetdService, - mBpfNetMaps); + final PermissionMonitor monitor = new PermissionMonitor( + realContext, mNetdService, mBpfNetMaps, mHandlerThread); final PackageManager manager = realContext.getPackageManager(); final PackageInfo systemInfo = manager.getPackageInfo(REAL_SYSTEM_PACKAGE_NAME, GET_PERMISSIONS | MATCH_ANY_USER); @@ -1148,7 +1189,7 @@ public class PermissionMonitorTest { when(mSystemConfigManager.getSystemPermissionUids(eq(UPDATE_DEVICE_STATS))) .thenReturn(new int[]{ MOCK_UID12 }); - mPermissionMonitor.startMonitoring(); + startMonitoring(); mBpfMapMonitor.expectTrafficPerm(PERMISSION_INTERNET, MOCK_APPID1); mBpfMapMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_APPID2); } @@ -1165,12 +1206,24 @@ public class PermissionMonitorTest { } return true; }), any(), any()); - return receiverCaptor.getValue(); + final BroadcastReceiver originalReceiver = receiverCaptor.getValue(); + return new BroadcastReceiver() { + @Override + public void onReceive(Context context, Intent intent) { + processOnHandlerThread(() -> originalReceiver.onReceive(context, intent)); + } + }; + } + + private void processOnHandlerThread(Runnable function) { + final Handler handler = mHandlerThread.getThreadHandler(); + handler.post(() -> function.run()); + HandlerUtils.waitForIdle(mHandlerThread, TIMEOUT_MS); } @Test public void testIntentReceiver() throws Exception { - mPermissionMonitor.startMonitoring(); + startMonitoring(); final BroadcastReceiver receiver = expectBroadcastReceiver( Intent.ACTION_PACKAGE_ADDED, Intent.ACTION_PACKAGE_REMOVED); @@ -1197,12 +1250,18 @@ public class PermissionMonitorTest { ArgumentCaptor.forClass(ContentObserver.class); verify(mDeps).registerContentObserver(any(), argThat(uri -> uri.equals(expectedUri)), anyBoolean(), captor.capture()); - return captor.getValue(); + final ContentObserver originalObserver = captor.getValue(); + return new ContentObserver(null) { + @Override + public void onChange(final boolean selfChange) { + processOnHandlerThread(() -> originalObserver.onChange(selfChange)); + } + }; } @Test public void testUidsAllowedOnRestrictedNetworksChanged() throws Exception { - mPermissionMonitor.startMonitoring(); + startMonitoring(); final ContentObserver contentObserver = expectRegisterContentObserver( Settings.Global.getUriFor(UIDS_ALLOWED_ON_RESTRICTED_NETWORKS)); @@ -1234,7 +1293,7 @@ public class PermissionMonitorTest { @Test public void testUidsAllowedOnRestrictedNetworksChangedWithSharedUid() throws Exception { - mPermissionMonitor.startMonitoring(); + startMonitoring(); final ContentObserver contentObserver = expectRegisterContentObserver( Settings.Global.getUriFor(UIDS_ALLOWED_ON_RESTRICTED_NETWORKS)); @@ -1267,7 +1326,7 @@ public class PermissionMonitorTest { @Test public void testUidsAllowedOnRestrictedNetworksChangedWithMultipleUsers() throws Exception { - mPermissionMonitor.startMonitoring(); + startMonitoring(); final ContentObserver contentObserver = expectRegisterContentObserver( Settings.Global.getUriFor(UIDS_ALLOWED_ON_RESTRICTED_NETWORKS)); @@ -1288,7 +1347,7 @@ public class PermissionMonitorTest { buildAndMockPackageInfoWithPermissions(MOCK_PACKAGE2, MOCK_UID22); doReturn(pkgs).when(mPackageManager) .getInstalledPackagesAsUser(eq(GET_PERMISSIONS), eq(MOCK_USER_ID2)); - mPermissionMonitor.onUserAdded(MOCK_USER2); + onUserAdded(MOCK_USER2); // MOCK_APPID1 in MOCK_USER1 should have SYSTEM permission but in MOCK_USER2 should have no // permissions. And MOCK_APPID2 has no permissions in either users. mNetdMonitor.expectNetworkPerm(PERMISSION_SYSTEM, new UserHandle[]{MOCK_USER1}, @@ -1307,7 +1366,7 @@ public class PermissionMonitorTest { mNetdMonitor.expectNoNetworkPerm(new UserHandle[]{MOCK_USER1, MOCK_USER2}, MOCK_APPID1); // Remove user MOCK_USER1 - mPermissionMonitor.onUserRemoved(MOCK_USER1); + onUserRemoved(MOCK_USER1); mNetdMonitor.expectNetworkPerm(PERMISSION_SYSTEM, new UserHandle[]{MOCK_USER2}, MOCK_APPID2); mNetdMonitor.expectNoNetworkPerm(new UserHandle[]{MOCK_USER2}, MOCK_APPID1); @@ -1327,7 +1386,7 @@ public class PermissionMonitorTest { buildPackageInfo(MOCK_PACKAGE1, MOCK_UID11), buildPackageInfo(MOCK_PACKAGE2, MOCK_UID12))) .when(mPackageManager).getInstalledPackagesAsUser(eq(GET_PERMISSIONS), anyInt()); - mPermissionMonitor.startMonitoring(); + startMonitoring(); mNetdMonitor.expectNoNetworkPerm(new UserHandle[]{MOCK_USER1}, MOCK_APPID1, MOCK_APPID2); mBpfMapMonitor.expectTrafficPerm(PERMISSION_NONE, MOCK_APPID1, MOCK_APPID2); @@ -1353,7 +1412,7 @@ public class PermissionMonitorTest { @Test public void testOnExternalApplicationsAvailable_AppsNotRegisteredOnStartMonitoring() throws Exception { - mPermissionMonitor.startMonitoring(); + startMonitoring(); final BroadcastReceiver receiver = expectBroadcastReceiver( Intent.ACTION_EXTERNAL_APPLICATIONS_AVAILABLE); @@ -1386,7 +1445,7 @@ public class PermissionMonitorTest { buildPackageInfo(MOCK_PACKAGE1, MOCK_UID11), buildPackageInfo(MOCK_PACKAGE2, MOCK_UID11))) .when(mPackageManager).getInstalledPackagesAsUser(eq(GET_PERMISSIONS), anyInt()); - mPermissionMonitor.startMonitoring(); + startMonitoring(); mNetdMonitor.expectNoNetworkPerm(new UserHandle[]{MOCK_USER1}, MOCK_APPID1); mBpfMapMonitor.expectTrafficPerm(PERMISSION_NONE, MOCK_APPID1); @@ -1413,7 +1472,7 @@ public class PermissionMonitorTest { buildPackageInfo(MOCK_PACKAGE1, MOCK_UID11), buildPackageInfo(MOCK_PACKAGE2, MOCK_UID11, CHANGE_NETWORK_STATE, INTERNET))) .when(mPackageManager).getInstalledPackagesAsUser(eq(GET_PERMISSIONS), anyInt()); - mPermissionMonitor.startMonitoring(); + startMonitoring(); mNetdMonitor.expectNetworkPerm(PERMISSION_NETWORK, new UserHandle[]{MOCK_USER1}, MOCK_APPID1); mBpfMapMonitor.expectTrafficPerm(PERMISSION_INTERNET, MOCK_APPID1); @@ -1478,7 +1537,7 @@ public class PermissionMonitorTest { private void addUserAndVerifyAppIdsPermissions(UserHandle user, int appId1Perm, int appId2Perm, int appId3Perm) { - mPermissionMonitor.onUserAdded(user); + processOnHandlerThread(() -> mPermissionMonitor.onUserAdded(user)); mBpfMapMonitor.expectTrafficPerm(appId1Perm, MOCK_APPID1); mBpfMapMonitor.expectTrafficPerm(appId2Perm, MOCK_APPID2); mBpfMapMonitor.expectTrafficPerm(appId3Perm, MOCK_APPID3); @@ -1486,7 +1545,7 @@ public class PermissionMonitorTest { private void removeUserAndVerifyAppIdsPermissions(UserHandle user, int appId1Perm, int appId2Perm, int appId3Perm) { - mPermissionMonitor.onUserRemoved(user); + processOnHandlerThread(() -> mPermissionMonitor.onUserRemoved(user)); mBpfMapMonitor.expectTrafficPerm(appId1Perm, MOCK_APPID1); mBpfMapMonitor.expectTrafficPerm(appId2Perm, MOCK_APPID2); mBpfMapMonitor.expectTrafficPerm(appId3Perm, MOCK_APPID3); @@ -1528,8 +1587,8 @@ public class PermissionMonitorTest { @Test public void testAppIdsTrafficPermission_Multiuser_PackageAdded() throws Exception { // Add two users with empty package list. - mPermissionMonitor.onUserAdded(MOCK_USER1); - mPermissionMonitor.onUserAdded(MOCK_USER2); + onUserAdded(MOCK_USER1); + onUserAdded(MOCK_USER2); final int[] netdPermissions = {PERMISSION_NONE, PERMISSION_INTERNET, PERMISSION_UPDATE_DEVICE_STATS, PERMISSION_TRAFFIC_ALL}; @@ -1598,8 +1657,8 @@ public class PermissionMonitorTest { @Test public void testAppIdsTrafficPermission_Multiuser_PackageRemoved() throws Exception { // Add two users with empty package list. - mPermissionMonitor.onUserAdded(MOCK_USER1); - mPermissionMonitor.onUserAdded(MOCK_USER2); + onUserAdded(MOCK_USER1); + onUserAdded(MOCK_USER2); int appId = MOCK_APPID1; // Verify that the permission combination is expected when same appId package is removed on From ea62910ed22d85d1de6e5dbbe8b802d445995ba9 Mon Sep 17 00:00:00 2001 From: Paul Hu Date: Thu, 25 Aug 2022 08:20:16 +0000 Subject: [PATCH 3/3] Add synchronized to UidRange getter and dump methods - The UidRange getter and dump methods which is to access mVpnInterfaceUidRanges and mVpnLockdownUidRanges need to guard with synchronized to ensure the value won't be changed during accessing. - Also make getVpnLockdownUidRanges() to be a package-private method. It's accessed from PermissionMonitorTest only. Bug: 232048835 Test: FrameworksNetTests CtsNetTestCases Change-Id: I850f155a01c309f9d75f5143dc0a46a1505415bd --- .../android/server/connectivity/PermissionMonitor.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/service/src/com/android/server/connectivity/PermissionMonitor.java b/service/src/com/android/server/connectivity/PermissionMonitor.java index 9f1613f5bc..ff979d8f13 100755 --- a/service/src/com/android/server/connectivity/PermissionMonitor.java +++ b/service/src/com/android/server/connectivity/PermissionMonitor.java @@ -1213,13 +1213,13 @@ public class PermissionMonitor { /** Should only be used by unit tests */ @VisibleForTesting - public Set getVpnInterfaceUidRanges(String iface) { + public synchronized Set getVpnInterfaceUidRanges(String iface) { return mVpnInterfaceUidRanges.get(iface); } /** Should only be used by unit tests */ @VisibleForTesting - public Set getVpnLockdownUidRanges() { + synchronized Set getVpnLockdownUidRanges() { return mVpnLockdownUidRanges.getSet(); } @@ -1295,8 +1295,10 @@ public class PermissionMonitor { pw.println(); pw.println("Lockdown filtering rules:"); pw.increaseIndent(); - for (final UidRange range : mVpnLockdownUidRanges.getSet()) { - pw.println("UIDs: " + range); + synchronized (this) { + for (final UidRange range : mVpnLockdownUidRanges.getSet()) { + pw.println("UIDs: " + range); + } } pw.decreaseIndent();