From 575f04c6d21b1ea65684739e16d3474ce80a5446 Mon Sep 17 00:00:00 2001 From: Sudheer Shanka Date: Sun, 23 Apr 2023 03:39:21 +0000 Subject: [PATCH] Use pending uid state when calculating network blocked state. Since uid state callbacks are handled asynchronously, it is possible that the network gets restricted before the uid state callback is handled. So, when checking if the uid is in the foreground, also consider any pending uid callback info. Fixes: 278453942 Fixes: 278661002 Test: atest tests/cts/hostside/src/com/android/cts/net/HostsideRestrictBackgroundNetworkTests.java Test: atest tests/cts/hostside/src/com/android/cts/net/HostsideConnOnActivityStartTest.java Test: atest services/tests/servicestests/src/com/android/server/net/NetworkPolicyManagerServiceTest.java Change-Id: Id5b6bd025731dc57952fef5e311d6efef310ff94 --- .../android/net/NetworkPolicyManager.java | 7 + .../net/NetworkPolicyManagerService.java | 87 +++++++--- .../net/NetworkPolicyManagerServiceTest.java | 164 ++++++++++++++++-- 3 files changed, 215 insertions(+), 43 deletions(-) diff --git a/core/java/android/net/NetworkPolicyManager.java b/core/java/android/net/NetworkPolicyManager.java index efed6883a1147..365f9130ddd7f 100644 --- a/core/java/android/net/NetworkPolicyManager.java +++ b/core/java/android/net/NetworkPolicyManager.java @@ -16,6 +16,7 @@ package android.net; +import static android.app.ActivityManager.PROCESS_STATE_UNKNOWN; import static android.app.ActivityManager.procStateToString; import static android.content.pm.PackageManager.GET_SIGNATURES; @@ -805,6 +806,9 @@ public class NetworkPolicyManager { /** @hide */ public static boolean isProcStateAllowedWhileIdleOrPowerSaveMode( int procState, @ProcessCapability int capability) { + if (procState == PROCESS_STATE_UNKNOWN) { + return false; + } return procState <= FOREGROUND_THRESHOLD_STATE || (capability & ActivityManager.PROCESS_CAPABILITY_POWER_RESTRICTED_NETWORK) != 0; } @@ -832,6 +836,9 @@ public class NetworkPolicyManager { /** @hide */ public static boolean isProcStateAllowedWhileOnRestrictBackground(int procState, @ProcessCapability int capabilities) { + if (procState == PROCESS_STATE_UNKNOWN) { + return false; + } return procState <= FOREGROUND_THRESHOLD_STATE // This is meant to be a user-initiated job, and therefore gets similar network // access to FGS. diff --git a/services/core/java/com/android/server/net/NetworkPolicyManagerService.java b/services/core/java/com/android/server/net/NetworkPolicyManagerService.java index a3d89e73c2e0c..b7082690276c9 100644 --- a/services/core/java/com/android/server/net/NetworkPolicyManagerService.java +++ b/services/core/java/com/android/server/net/NetworkPolicyManagerService.java @@ -28,6 +28,7 @@ import static android.Manifest.permission.READ_PHONE_STATE; import static android.Manifest.permission.READ_PRIVILEGED_PHONE_STATE; import static android.app.ActivityManager.PROCESS_STATE_UNKNOWN; import static android.app.ActivityManager.isProcStateConsideredInteraction; +import static android.app.ActivityManager.printCapabilitiesSummary; import static android.app.ActivityManager.procStateToString; import static android.app.PendingIntent.FLAG_IMMUTABLE; import static android.app.PendingIntent.FLAG_UPDATE_CURRENT; @@ -1133,6 +1134,7 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub { if (!callbackInfo.isPending) { mUidEventHandler.obtainMessage(UID_MSG_STATE_CHANGED, callbackInfo) .sendToTarget(); + callbackInfo.isPending = true; } } } @@ -1156,6 +1158,19 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub { this.procStateSeq = procStateSeq; this.capability = capability; } + + @Override + public String toString() { + final StringBuilder sb = new StringBuilder(); + sb.append("{"); + sb.append("uid=").append(uid).append(","); + sb.append("proc_state=").append(procStateToString(procState)).append(","); + sb.append("seq=").append(procStateSeq).append(","); + sb.append("cap="); printCapabilitiesSummary(sb, capability); sb.append(","); + sb.append("pending=").append(isPending); + sb.append("}"); + return sb.toString(); + } } final private BroadcastReceiver mPowerSaveWhitelistReceiver = new BroadcastReceiver() { @@ -3982,31 +3997,31 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub { synchronized (mUidBlockedState) { collectKeys(mUidBlockedState, knownUids); } + synchronized (mUidStateCallbackInfos) { + collectKeys(mUidStateCallbackInfos, knownUids); + } fout.println("Status for all known UIDs:"); fout.increaseIndent(); size = knownUids.size(); for (int i = 0; i < size; i++) { final int uid = knownUids.keyAt(i); - fout.print("UID="); - fout.print(uid); + fout.print("UID", uid); final UidState uidState = mUidState.get(uid); - if (uidState == null) { - fout.print(" state={null}"); - } else { - fout.print(" state="); - fout.print(uidState.toString()); - } + fout.print("state", uidState); synchronized (mUidBlockedState) { final UidBlockedState uidBlockedState = mUidBlockedState.get(uid); - if (uidBlockedState == null) { - fout.print(" blocked_state={null}"); - } else { - fout.print(" blocked_state="); - fout.print(uidBlockedState); - } + fout.print("blocked_state", uidBlockedState); + } + + synchronized (mUidStateCallbackInfos) { + final UidStateCallbackInfo callbackInfo = mUidStateCallbackInfos.get(uid); + fout.println(); + fout.increaseIndent(); + fout.print("callback_info", callbackInfo); + fout.decreaseIndent(); } fout.println(); } @@ -4052,27 +4067,49 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub { } @VisibleForTesting - boolean isUidForeground(int uid) { - synchronized (mUidRulesFirstLock) { - return isProcStateAllowedWhileIdleOrPowerSaveMode(mUidState.get(uid)); + @GuardedBy("mUidRulesFirstLock") + boolean isUidForegroundOnRestrictBackgroundUL(int uid) { + final UidState uidState = mUidState.get(uid); + if (isProcStateAllowedWhileOnRestrictBackground(uidState)) { + return true; } + // Check if there is any pending state change. + synchronized (mUidStateCallbackInfos) { + final UidStateCallbackInfo callbackInfo = mUidStateCallbackInfos.get(uid); + final long prevProcStateSeq = uidState != null ? uidState.procStateSeq : -1; + if (callbackInfo != null && callbackInfo.isPending + && callbackInfo.procStateSeq >= prevProcStateSeq) { + return isProcStateAllowedWhileOnRestrictBackground(callbackInfo.procState, + callbackInfo.capability); + } + } + return false; } + @VisibleForTesting @GuardedBy("mUidRulesFirstLock") - private boolean isUidForegroundOnRestrictBackgroundUL(int uid) { + boolean isUidForegroundOnRestrictPowerUL(int uid) { final UidState uidState = mUidState.get(uid); - return isProcStateAllowedWhileOnRestrictBackground(uidState); - } - - @GuardedBy("mUidRulesFirstLock") - private boolean isUidForegroundOnRestrictPowerUL(int uid) { - final UidState uidState = mUidState.get(uid); - return isProcStateAllowedWhileIdleOrPowerSaveMode(uidState); + if (isProcStateAllowedWhileIdleOrPowerSaveMode(uidState)) { + return true; + } + // Check if there is any pending state change. + synchronized (mUidStateCallbackInfos) { + final UidStateCallbackInfo callbackInfo = mUidStateCallbackInfos.get(uid); + final long prevProcStateSeq = uidState != null ? uidState.procStateSeq : -1; + if (callbackInfo != null && callbackInfo.isPending + && callbackInfo.procStateSeq >= prevProcStateSeq) { + return isProcStateAllowedWhileIdleOrPowerSaveMode(callbackInfo.procState, + callbackInfo.capability); + } + } + return false; } @GuardedBy("mUidRulesFirstLock") private boolean isUidTop(int uid) { final UidState uidState = mUidState.get(uid); + // TODO: Consider taking pending uid state change into account. return isProcStateAllowedWhileInLowPowerStandby(uidState); } diff --git a/services/tests/servicestests/src/com/android/server/net/NetworkPolicyManagerServiceTest.java b/services/tests/servicestests/src/com/android/server/net/NetworkPolicyManagerServiceTest.java index b7f90d43881bd..c04df30cb6fdb 100644 --- a/services/tests/servicestests/src/com/android/server/net/NetworkPolicyManagerServiceTest.java +++ b/services/tests/servicestests/src/com/android/server/net/NetworkPolicyManagerServiceTest.java @@ -18,6 +18,12 @@ package com.android.server.net; import static android.Manifest.permission.CONNECTIVITY_USE_RESTRICTED_NETWORKS; import static android.Manifest.permission.NETWORK_STACK; +import static android.app.ActivityManager.PROCESS_CAPABILITY_NONE; +import static android.app.ActivityManager.PROCESS_CAPABILITY_POWER_RESTRICTED_NETWORK; +import static android.app.ActivityManager.PROCESS_CAPABILITY_USER_RESTRICTED_NETWORK; +import static android.app.ActivityManager.PROCESS_STATE_IMPORTANT_FOREGROUND; +import static android.app.ActivityManager.PROCESS_STATE_SERVICE; +import static android.app.ActivityManager.PROCESS_STATE_TOP; import static android.net.ConnectivityManager.BLOCKED_METERED_REASON_DATA_SAVER; import static android.net.ConnectivityManager.BLOCKED_METERED_REASON_USER_RESTRICTED; import static android.net.ConnectivityManager.BLOCKED_REASON_APP_STANDBY; @@ -396,6 +402,26 @@ public class NetworkPolicyManagerServiceTest { } } + // TODO: Use TestLooperManager instead. + /** + * Helper that leverages try-with-resources to pause dispatch of + * {@link #mHandlerThread} until released. + */ + static class SyncBarrier implements AutoCloseable { + private final int mToken; + private Handler mHandler; + + SyncBarrier(Handler handler) { + mHandler = handler; + mToken = mHandler.getLooper().getQueue().postSyncBarrier(); + } + + @Override + public void close() throws Exception { + mHandler.getLooper().getQueue().removeSyncBarrier(mToken); + } + } + @Before public void callSystemReady() throws Exception { MockitoAnnotations.initMocks(this); @@ -1018,25 +1044,113 @@ public class NetworkPolicyManagerServiceTest { // don't check for side-effects (like calls to NetworkManagementService) neither cover all // different modes (Data Saver, Battery Saver, Doze, App idle, etc...). // These scenarios are extensively tested on CTS' HostsideRestrictBackgroundNetworkTests. + @SuppressWarnings("GuardedBy") @Test public void testUidForeground() throws Exception { // push all uids into background long procStateSeq = 0; - callOnUidStateChanged(UID_A, ActivityManager.PROCESS_STATE_SERVICE, procStateSeq++); - callOnUidStateChanged(UID_B, ActivityManager.PROCESS_STATE_SERVICE, procStateSeq++); - assertFalse(mService.isUidForeground(UID_A)); - assertFalse(mService.isUidForeground(UID_B)); + callAndWaitOnUidStateChanged(UID_A, PROCESS_STATE_SERVICE, procStateSeq++); + callAndWaitOnUidStateChanged(UID_B, PROCESS_STATE_SERVICE, procStateSeq++); + assertFalse(mService.isUidForegroundOnRestrictPowerUL(UID_A)); + assertFalse(mService.isUidForegroundOnRestrictBackgroundUL(UID_A)); + assertFalse(mService.isUidForegroundOnRestrictPowerUL(UID_B)); + assertFalse(mService.isUidForegroundOnRestrictBackgroundUL(UID_B)); // push one of the uids into foreground - callOnUidStateChanged(UID_A, ActivityManager.PROCESS_STATE_TOP, procStateSeq++); - assertTrue(mService.isUidForeground(UID_A)); - assertFalse(mService.isUidForeground(UID_B)); + callAndWaitOnUidStateChanged(UID_A, PROCESS_STATE_TOP, procStateSeq++); + assertTrue(mService.isUidForegroundOnRestrictPowerUL(UID_A)); + assertTrue(mService.isUidForegroundOnRestrictBackgroundUL(UID_A)); + assertFalse(mService.isUidForegroundOnRestrictPowerUL(UID_B)); + assertFalse(mService.isUidForegroundOnRestrictBackgroundUL(UID_B)); // and swap another uid into foreground - callOnUidStateChanged(UID_A, ActivityManager.PROCESS_STATE_SERVICE, procStateSeq++); - callOnUidStateChanged(UID_B, ActivityManager.PROCESS_STATE_TOP, procStateSeq++); - assertFalse(mService.isUidForeground(UID_A)); - assertTrue(mService.isUidForeground(UID_B)); + callAndWaitOnUidStateChanged(UID_A, PROCESS_STATE_SERVICE, procStateSeq++); + callAndWaitOnUidStateChanged(UID_B, PROCESS_STATE_TOP, procStateSeq++); + assertFalse(mService.isUidForegroundOnRestrictPowerUL(UID_A)); + assertFalse(mService.isUidForegroundOnRestrictBackgroundUL(UID_A)); + assertTrue(mService.isUidForegroundOnRestrictPowerUL(UID_B)); + assertTrue(mService.isUidForegroundOnRestrictBackgroundUL(UID_B)); + + // change capability of an uid to allow access to power restricted network + callAndWaitOnUidStateChanged(UID_A, PROCESS_STATE_SERVICE, procStateSeq++, + PROCESS_CAPABILITY_POWER_RESTRICTED_NETWORK); + callAndWaitOnUidStateChanged(UID_B, PROCESS_STATE_SERVICE, procStateSeq++, + PROCESS_CAPABILITY_NONE); + assertTrue(mService.isUidForegroundOnRestrictPowerUL(UID_A)); + assertFalse(mService.isUidForegroundOnRestrictBackgroundUL(UID_A)); + assertFalse(mService.isUidForegroundOnRestrictPowerUL(UID_B)); + assertFalse(mService.isUidForegroundOnRestrictBackgroundUL(UID_B)); + + // change capability of an uid to allow access to user restricted network + callAndWaitOnUidStateChanged(UID_A, PROCESS_STATE_IMPORTANT_FOREGROUND, procStateSeq++, + PROCESS_CAPABILITY_USER_RESTRICTED_NETWORK); + callAndWaitOnUidStateChanged(UID_B, PROCESS_STATE_SERVICE, procStateSeq++, + PROCESS_CAPABILITY_NONE); + assertFalse(mService.isUidForegroundOnRestrictPowerUL(UID_A)); + assertTrue(mService.isUidForegroundOnRestrictBackgroundUL(UID_A)); + assertFalse(mService.isUidForegroundOnRestrictPowerUL(UID_B)); + assertFalse(mService.isUidForegroundOnRestrictBackgroundUL(UID_B)); + } + + @SuppressWarnings("GuardedBy") + @Test + public void testUidForeground_withPendingState() throws Exception { + long procStateSeq = 0; + callAndWaitOnUidStateChanged(UID_A, PROCESS_STATE_SERVICE, + procStateSeq++, PROCESS_CAPABILITY_NONE); + callAndWaitOnUidStateChanged(UID_B, PROCESS_STATE_SERVICE, + procStateSeq++, PROCESS_CAPABILITY_NONE); + assertFalse(mService.isUidForegroundOnRestrictPowerUL(UID_A)); + assertFalse(mService.isUidForegroundOnRestrictBackgroundUL(UID_A)); + assertFalse(mService.isUidForegroundOnRestrictPowerUL(UID_B)); + assertFalse(mService.isUidForegroundOnRestrictBackgroundUL(UID_B)); + + try (SyncBarrier b = new SyncBarrier(mService.mUidEventHandler)) { + // Verify that a callback with an old procStateSeq is ignored. + callOnUidStatechanged(UID_A, PROCESS_STATE_TOP, 0, + PROCESS_CAPABILITY_POWER_RESTRICTED_NETWORK); + assertFalse(mService.isUidForegroundOnRestrictPowerUL(UID_A)); + assertFalse(mService.isUidForegroundOnRestrictBackgroundUL(UID_A)); + assertFalse(mService.isUidForegroundOnRestrictPowerUL(UID_B)); + assertFalse(mService.isUidForegroundOnRestrictBackgroundUL(UID_B)); + + callOnUidStatechanged(UID_A, PROCESS_STATE_SERVICE, procStateSeq++, + PROCESS_CAPABILITY_POWER_RESTRICTED_NETWORK); + assertTrue(mService.isUidForegroundOnRestrictPowerUL(UID_A)); + assertFalse(mService.isUidForegroundOnRestrictBackgroundUL(UID_A)); + assertFalse(mService.isUidForegroundOnRestrictPowerUL(UID_B)); + assertFalse(mService.isUidForegroundOnRestrictBackgroundUL(UID_B)); + + callOnUidStatechanged(UID_A, PROCESS_STATE_IMPORTANT_FOREGROUND, procStateSeq++, + PROCESS_CAPABILITY_USER_RESTRICTED_NETWORK); + assertFalse(mService.isUidForegroundOnRestrictPowerUL(UID_A)); + assertTrue(mService.isUidForegroundOnRestrictBackgroundUL(UID_A)); + assertFalse(mService.isUidForegroundOnRestrictPowerUL(UID_B)); + assertFalse(mService.isUidForegroundOnRestrictBackgroundUL(UID_B)); + + callOnUidStatechanged(UID_A, PROCESS_STATE_TOP, procStateSeq++, + PROCESS_CAPABILITY_NONE); + assertTrue(mService.isUidForegroundOnRestrictPowerUL(UID_A)); + assertTrue(mService.isUidForegroundOnRestrictBackgroundUL(UID_A)); + assertFalse(mService.isUidForegroundOnRestrictPowerUL(UID_B)); + assertFalse(mService.isUidForegroundOnRestrictBackgroundUL(UID_B)); + } + waitForUidEventHandlerIdle(); + + assertTrue(mService.isUidForegroundOnRestrictPowerUL(UID_A)); + assertTrue(mService.isUidForegroundOnRestrictBackgroundUL(UID_A)); + assertFalse(mService.isUidForegroundOnRestrictPowerUL(UID_B)); + assertFalse(mService.isUidForegroundOnRestrictBackgroundUL(UID_B)); + + try (SyncBarrier b = new SyncBarrier(mService.mUidEventHandler)) { + callOnUidStatechanged(UID_A, PROCESS_STATE_SERVICE, procStateSeq++, + PROCESS_CAPABILITY_NONE); + assertTrue(mService.isUidForegroundOnRestrictPowerUL(UID_A)); + assertTrue(mService.isUidForegroundOnRestrictBackgroundUL(UID_A)); + assertFalse(mService.isUidForegroundOnRestrictPowerUL(UID_B)); + assertFalse(mService.isUidForegroundOnRestrictBackgroundUL(UID_B)); + } + waitForUidEventHandlerIdle(); } @Test @@ -1417,14 +1531,28 @@ public class NetworkPolicyManagerServiceTest { @Test public void testOnUidStateChanged_notifyAMS() throws Exception { final long procStateSeq = 222; - callOnUidStateChanged(UID_A, ActivityManager.PROCESS_STATE_SERVICE, procStateSeq); + callAndWaitOnUidStateChanged(UID_A, PROCESS_STATE_SERVICE, procStateSeq); verify(mActivityManagerInternal).notifyNetworkPolicyRulesUpdated(UID_A, procStateSeq); } - private void callOnUidStateChanged(int uid, int procState, long procStateSeq) + private void callAndWaitOnUidStateChanged(int uid, int procState, long procStateSeq) throws Exception { - mUidObserver.onUidStateChanged(uid, procState, procStateSeq, - ActivityManager.PROCESS_CAPABILITY_NONE); + callAndWaitOnUidStateChanged(uid, procState, procStateSeq, + PROCESS_CAPABILITY_NONE); + } + + private void callAndWaitOnUidStateChanged(int uid, int procState, long procStateSeq, + int capability) throws Exception { + callOnUidStatechanged(uid, procState, procStateSeq, capability); + waitForUidEventHandlerIdle(); + } + + private void callOnUidStatechanged(int uid, int procState, long procStateSeq, int capability) + throws Exception { + mUidObserver.onUidStateChanged(uid, procState, procStateSeq, capability); + } + + private void waitForUidEventHandlerIdle() throws Exception { final CountDownLatch latch = new CountDownLatch(1); mService.mUidEventHandler.post(() -> { latch.countDown(); @@ -1927,9 +2055,9 @@ public class NetworkPolicyManagerServiceTest { @Test public void testLowPowerStandbyAllowlist() throws Exception { - callOnUidStateChanged(UID_A, ActivityManager.PROCESS_STATE_TOP, 0); - callOnUidStateChanged(UID_B, ActivityManager.PROCESS_STATE_FOREGROUND_SERVICE, 0); - callOnUidStateChanged(UID_C, ActivityManager.PROCESS_STATE_FOREGROUND_SERVICE, 0); + callAndWaitOnUidStateChanged(UID_A, PROCESS_STATE_TOP, 0); + callAndWaitOnUidStateChanged(UID_B, ActivityManager.PROCESS_STATE_FOREGROUND_SERVICE, 0); + callAndWaitOnUidStateChanged(UID_C, ActivityManager.PROCESS_STATE_FOREGROUND_SERVICE, 0); expectHasInternetPermission(UID_A, true); expectHasInternetPermission(UID_B, true); expectHasInternetPermission(UID_C, true);