From f4923ea0b25edb6e6ca6804ed4c66f57a8ec29c2 Mon Sep 17 00:00:00 2001 From: Sudheer Shanka Date: Mon, 27 Mar 2017 15:36:35 -0700 Subject: [PATCH] Don't increment procStateSeq if uid doesn't have internet permission. Bug: 27803922 Test: runtest -c com.android.server.am.ActivityManagerServiceTest frameworks-services and manual Change-Id: I02bfd86837813cc79f37301d91489485c6a1d056 --- .../server/am/ActivityManagerService.java | 44 ++++++++++--- .../java/com/android/server/am/UidRecord.java | 26 ++++++-- .../am/ActivityManagerInternalTest.java | 4 +- .../server/am/ActivityManagerServiceTest.java | 65 ++++++++++++++----- 4 files changed, 104 insertions(+), 35 deletions(-) diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index 8cb0eee07b3db..1d60906ec85e5 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -6552,6 +6552,7 @@ public class ActivityManagerService extends IActivityManager.Stub if (Arrays.binarySearch(mDeviceIdleTempWhitelist, UserHandle.getAppId(proc.uid)) >= 0) { uidRec.setWhitelist = uidRec.curWhitelist = true; } + uidRec.updateHasInternetPermission(); mActiveUids.put(proc.uid, uidRec); noteUidProcessState(uidRec.uid, uidRec.curProcState); enqueueUidChangeLocked(uidRec, -1, UidRecord.CHANGE_ACTIVE); @@ -18865,9 +18866,7 @@ public class ActivityManagerService extends IActivityManager.Stub } switch (action) { case Intent.ACTION_UID_REMOVED: - final Bundle intentExtras = intent.getExtras(); - final int uid = intentExtras != null - ? intentExtras.getInt(Intent.EXTRA_UID) : -1; + final int uid = getUidFromIntent(intent); if (uid >= 0) { mBatteryStatsService.removeUid(uid); mAppOpsService.uidRemoved(uid); @@ -19063,6 +19062,18 @@ public class ActivityManagerService extends IActivityManager.Stub mHandler.sendEmptyMessage(HANDLE_TRUST_STORAGE_UPDATE_MSG); break; } + + if (Intent.ACTION_PACKAGE_ADDED.equals(action) || + Intent.ACTION_PACKAGE_REMOVED.equals(action) || + Intent.ACTION_PACKAGE_REPLACED.equals(action)) { + final int uid = getUidFromIntent(intent); + if (uid != -1) { + final UidRecord uidRec = mActiveUids.get(uid); + if (uidRec != null) { + uidRec.updateHasInternetPermission(); + } + } + } } // Add to the sticky list if requested. @@ -19329,6 +19340,18 @@ public class ActivityManagerService extends IActivityManager.Stub return ActivityManager.BROADCAST_SUCCESS; } + /** + * @return uid from the extra field {@link Intent#EXTRA_UID} if present, Otherwise -1 + */ + private int getUidFromIntent(Intent intent) { + if (intent == null) { + return -1; + } + final Bundle intentExtras = intent.getExtras(); + return intent.hasExtra(Intent.EXTRA_UID) + ? intentExtras.getInt(Intent.EXTRA_UID) : -1; + } + final void rotateBroadcastStatsIfNeededLocked() { final long now = SystemClock.elapsedRealtime(); if (mCurBroadcastStats == null || @@ -22547,6 +22570,9 @@ public class ActivityManagerService extends IActivityManager.Stub if (!mInjector.isNetworkRestrictedForUid(uidRec.uid)) { continue; } + if (!UserHandle.isApp(uidRec.uid) || !uidRec.hasInternetPermission) { + continue; + } // If process state is not changed, then there's nothing to do. if (uidRec.setProcState == uidRec.curProcState) { continue; @@ -22557,7 +22583,7 @@ public class ActivityManagerService extends IActivityManager.Stub if (blockState == NETWORK_STATE_NO_CHANGE) { continue; } - synchronized (uidRec.lock) { + synchronized (uidRec.networkStateLock) { uidRec.curProcStateSeq = ++mProcStateSeqCounter; if (blockState == NETWORK_STATE_BLOCK) { if (blockingUids == null) { @@ -22570,7 +22596,7 @@ public class ActivityManagerService extends IActivityManager.Stub + " threads for uid: " + uidRec); } if (uidRec.waitingForNetwork) { - uidRec.lock.notifyAll(); + uidRec.networkStateLock.notifyAll(); } } } @@ -23500,7 +23526,7 @@ public class ActivityManagerService extends IActivityManager.Stub return; } } - synchronized (record.lock) { + synchronized (record.networkStateLock) { if (record.lastNetworkUpdatedProcStateSeq >= procStateSeq) { if (DEBUG_NETWORK) { Slog.d(TAG_NETWORK, "procStateSeq: " + procStateSeq + " has already" @@ -23522,7 +23548,7 @@ public class ActivityManagerService extends IActivityManager.Stub Slog.d(TAG_NETWORK, "Notifying all blocking threads for uid: " + uid + ", procStateSeq: " + procStateSeq); } - record.lock.notifyAll(); + record.networkStateLock.notifyAll(); } } } @@ -23547,7 +23573,7 @@ public class ActivityManagerService extends IActivityManager.Stub return; } } - synchronized (record.lock) { + synchronized (record.networkStateLock) { if (record.lastDispatchedProcStateSeq < procStateSeq) { if (DEBUG_NETWORK) { Slog.d(TAG_NETWORK, "Uid state change for seq no. " + procStateSeq + " is not " @@ -23581,7 +23607,7 @@ public class ActivityManagerService extends IActivityManager.Stub } final long startTime = SystemClock.uptimeMillis(); record.waitingForNetwork = true; - record.lock.wait(mWaitForNetworkTimeoutMs); + record.networkStateLock.wait(mWaitForNetworkTimeoutMs); record.waitingForNetwork = false; final long totalTime = SystemClock.uptimeMillis() - startTime; if (totalTime >= mWaitForNetworkTimeoutMs) { diff --git a/services/core/java/com/android/server/am/UidRecord.java b/services/core/java/com/android/server/am/UidRecord.java index 67b80f61775ee..c0fb77f086472 100644 --- a/services/core/java/com/android/server/am/UidRecord.java +++ b/services/core/java/com/android/server/am/UidRecord.java @@ -16,7 +16,9 @@ package com.android.server.am; +import android.Manifest; import android.app.ActivityManager; +import android.content.pm.PackageManager; import android.os.SystemClock; import android.os.UserHandle; import android.util.TimeUtils; @@ -43,30 +45,37 @@ public final class UidRecord { * {@link ActivityManagerService#mProcStateSeqCounter} * when {@link #curProcState} changes from background to foreground or vice versa. */ - @GuardedBy("lock") + @GuardedBy("networkStateUpdate") long curProcStateSeq; /** * Last seq number for which NetworkPolicyManagerService notified ActivityManagerService that * network policies rules were updated. */ - @GuardedBy("lock") + @GuardedBy("networkStateUpdate") long lastNetworkUpdatedProcStateSeq; /** * Last seq number for which AcitivityManagerService dispatched uid state change to * NetworkPolicyManagerService. */ - @GuardedBy("lock") + @GuardedBy("networkStateUpdate") long lastDispatchedProcStateSeq; /** * Indicates if any thread is waiting for network rules to get updated for {@link #uid}. */ - @GuardedBy("lock") - boolean waitingForNetwork; + volatile boolean waitingForNetwork; - final Object lock = new Object(); + /** + * Indicates whether this uid has internet permission or not. + */ + volatile boolean hasInternetPermission; + + /** + * This object is used for waiting for the network state to get updated. + */ + final Object networkStateLock = new Object(); static final int CHANGE_PROCSTATE = 0; static final int CHANGE_GONE = 1; @@ -95,6 +104,11 @@ public final class UidRecord { curProcState = ActivityManager.PROCESS_STATE_CACHED_EMPTY; } + public void updateHasInternetPermission() { + hasInternetPermission = ActivityManager.checkUidPermission(Manifest.permission.INTERNET, + uid) == PackageManager.PERMISSION_GRANTED; + } + /** * If the change being dispatched is neither CHANGE_GONE nor CHANGE_GONE_IDLE (not interested in * these changes), then update the {@link #lastDispatchedProcStateSeq} with diff --git a/services/tests/servicestests/src/com/android/server/am/ActivityManagerInternalTest.java b/services/tests/servicestests/src/com/android/server/am/ActivityManagerInternalTest.java index e7c91c00694a7..bce87dc58819b 100644 --- a/services/tests/servicestests/src/com/android/server/am/ActivityManagerInternalTest.java +++ b/services/tests/servicestests/src/com/android/server/am/ActivityManagerInternalTest.java @@ -105,9 +105,9 @@ public class ActivityManagerInternalTest { final UidRecord record2 = addActiveUidRecord(TEST_UID2, curProcStateSeq, lastNetworkUpdatedProcStateSeq); - final CustomThread thread1 = new CustomThread(record1.lock); + final CustomThread thread1 = new CustomThread(record1.networkStateLock); thread1.startAndWait("Unexpected state for " + record1); - final CustomThread thread2 = new CustomThread(record2.lock); + final CustomThread thread2 = new CustomThread(record2.networkStateLock); thread2.startAndWait("Unexpected state for " + record2); mAmi.notifyNetworkPolicyRulesUpdated(TEST_UID1, expectedProcStateSeq); diff --git a/services/tests/servicestests/src/com/android/server/am/ActivityManagerServiceTest.java b/services/tests/servicestests/src/com/android/server/am/ActivityManagerServiceTest.java index 1e038dfaf1af2..092c60be254e5 100644 --- a/services/tests/servicestests/src/com/android/server/am/ActivityManagerServiceTest.java +++ b/services/tests/servicestests/src/com/android/server/am/ActivityManagerServiceTest.java @@ -105,7 +105,7 @@ import java.util.function.Function; public class ActivityManagerServiceTest { private static final String TAG = ActivityManagerServiceTest.class.getSimpleName(); - private static final int TEST_UID = 111; + private static final int TEST_UID = 11111; private static final long TEST_PROC_STATE_SEQ1 = 555; private static final long TEST_PROC_STATE_SEQ2 = 556; @@ -121,6 +121,7 @@ public class ActivityManagerServiceTest { @Mock private Context mContext; @Mock private AppOpsService mAppOpsService; @Mock private PackageManager mPackageManager; + @Mock private BatteryStatsImpl mBatteryStatsImpl; private TestInjector mInjector; private ActivityManagerService mAms; @@ -149,20 +150,9 @@ public class ActivityManagerServiceTest { @MediumTest @Test public void incrementProcStateSeqAndNotifyAppsLocked() throws Exception { - final UidRecord uidRec = new UidRecord(TEST_UID); - uidRec.waitingForNetwork = true; - mAms.mActiveUids.put(TEST_UID, uidRec); - final BatteryStatsImpl batteryStats = Mockito.mock(BatteryStatsImpl.class); - final ProcessRecord appRec = new ProcessRecord(batteryStats, - new ApplicationInfo(), TAG, TEST_UID); - appRec.thread = Mockito.mock(IApplicationThread.class); - mAms.mLruProcesses.add(appRec); - - final ProcessRecord appRec2 = new ProcessRecord(batteryStats, - new ApplicationInfo(), TAG, TEST_UID + 1); - appRec2.thread = Mockito.mock(IApplicationThread.class); - mAms.mLruProcesses.add(appRec2); + final UidRecord uidRec = addUidRecord(TEST_UID); + addUidRecord(TEST_UID + 1); // Uid state is not moving from background to foreground or vice versa. verifySeqCounterAndInteractions(uidRec, @@ -235,12 +225,51 @@ public class ActivityManagerServiceTest { 44, // exptectedCurProcStateSeq -1, // expectedBlockState, -1 to verify there are no interactions with main thread. false); // expectNotify + + // Verify when the uid doesn't have internet permission, then procStateSeq is not + // incremented. + uidRec.hasInternetPermission = false; + mAms.mWaitForNetworkTimeoutMs = 111; + mInjector.setNetworkRestrictedForUid(true); + verifySeqCounterAndInteractions(uidRec, + PROCESS_STATE_CACHED_ACTIVITY, // prevState + PROCESS_STATE_FOREGROUND_SERVICE, // curState + 44, // expectedGlobalCounter + 44, // exptectedCurProcStateSeq + -1, // expectedBlockState, -1 to verify there are no interactions with main thread. + false); // expectNotify + + // Verify procStateSeq is not incremented when the uid is not an application, regardless + // of the process state. + final int notAppUid = 111; + final UidRecord uidRec2 = addUidRecord(notAppUid); + verifySeqCounterAndInteractions(uidRec2, + PROCESS_STATE_CACHED_EMPTY, // prevState + PROCESS_STATE_TOP, // curState + 44, // expectedGlobalCounter + 0, // exptectedCurProcStateSeq + -1, // expectedBlockState, -1 to verify there are no interactions with main thread. + false); // expectNotify + } + + private UidRecord addUidRecord(int uid) { + final UidRecord uidRec = new UidRecord(uid); + uidRec.waitingForNetwork = true; + uidRec.hasInternetPermission = true; + mAms.mActiveUids.put(uid, uidRec); + + final ProcessRecord appRec = new ProcessRecord(mBatteryStatsImpl, + new ApplicationInfo(), TAG, uid); + appRec.thread = Mockito.mock(IApplicationThread.class); + mAms.mLruProcesses.add(appRec); + + return uidRec; } private void verifySeqCounterAndInteractions(UidRecord uidRec, int prevState, int curState, int expectedGlobalCounter, int expectedCurProcStateSeq, int expectedBlockState, boolean expectNotify) throws Exception { - CustomThread thread = new CustomThread(uidRec.lock); + CustomThread thread = new CustomThread(uidRec.networkStateLock); thread.startAndWait("Unexpected state for " + uidRec); uidRec.setProcState = prevState; @@ -720,7 +749,7 @@ public class ActivityManagerServiceTest { record.lastNetworkUpdatedProcStateSeq = lastNetworkUpdatedProcStateSeq; mAms.mActiveUids.put(Process.myUid(), record); - CustomThread thread = new CustomThread(record.lock, new Runnable() { + CustomThread thread = new CustomThread(record.networkStateLock, new Runnable() { @Override public void run() { mAms.waitForNetworkStateUpdate(procStateSeqToWait); @@ -730,8 +759,8 @@ public class ActivityManagerServiceTest { if (expectWait) { thread.startAndWait(errMsg, true); thread.assertTimedWaiting(errMsg); - synchronized (record.lock) { - record.lock.notifyAll(); + synchronized (record.networkStateLock) { + record.networkStateLock.notifyAll(); } thread.assertTerminated(errMsg); assertTrue(thread.mNotified);