diff options
author | Michael Plass <mplass@google.com> | 2020-04-01 20:21:21 -0700 |
---|---|---|
committer | Michael Plass <mplass@google.com> | 2020-04-06 10:18:43 -0700 |
commit | 47df04e7471eadc2871791385916685cf40a081b (patch) | |
tree | df886a13910f0fbfbbbf569172ff7904017260b4 | |
parent | bb96a3d98cd5411a7d2d81acf409711e1984aa22 (diff) |
ConnectedNetworkScorer: Do not send invalid sessionId
Generate a sessionId for the external scorer that is not the same as
the android.net.Network netId, to avoid any assumptions that they are
the same.
Assure that the external scorer is never sent an invalid sessionId,
and that a previous session is stopped before a new one is started.
Bug: 122133502
Test: atest android.net.wifi.cts.ConnectedNetworkScorerTest
Test: atest com.android.server.wifi.WifiScoreReportTest
Change-Id: I008d26490df10e3037e1c99025f72c21e3e335d9
-rw-r--r-- | service/java/com/android/server/wifi/WifiScoreReport.java | 170 | ||||
-rw-r--r-- | tests/wifitests/Android.bp | 6 | ||||
-rw-r--r-- | tests/wifitests/src/com/android/server/wifi/WifiScoreReportTest.java | 74 |
3 files changed, 161 insertions, 89 deletions
diff --git a/service/java/com/android/server/wifi/WifiScoreReport.java b/service/java/com/android/server/wifi/WifiScoreReport.java index ad3c1ca27..5561a9104 100644 --- a/service/java/com/android/server/wifi/WifiScoreReport.java +++ b/service/java/com/android/server/wifi/WifiScoreReport.java @@ -56,11 +56,10 @@ public class WifiScoreReport { // Cache of the last score private int mScore = ConnectedScore.WIFI_MAX_SCORE; - private int mSessionId = INVALID_SESSION_ID; private final ScoringParams mScoringParams; private final Clock mClock; - private int mSessionNumber = 0; + private int mSessionNumber = 0; // not to be confused with sessionid, this just counts resets private String mInterfaceName; private final BssidBlocklistMonitor mBssidBlocklistMonitor; private long mLastScoreBreachLowTimeMillis = -1; @@ -75,16 +74,19 @@ public class WifiScoreReport { WifiThreadRunner mWifiThreadRunner; /** - * Callback proxy. See {@link WifiManager#ScoreUpdateObserver}. + * Callback proxy. See {@link android.net.wifi.WifiManager.ScoreUpdateObserver}. */ private class ScoreUpdateObserverProxy extends IScoreUpdateObserver.Stub { @Override public void notifyScoreUpdate(int sessionId, int score) { mWifiThreadRunner.post(() -> { - if (mWifiConnectedNetworkScorerHolder == null) { - return; - } - if (!validateSessionId(sessionId)) { + if (mWifiConnectedNetworkScorerHolder == null + || sessionId == INVALID_SESSION_ID + || sessionId != getCurrentSessionId()) { + Log.w(TAG, "Ignoring stale/invalid external score" + + " sessionId=" + sessionId + + " currentSessionId=" + getCurrentSessionId() + + " score=" + score); return; } if (mNetworkAgent != null) { @@ -108,21 +110,20 @@ public class WifiScoreReport { @Override public void triggerUpdateOfWifiUsabilityStats(int sessionId) { mWifiThreadRunner.post(() -> { - if (mWifiConnectedNetworkScorerHolder == null) { - return; - } - if (!validateSessionId(sessionId)) { + if (mWifiConnectedNetworkScorerHolder == null + || sessionId == INVALID_SESSION_ID + || sessionId != getCurrentSessionId() + || mInterfaceName == null) { + Log.w(TAG, "Ignoring triggerUpdateOfWifiUsabilityStats" + + " sessionId=" + sessionId + + " currentSessionId=" + getCurrentSessionId() + + " interfaceName=" + mInterfaceName); return; } - if (mInterfaceName == null) { - Log.e(TAG, "getWifiLinkLayerStats called without an interface"); - return; - } - WifiLinkLayerStats stats = mWifiNative.getWifiLinkLayerStats(mInterfaceName); // update mWifiInfo - // TODO: b/149583741 Better coordinate this class and ClientModeImpl to remove + // TODO(b/153075963): Better coordinate this class and ClientModeImpl to remove // redundant codes below and in ClientModeImpl#fetchRssiLinkSpeedAndFrequencyNative. WifiNl80211Manager.SignalPollResult pollResult = mWifiNative.signalPoll(mInterfaceName); @@ -159,34 +160,19 @@ public class WifiScoreReport { } } + // TODO(b/153075963): This should not be plumbed through WifiMetrics mWifiMetrics.updateWifiUsabilityStatsEntries(mWifiInfo, stats); }); } - - private boolean validateSessionId(int sessionId) { - if (sessionId == INVALID_SESSION_ID) { - Log.e(TAG, "Called with invalid ID"); - return false; - } - if (mSessionId == INVALID_SESSION_ID) { - if (mVerboseLoggingEnabled) Log.d(TAG, "Called with score after session reset"); - return false; - } - if (sessionId != mSessionId) { - // This indicates a more serious timing issue - Log.w(TAG, "Called with old session ID"); - return false; - } - return true; - } } /** - * Container for storing info about external scorer and tracks its death. + * Container for storing info about external scorer and tracking its death. */ private final class WifiConnectedNetworkScorerHolder implements IBinder.DeathRecipient { private final IBinder mBinder; private final IWifiConnectedNetworkScorer mScorer; + private int mSessionId = INVALID_SESSION_ID; WifiConnectedNetworkScorerHolder(IBinder binder, IWifiConnectedNetworkScorer scorer) { mBinder = binder; @@ -209,10 +195,9 @@ public class WifiScoreReport { /** * App hosting the binder has died. */ + @Override public void binderDied() { - mWifiThreadRunner.post(() -> { - revertAospConnectedScorer(); - }); + mWifiThreadRunner.post(() -> revertToDefaultConnectedScorer()); } /** @@ -223,10 +208,39 @@ public class WifiScoreReport { } /** - * Get Wi-Fi connected network scorer + * Starts a new scoring session. */ - public IWifiConnectedNetworkScorer getScorer() { - return mScorer; + public void startSession(int sessionId) { + if (sessionId == INVALID_SESSION_ID) { + throw new IllegalArgumentException(); + } + if (mSessionId != INVALID_SESSION_ID) { + // This is not expected to happen, log if it does + Log.e(TAG, "Stopping session " + mSessionId + " before starting " + sessionId); + stopSession(); + } + // Bail now if the scorer has gone away + if (this != mWifiConnectedNetworkScorerHolder) { + return; + } + mSessionId = sessionId; + try { + mScorer.onStart(sessionId); + } catch (RemoteException e) { + Log.e(TAG, "Unable to start Wifi connected network scorer " + this, e); + revertToDefaultConnectedScorer(); + } + } + public void stopSession() { + final int sessionId = mSessionId; + if (sessionId == INVALID_SESSION_ID) return; + mSessionId = INVALID_SESSION_ID; + try { + mScorer.onStop(sessionId); + } catch (RemoteException e) { + Log.e(TAG, "Unable to stop Wifi connected network scorer " + this, e); + revertToDefaultConnectedScorer(); + } } } @@ -353,7 +367,7 @@ public class WifiScoreReport { mScore = score; } - private void updateWifiMetrics(long now, int s2, int score) { + private int getCurrentNetId() { int netId = 0; if (mNetworkAgent != null) { final Network network = mNetworkAgent.getNetwork(); @@ -361,6 +375,28 @@ public class WifiScoreReport { netId = network.getNetId(); } } + return netId; + } + + private int getCurrentSessionId() { + return sessionIdFromNetId(getCurrentNetId()); + } + + /** + * Encodes a network id into a scoring session id. + * + * We use a different numeric value for session id and the network id + * to make it clear that these are not the same thing. However, for + * easier debugging, the network id can be recovered by dropping the + * last decimal digit (at least until they get very, very, large). + */ + public static int sessionIdFromNetId(final int netId) { + if (netId <= 0) return INVALID_SESSION_ID; + return (int) (((long) netId * 10 + (8 - (netId % 9))) % Integer.MAX_VALUE + 1); + } + + private void updateWifiMetrics(long now, int s2, int score) { + int netId = getCurrentNetId(); mAggressiveConnectedScore.updateUsingWifiInfo(mWifiInfo, now); int s1 = mAggressiveConnectedScore.generateScore(); @@ -516,6 +552,7 @@ public class WifiScoreReport { */ public boolean setWifiConnectedNetworkScorer(IBinder binder, IWifiConnectedNetworkScorer scorer) { + if (binder == null || scorer == null) return false; // Enforce that only a single scorer can be set successfully. if (mWifiConnectedNetworkScorerHolder != null) { Log.e(TAG, "Failed to set current scorer because one scorer is already set"); @@ -532,15 +569,17 @@ public class WifiScoreReport { scorer.onSetScoreUpdateObserver(mScoreUpdateObserver); } catch (RemoteException e) { Log.e(TAG, "Unable to set score update observer " + scorer, e); - revertAospConnectedScorer(); + revertToDefaultConnectedScorer(); return false; } - if (mSessionId != INVALID_SESSION_ID) { - startConnectedNetworkScorer(mSessionId); - } // Disable AOSP scorer mVelocityBasedConnectedScore = null; mWifiMetrics.setIsExternalWifiScorerOn(true); + // If there is already a connection, start a new session + final int netId = getCurrentNetId(); + if (netId > 0) { + startConnectedNetworkScorer(netId); + } return true; } @@ -552,26 +591,25 @@ public class WifiScoreReport { return; } mWifiConnectedNetworkScorerHolder.reset(); - revertAospConnectedScorer(); + revertToDefaultConnectedScorer(); } /** * Start the registered Wi-Fi connected network scorer. - * @param sessionId + * @param netId identifies the current android.net.Network */ - public void startConnectedNetworkScorer(int sessionId) { - mSessionId = sessionId; - if (mWifiConnectedNetworkScorerHolder == null) { + public void startConnectedNetworkScorer(int netId) { + final int sessionId = getCurrentSessionId(); + if (mWifiConnectedNetworkScorerHolder == null + || netId != getCurrentNetId() + || sessionId == INVALID_SESSION_ID) { + Log.w(TAG, "Cannot start external scoring" + + " netId=" + netId + + " currentNetId=" + getCurrentNetId() + + " sessionId=" + sessionId); return; } - try { - IWifiConnectedNetworkScorer scorer = mWifiConnectedNetworkScorerHolder.getScorer(); - scorer.onStart(sessionId); - } catch (RemoteException e) { - Log.e(TAG, "Unable to start Wifi connected network scorer " - + mWifiConnectedNetworkScorerHolder, e); - revertAospConnectedScorer(); - } + mWifiConnectedNetworkScorerHolder.startSession(sessionId); mLastScoreBreachLowTimeMillis = INVALID_WALL_CLOCK_MILLIS; } @@ -582,15 +620,8 @@ public class WifiScoreReport { if (mWifiConnectedNetworkScorerHolder == null) { return; } - try { - IWifiConnectedNetworkScorer scorer = mWifiConnectedNetworkScorerHolder.getScorer(); - scorer.onStop(mSessionId); - } catch (RemoteException e) { - Log.e(TAG, "Unable to stop Wifi connected network scorer " - + mWifiConnectedNetworkScorerHolder, e); - } + mWifiConnectedNetworkScorerHolder.stopSession(); - mSessionId = INVALID_SESSION_ID; long millis = mClock.getWallClockMillis(); // Blocklist the current BSS if ((mLastScoreBreachLowTimeMillis != INVALID_WALL_CLOCK_MILLIS) @@ -605,7 +636,6 @@ public class WifiScoreReport { /** * Set NetworkAgent - * @param agent */ public void setNetworkAgent(NetworkAgent agent) { mNetworkAgent = agent; @@ -626,8 +656,8 @@ public class WifiScoreReport { mInterfaceName = ifaceName; } - private void revertAospConnectedScorer() { - // Create AOSP scorer and null external Wifi connected network scorer + private void revertToDefaultConnectedScorer() { + Log.d(TAG, "Using VelocityBasedConnectedScore"); mVelocityBasedConnectedScore = new VelocityBasedConnectedScore(mScoringParams, mClock); mWifiConnectedNetworkScorerHolder = null; mWifiMetrics.setIsExternalWifiScorerOn(false); diff --git a/tests/wifitests/Android.bp b/tests/wifitests/Android.bp index 7fd2a5f75..2f3cbad1b 100644 --- a/tests/wifitests/Android.bp +++ b/tests/wifitests/Android.bp @@ -900,6 +900,9 @@ android_test { "com.android.server.wifi.util.KeyValueListParser", "com.android.server.wifi.util.KeyValueListParser$*", "com.android.server.wifi.util.KeyValueListParser.**", + "com.android.server.wifi.util.LruConnectionTracker", + "com.android.server.wifi.util.LruConnectionTracker$*", + "com.android.server.wifi.util.LruConnectionTracker.**", "com.android.server.wifi.util.LruList", "com.android.server.wifi.util.LruList$*", "com.android.server.wifi.util.LruList.**", @@ -927,6 +930,9 @@ android_test { "com.android.server.wifi.util.ScanResultUtil", "com.android.server.wifi.util.ScanResultUtil$*", "com.android.server.wifi.util.ScanResultUtil.**", + "com.android.server.wifi.util.SettingsMigrationDataHolder", + "com.android.server.wifi.util.SettingsMigrationDataHolder$*", + "com.android.server.wifi.util.SettingsMigrationDataHolder.**", "com.android.server.wifi.util.StringUtil", "com.android.server.wifi.util.StringUtil$*", "com.android.server.wifi.util.StringUtil.**", diff --git a/tests/wifitests/src/com/android/server/wifi/WifiScoreReportTest.java b/tests/wifitests/src/com/android/server/wifi/WifiScoreReportTest.java index 41775ec25..5fe8eaac7 100644 --- a/tests/wifitests/src/com/android/server/wifi/WifiScoreReportTest.java +++ b/tests/wifitests/src/com/android/server/wifi/WifiScoreReportTest.java @@ -60,7 +60,6 @@ import com.android.wifi.resources.R; import org.junit.After; import org.junit.Before; import org.junit.Test; -import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.MockitoAnnotations; @@ -82,7 +81,8 @@ public class WifiScoreReportTest extends WifiBaseTest { } } - private static final int TEST_SESSION_ID = 8603706; + private static final int TEST_NETWORK_ID = 860370; + private static final int TEST_SESSION_ID = 8603703; // last digit is a check digit FakeClock mClock; WifiConfiguration mWifiConfiguration; @@ -100,6 +100,7 @@ public class WifiScoreReportTest extends WifiBaseTest { @Mock IWifiConnectedNetworkScorer mWifiConnectedNetworkScorer; @Mock WifiNative mWifiNative; @Mock BssidBlocklistMonitor mBssidBlocklistMonitor; + @Mock Network mNetwork; private TestLooper mLooper; public class WifiConnectedNetworkScorerImpl extends IWifiConnectedNetworkScorer.Stub { @@ -189,10 +190,11 @@ public class WifiScoreReportTest extends WifiBaseTest { final ConnectivityManager cm = mock(ConnectivityManager.class); when(mContext.getSystemService(Context.CONNECTIVITY_SERVICE)).thenReturn(cm); when(cm.registerNetworkAgent(any(), any(), any(), any(), anyInt(), any(), anyInt())) - .thenReturn(mock(Network.class)); + .thenReturn(mNetwork); + when(mNetwork.getNetId()).thenReturn(0); mNetworkAgent = spy(new TestNetworkAgent(mContext)); mClock = new FakeClock(); - mScoringParams = new ScoringParams(mContext); + mScoringParams = new ScoringParams(); mWifiThreadRunner = new WifiThreadRunner(new Handler(mLooper.getLooper())); mWifiScoreReport = new WifiScoreReport(mScoringParams, mClock, mWifiMetrics, mWifiInfo, mWifiNative, mBssidBlocklistMonitor, mWifiThreadRunner); @@ -412,7 +414,7 @@ public class WifiScoreReportTest extends WifiBaseTest { asks++; } } - assertTrue(oops + " asks:" + asks, asks > 5 && asks < 12); + assertTrue(oops + " asks:" + asks, asks > 5 && asks < 15); } @@ -609,16 +611,45 @@ public class WifiScoreReportTest extends WifiBaseTest { } /** + * Verify netId to sessionId conversion. + */ + @Test + public void testSessionId() throws Exception { + assertEquals(-1, WifiScoreReport.sessionIdFromNetId(Integer.MIN_VALUE)); + assertEquals(-1, WifiScoreReport.sessionIdFromNetId(-42)); + assertEquals(-1, WifiScoreReport.sessionIdFromNetId(-1)); + assertEquals(-1, WifiScoreReport.sessionIdFromNetId(0)); + assertEquals(18, WifiScoreReport.sessionIdFromNetId(1)); + assertEquals(3339, WifiScoreReport.sessionIdFromNetId(333)); + assertEquals(TEST_SESSION_ID, WifiScoreReport.sessionIdFromNetId(TEST_NETWORK_ID)); + int dangerOfOverflow = Integer.MAX_VALUE / 10; + assertEquals(214748364, dangerOfOverflow); + assertEquals(2147483646, WifiScoreReport.sessionIdFromNetId(dangerOfOverflow)); + assertEquals(8, WifiScoreReport.sessionIdFromNetId(dangerOfOverflow + 1)); + assertEquals(8, WifiScoreReport.sessionIdFromNetId(Integer.MAX_VALUE)); + } + + /** * Verify that client gets session ID when onStart() method is called. */ @Test public void testClientGetSessionIdOnStart() throws Exception { - ArgumentCaptor<Integer> startId = ArgumentCaptor.forClass(Integer.class); // Register Client for verification. mWifiScoreReport.setWifiConnectedNetworkScorer(mAppBinder, mWifiConnectedNetworkScorer); - mWifiScoreReport.startConnectedNetworkScorer(TEST_SESSION_ID); - verify(mWifiConnectedNetworkScorer).onStart(startId.capture()); - assertEquals((int) startId.getValue(), TEST_SESSION_ID); + when(mNetwork.getNetId()).thenReturn(TEST_NETWORK_ID); + mWifiScoreReport.startConnectedNetworkScorer(TEST_NETWORK_ID); + verify(mWifiConnectedNetworkScorer).onStart(TEST_SESSION_ID); + } + + /** + * Verify that onStart is called if there is already an active network when registered. + */ + @Test + public void testClientStartOnRegWhileActive() throws Exception { + when(mNetwork.getNetId()).thenReturn(TEST_NETWORK_ID); + mWifiScoreReport.startConnectedNetworkScorer(TEST_NETWORK_ID); + mWifiScoreReport.setWifiConnectedNetworkScorer(mAppBinder, mWifiConnectedNetworkScorer); + verify(mWifiConnectedNetworkScorer).onStart(TEST_SESSION_ID); } /** @@ -626,13 +657,12 @@ public class WifiScoreReportTest extends WifiBaseTest { */ @Test public void testClientGetSessionIdOnStop() throws Exception { - ArgumentCaptor<Integer> stopId = ArgumentCaptor.forClass(Integer.class); // Register Client for verification. mWifiScoreReport.setWifiConnectedNetworkScorer(mAppBinder, mWifiConnectedNetworkScorer); - mWifiScoreReport.startConnectedNetworkScorer(TEST_SESSION_ID); + when(mNetwork.getNetId()).thenReturn(TEST_NETWORK_ID); + mWifiScoreReport.startConnectedNetworkScorer(TEST_NETWORK_ID); mWifiScoreReport.stopConnectedNetworkScorer(); - verify(mWifiConnectedNetworkScorer).onStop(stopId.capture()); - assertEquals((int) stopId.getValue(), TEST_SESSION_ID); + verify(mWifiConnectedNetworkScorer).onStop(TEST_SESSION_ID); } /** @@ -655,7 +685,8 @@ public class WifiScoreReportTest extends WifiBaseTest { WifiConnectedNetworkScorerImpl scorerImpl = new WifiConnectedNetworkScorerImpl(); // Register Client for verification. mWifiScoreReport.setWifiConnectedNetworkScorer(mAppBinder, scorerImpl); - mWifiScoreReport.startConnectedNetworkScorer(TEST_SESSION_ID); + when(mNetwork.getNetId()).thenReturn(TEST_NETWORK_ID); + mWifiScoreReport.startConnectedNetworkScorer(TEST_NETWORK_ID); assertEquals(TEST_SESSION_ID, scorerImpl.mSessionId); @@ -687,7 +718,8 @@ public class WifiScoreReportTest extends WifiBaseTest { // Register Client for verification. mWifiScoreReport.setWifiConnectedNetworkScorer(mAppBinder, scorerImpl); mWifiScoreReport.setInterfaceName("wlan0"); - mWifiScoreReport.startConnectedNetworkScorer(TEST_SESSION_ID); + when(mNetwork.getNetId()).thenReturn(TEST_NETWORK_ID); + mWifiScoreReport.startConnectedNetworkScorer(TEST_NETWORK_ID); scorerImpl.mScoreUpdateObserver.triggerUpdateOfWifiUsabilityStats(scorerImpl.mSessionId); mLooper.dispatchAll(); @@ -703,7 +735,8 @@ public class WifiScoreReportTest extends WifiBaseTest { WifiConnectedNetworkScorerImpl scorerImpl = new WifiConnectedNetworkScorerImpl(); // Register Client for verification. mWifiScoreReport.setWifiConnectedNetworkScorer(mAppBinder, scorerImpl); - mWifiScoreReport.startConnectedNetworkScorer(TEST_SESSION_ID); + when(mNetwork.getNetId()).thenReturn(TEST_NETWORK_ID); + mWifiScoreReport.startConnectedNetworkScorer(TEST_NETWORK_ID); mClock.mStepMillis = 0; mClock.mWallClockMillis = 5001; @@ -732,7 +765,8 @@ public class WifiScoreReportTest extends WifiBaseTest { WifiConnectedNetworkScorerImpl scorerImpl = new WifiConnectedNetworkScorerImpl(); // Register Client for verification. mWifiScoreReport.setWifiConnectedNetworkScorer(mAppBinder, scorerImpl); - mWifiScoreReport.startConnectedNetworkScorer(TEST_SESSION_ID); + when(mNetwork.getNetId()).thenReturn(TEST_NETWORK_ID); + mWifiScoreReport.startConnectedNetworkScorer(TEST_NETWORK_ID); mClock.mStepMillis = 0; mClock.mWallClockMillis = 10; @@ -755,7 +789,8 @@ public class WifiScoreReportTest extends WifiBaseTest { WifiConnectedNetworkScorerImpl scorerImpl = new WifiConnectedNetworkScorerImpl(); // Register Client for verification. mWifiScoreReport.setWifiConnectedNetworkScorer(mAppBinder, scorerImpl); - mWifiScoreReport.startConnectedNetworkScorer(TEST_SESSION_ID); + when(mNetwork.getNetId()).thenReturn(TEST_NETWORK_ID); + mWifiScoreReport.startConnectedNetworkScorer(TEST_NETWORK_ID); mClock.mStepMillis = 0; mClock.mWallClockMillis = 10; @@ -777,7 +812,8 @@ public class WifiScoreReportTest extends WifiBaseTest { WifiConnectedNetworkScorerImpl scorerImpl = new WifiConnectedNetworkScorerImpl(); // Register Client for verification. mWifiScoreReport.setWifiConnectedNetworkScorer(mAppBinder, scorerImpl); - mWifiScoreReport.startConnectedNetworkScorer(TEST_SESSION_ID); + when(mNetwork.getNetId()).thenReturn(TEST_NETWORK_ID); + mWifiScoreReport.startConnectedNetworkScorer(TEST_NETWORK_ID); mClock.mStepMillis = 0; mClock.mWallClockMillis = 10; |