WifiScanningServiceImpl: Add ClientInfo null checks
Add missing null checks for |ClientInfo| in a few places. ClientInfo
could end up being null if there was a pending cleanup of the client
before processing of the request in the appropriate state machine.
Also, add a unit test to simulate the scenario in the bug specified.
BUG: 30241457
Change-Id: Ic4412ae03b5176764b10cba357d19086c0c09e6e
TEST: Unit tests
diff --git a/service/java/com/android/server/wifi/scanner/WifiScanningServiceImpl.java b/service/java/com/android/server/wifi/scanner/WifiScanningServiceImpl.java
index 7024e3b..6ae2237 100644
--- a/service/java/com/android/server/wifi/scanner/WifiScanningServiceImpl.java
+++ b/service/java/com/android/server/wifi/scanner/WifiScanningServiceImpl.java
@@ -1052,8 +1052,11 @@
replySucceeded(msg);
break;
case WifiScanner.CMD_SET_HOTLIST:
- addHotlist(ci, msg.arg2, (WifiScanner.HotlistSettings) msg.obj);
- replySucceeded(msg);
+ if (addHotlist(ci, msg.arg2, (WifiScanner.HotlistSettings) msg.obj)) {
+ replySucceeded(msg);
+ } else {
+ replyFailed(msg, WifiScanner.REASON_INVALID_REQUEST, "bad request");
+ }
break;
case WifiScanner.CMD_RESET_HOTLIST:
removeHotlist(ci, msg.arg2);
@@ -1290,14 +1293,22 @@
mActiveBackgroundScans.clear();
}
- private void addHotlist(ClientInfo ci, int handler, WifiScanner.HotlistSettings settings) {
+ private boolean addHotlist(ClientInfo ci, int handler,
+ WifiScanner.HotlistSettings settings) {
+ if (ci == null) {
+ Log.d(TAG, "Failing hotlist request ClientInfo not found " + handler);
+ return false;
+ }
mActiveHotlistSettings.addRequest(ci, handler, null, settings);
resetHotlist();
+ return true;
}
private void removeHotlist(ClientInfo ci, int handler) {
- mActiveHotlistSettings.removeRequest(ci, handler);
- resetHotlist();
+ if (ci != null) {
+ mActiveHotlistSettings.removeRequest(ci, handler);
+ resetHotlist();
+ }
}
private void resetHotlist() {
@@ -2162,9 +2173,12 @@
ClientInfo ci = mClients.get(msg.replyTo);
switch (msg.what) {
case WifiScanner.CMD_START_TRACKING_CHANGE:
- addWifiChangeHandler(ci, msg.arg2);
- replySucceeded(msg);
- transitionTo(mMovingState);
+ if (addWifiChangeHandler(ci, msg.arg2)) {
+ replySucceeded(msg);
+ transitionTo(mMovingState);
+ } else {
+ replyFailed(msg, WifiScanner.REASON_INVALID_REQUEST, "bad request");
+ }
break;
case WifiScanner.CMD_STOP_TRACKING_CHANGE:
// nothing to do
@@ -2196,8 +2210,11 @@
ClientInfo ci = mClients.get(msg.replyTo);
switch (msg.what) {
case WifiScanner.CMD_START_TRACKING_CHANGE:
- addWifiChangeHandler(ci, msg.arg2);
- replySucceeded(msg);
+ if (addWifiChangeHandler(ci, msg.arg2)) {
+ replySucceeded(msg);
+ } else {
+ replyFailed(msg, WifiScanner.REASON_INVALID_REQUEST, "bad request");
+ }
break;
case WifiScanner.CMD_STOP_TRACKING_CHANGE:
removeWifiChangeHandler(ci, msg.arg2);
@@ -2249,8 +2266,11 @@
ClientInfo ci = mClients.get(msg.replyTo);
switch (msg.what) {
case WifiScanner.CMD_START_TRACKING_CHANGE:
- addWifiChangeHandler(ci, msg.arg2);
- replySucceeded(msg);
+ if (addWifiChangeHandler(ci, msg.arg2)) {
+ replySucceeded(msg);
+ } else {
+ replyFailed(msg, WifiScanner.REASON_INVALID_REQUEST, "bad request");
+ }
break;
case WifiScanner.CMD_STOP_TRACKING_CHANGE:
removeWifiChangeHandler(ci, msg.arg2);
@@ -2476,7 +2496,11 @@
}
}
- private void addWifiChangeHandler(ClientInfo ci, int handler) {
+ private boolean addWifiChangeHandler(ClientInfo ci, int handler) {
+ if (ci == null) {
+ Log.d(TAG, "Failing wifi change request ClientInfo not found " + handler);
+ return false;
+ }
mActiveWifiChangeHandlers.add(Pair.create(ci, handler));
// Add an internal client to make background scan requests.
if (mInternalClientInfo == null) {
@@ -2484,11 +2508,14 @@
new InternalClientInfo(ci.getUid(), new Messenger(this.getHandler()));
mInternalClientInfo.register();
}
+ return true;
}
private void removeWifiChangeHandler(ClientInfo ci, int handler) {
- mActiveWifiChangeHandlers.remove(Pair.create(ci, handler));
- untrackSignificantWifiChangeOnEmpty();
+ if (ci != null) {
+ mActiveWifiChangeHandlers.remove(Pair.create(ci, handler));
+ untrackSignificantWifiChangeOnEmpty();
+ }
}
private void untrackSignificantWifiChangeOnEmpty() {
@@ -2602,7 +2629,7 @@
StringBuilder sb = new StringBuilder();
sb.append(request)
.append(": ")
- .append(ci.toString())
+ .append((ci == null) ? "ClientInfo[unknown]" : ci.toString())
.append(",Id=")
.append(id);
if (workSource != null) {
@@ -2623,7 +2650,7 @@
StringBuilder sb = new StringBuilder();
sb.append(callback)
.append(": ")
- .append(ci.toString())
+ .append((ci == null) ? "ClientInfo[unknown]" : ci.toString())
.append(",Id=")
.append(id);
if (extra != null) {
diff --git a/tests/wifitests/src/com/android/server/wifi/scanner/WifiScanningServiceTest.java b/tests/wifitests/src/com/android/server/wifi/scanner/WifiScanningServiceTest.java
index ceed019..03a11dc 100644
--- a/tests/wifitests/src/com/android/server/wifi/scanner/WifiScanningServiceTest.java
+++ b/tests/wifitests/src/com/android/server/wifi/scanner/WifiScanningServiceTest.java
@@ -39,6 +39,7 @@
import android.util.Pair;
import com.android.internal.app.IBatteryStats;
+import com.android.internal.util.AsyncChannel;
import com.android.internal.util.Protocol;
import com.android.server.wifi.BidirectionalAsyncChannel;
import com.android.server.wifi.Clock;
@@ -1635,4 +1636,39 @@
expectSwPnoScan(order, scanSettings.second, scanResults);
verifyPnoNetworkFoundRecieved(order, handler, requestId, scanResults.getRawScanResults());
}
+
+ /**
+ * Tries to simulate the race scenario where a client is disconnected immediately after single
+ * scan request is sent to |SingleScanStateMachine|.
+ */
+ @Test
+ public void processSingleScanRequestAfterDisconnect() throws Exception {
+ startServiceAndLoadDriver();
+ BidirectionalAsyncChannel controlChannel = connectChannel(mock(Handler.class));
+ mLooper.dispatchAll();
+
+ // Send the single scan request and then send the disconnect immediately after.
+ WifiScanner.ScanSettings requestSettings = createRequest(WifiScanner.WIFI_BAND_BOTH, 0,
+ 0, 20, WifiScanner.REPORT_EVENT_AFTER_EACH_SCAN);
+ int requestId = 10;
+
+ sendSingleScanRequest(controlChannel, requestId, requestSettings, null);
+ // Can't call |disconnect| here because that sends |CMD_CHANNEL_DISCONNECT| followed by
+ // |CMD_CHANNEL_DISCONNECTED|.
+ controlChannel.sendMessage(Message.obtain(null, AsyncChannel.CMD_CHANNEL_DISCONNECTED, 0,
+ 0, null));
+
+ // Now process the above 2 actions. This should result in first processing the single scan
+ // request (which forwards the request to SingleScanStateMachine) and then processing the
+ // disconnect after.
+ mLooper.dispatchAll();
+
+ // Now check that we logged the invalid request.
+ String serviceDump = dumpService();
+ Pattern logLineRegex = Pattern.compile("^.+" + "singleScanInvalidRequest: "
+ + "ClientInfo\\[unknown\\],Id=" + requestId + ",bad request$", Pattern.MULTILINE);
+ assertTrue("dump did not contain log with ClientInfo[unknown]: " + serviceDump + "\n",
+ logLineRegex.matcher(serviceDump).find());
+ }
+
}