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

2 files changed, 80 insertions, 17 deletions

diff --git a/service/java/com/android/server/wifi/scanner/WifiScanningServiceImpl.java b/service/java/com/android/server/wifi/scanner/WifiScanningServiceImpl.java
index 7024e3b86..6ae223717 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 @@ public class WifiScanningServiceImpl extends IWifiScanner.Stub {
                         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 @@ public class WifiScanningServiceImpl extends IWifiScanner.Stub {
             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 @@ public class WifiScanningServiceImpl extends IWifiScanner.Stub {
                 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 @@ public class WifiScanningServiceImpl extends IWifiScanner.Stub {
                 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 @@ public class WifiScanningServiceImpl extends IWifiScanner.Stub {
                 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 @@ public class WifiScanningServiceImpl extends IWifiScanner.Stub {
             }
         }
 
-        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 @@ public class WifiScanningServiceImpl extends IWifiScanner.Stub {
                         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 @@ public class WifiScanningServiceImpl extends IWifiScanner.Stub {
         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 @@ public class WifiScanningServiceImpl extends IWifiScanner.Stub {
         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 ceed019c3..03a11dceb 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.test.suitebuilder.annotation.SmallTest;
 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 @@ public class WifiScanningServiceTest {
         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());
+    }
+
 }