From 31f4cf48ac922a0ab79eb1a3d94b147dc72b4b1b Mon Sep 17 00:00:00 2001
From: Etan Cohen <etancohen@google.com>
Date: Tue, 29 Oct 2019 12:26:09 -0700
Subject: [WifiLock] Move arg validity check earlier - prevent service crash

Move the argument validity check into the binder thread - which crashes
the caller as opposed to the service.

Bug: 143518156
Test: atest com.android.server.wifi
Merged-In: Ic7c3f59524aa6a0e2aa41b34dd8179b31dba84e3
Change-Id: Ic7c3f59524aa6a0e2aa41b34dd8179b31dba84e3
---
 service/java/com/android/server/wifi/WifiLockManager.java   | 12 +++++++-----
 service/java/com/android/server/wifi/WifiServiceImpl.java   |  4 ++++
 .../src/com/android/server/wifi/WifiLockManagerTest.java    | 11 -----------
 .../src/com/android/server/wifi/WifiServiceImplTest.java    | 13 +++++++++++++
 4 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/service/java/com/android/server/wifi/WifiLockManager.java b/service/java/com/android/server/wifi/WifiLockManager.java
index e292a84d8..45e68824c 100644
--- a/service/java/com/android/server/wifi/WifiLockManager.java
+++ b/service/java/com/android/server/wifi/WifiLockManager.java
@@ -183,10 +183,6 @@ public class WifiLockManager {
      * @return true if the lock was successfully acquired, false if the lockMode was invalid.
      */
     public boolean acquireWifiLock(int lockMode, String tag, IBinder binder, WorkSource ws) {
-        if (!isValidLockMode(lockMode)) {
-            throw new IllegalArgumentException("lockMode =" + lockMode);
-        }
-
         // Make a copy of the WorkSource before adding it to the WakeLock
         // This is to make sure worksource value can not be changed by caller
         // after function returns.
@@ -384,7 +380,13 @@ public class WifiLockManager {
         }
     }
 
-    private static boolean isValidLockMode(int lockMode) {
+    /**
+     * Validate that the lock mode is valid - i.e. one of the supported enumerations.
+     *
+     * @param lockMode The lock mode to verify.
+     * @return true for valid lock modes, false otherwise.
+     */
+    public static boolean isValidLockMode(int lockMode) {
         if (lockMode != WifiManager.WIFI_MODE_FULL
                 && lockMode != WifiManager.WIFI_MODE_SCAN_ONLY
                 && lockMode != WifiManager.WIFI_MODE_FULL_HIGH_PERF
diff --git a/service/java/com/android/server/wifi/WifiServiceImpl.java b/service/java/com/android/server/wifi/WifiServiceImpl.java
index 4dbf71fd7..0afa317c9 100644
--- a/service/java/com/android/server/wifi/WifiServiceImpl.java
+++ b/service/java/com/android/server/wifi/WifiServiceImpl.java
@@ -2823,6 +2823,10 @@ public class WifiServiceImpl extends BaseWifiService {
         WorkSource updatedWs = (ws == null || ws.isEmpty())
                 ? new WorkSource(Binder.getCallingUid()) : ws;
 
+        if (!WifiLockManager.isValidLockMode(lockMode)) {
+            throw new IllegalArgumentException("lockMode =" + lockMode);
+        }
+
         Mutable<Boolean> lockSuccess = new Mutable<>();
         boolean runWithScissorsSuccess = mWifiInjector.getClientModeImplHandler().runWithScissors(
                 () -> {
diff --git a/tests/wifitests/src/com/android/server/wifi/WifiLockManagerTest.java b/tests/wifitests/src/com/android/server/wifi/WifiLockManagerTest.java
index a40de55e9..255073b8b 100644
--- a/tests/wifitests/src/com/android/server/wifi/WifiLockManagerTest.java
+++ b/tests/wifitests/src/com/android/server/wifi/WifiLockManagerTest.java
@@ -160,17 +160,6 @@ public class WifiLockManagerTest {
         assertEquals(WifiManager.WIFI_MODE_NO_LOCKS_HELD, mWifiLockManager.getStrongestLockMode());
     }
 
-    /**
-     * Test to verify that the lock mode is verified before adding a lock.
-     *
-     * Steps: call acquireWifiLock with an invalid lock mode.
-     * Expected: the call should throw an IllegalArgumentException.
-     */
-    @Test(expected = IllegalArgumentException.class)
-    public void acquireWifiLockShouldThrowExceptionOnInivalidLockMode() throws Exception {
-        mWifiLockManager.acquireWifiLock(WIFI_LOCK_MODE_INVALID, "", mBinder, mWorkSource);
-    }
-
     /**
      * Test that a call to acquireWifiLock with valid parameters works.
      *
diff --git a/tests/wifitests/src/com/android/server/wifi/WifiServiceImplTest.java b/tests/wifitests/src/com/android/server/wifi/WifiServiceImplTest.java
index bd8129a38..d5f7dd981 100644
--- a/tests/wifitests/src/com/android/server/wifi/WifiServiceImplTest.java
+++ b/tests/wifitests/src/com/android/server/wifi/WifiServiceImplTest.java
@@ -4271,4 +4271,17 @@ public class WifiServiceImplTest {
         } catch (RemoteException e) {
         }
     }
+
+    /**
+     * Test to verify that the lock mode is verified before dispatching the operation
+     *
+     * Steps: call acquireWifiLock with an invalid lock mode.
+     * Expected: the call should throw an IllegalArgumentException.
+     */
+    @Test(expected = IllegalArgumentException.class)
+    public void acquireWifiLockShouldThrowExceptionOnInvalidLockMode() throws Exception {
+        final int wifiLockModeInvalid = -1;
+
+        mWifiServiceImpl.acquireWifiLock(mAppBinder, wifiLockModeInvalid, "", null);
+    }
 }
-- 
cgit v1.2.3