P2P: Modify loadGroups & fix NPE

Some of the loadGroups logic was incorrectly transformed from socket to
HIDL supplicant calls, (isCurrent vs !isCurrent). Fixed that.

startWpsPbc no longer accepts a null value as a wildcard, and causes a
NPE. Fixed that too.

Bug: 37286961
Test: Manual + CTS Verifier + new unit test
Change-Id: Ie24e89f7bc9d0f03511ace2e5c0ac1cbbe31e359

3 files changed, 39 insertions, 33 deletions

diff --git a/service/java/com/android/server/wifi/p2p/SupplicantP2pIfaceHal.java b/service/java/com/android/server/wifi/p2p/SupplicantP2pIfaceHal.java
index fc63894b8..6f4543edb 100644
--- a/service/java/com/android/server/wifi/p2p/SupplicantP2pIfaceHal.java
+++ b/service/java/com/android/server/wifi/p2p/SupplicantP2pIfaceHal.java
@@ -1471,23 +1471,19 @@ public class SupplicantP2pIfaceHal {
      * @return true, if operation was successful.
      */
     public boolean startWpsPbc(String groupIfName, String bssid) {
-        if (TextUtils.isEmpty(groupIfName)) return false;
+        if (TextUtils.isEmpty(groupIfName)) {
+            Log.e(TAG, "Group name required when requesting WPS PBC. Got (" + groupIfName + ")");
+            return false;
+        }
         synchronized (mLock) {
             if (!checkSupplicantP2pIfaceAndLogFailure("startWpsPbc")) return false;
-            if (groupIfName == null) {
-                Log.e(TAG, "Group name required when requesting WPS PBC.");
-                return false;
-            }
-
             // Null values should be fine, since bssid can be empty.
             byte[] macAddress = null;
-            if (bssid != null) {
-                try {
-                    macAddress = NativeUtil.macAddressToByteArray(bssid);
-                } catch (Exception e) {
-                    Log.e(TAG, "Could not parse BSSID.", e);
-                    return false;
-                }
+            try {
+                macAddress = NativeUtil.macAddressToByteArray(bssid);
+            } catch (Exception e) {
+                Log.e(TAG, "Could not parse BSSID.", e);
+                return false;
             }
 
             SupplicantResult<Void> result = new SupplicantResult(
@@ -1556,13 +1552,11 @@ public class SupplicantP2pIfaceHal {
 
             // Null values should be fine, since bssid can be empty.
             byte[] macAddress = null;
-            if (bssid != null) {
-                try {
-                    macAddress = NativeUtil.macAddressToByteArray(bssid);
-                } catch (Exception e) {
-                    Log.e(TAG, "Could not parse BSSID.", e);
-                    return null;
-                }
+            try {
+                macAddress = NativeUtil.macAddressToByteArray(bssid);
+            } catch (Exception e) {
+                Log.e(TAG, "Could not parse BSSID.", e);
+                return null;
             }
 
             SupplicantResult<String> result = new SupplicantResult(
@@ -1746,8 +1740,9 @@ public class SupplicantP2pIfaceHal {
     }
 
     /**
-     * Populate list of available networks or update existing list.
+     * Get the persistent group list from wpa_supplicant's p2p mgmt interface
      *
+     * @param groups WifiP2pGroupList to store persistent groups in
      * @return true, if list has been modified.
      */
     public boolean loadGroups(WifiP2pGroupList groups) {
@@ -1774,8 +1769,10 @@ public class SupplicantP2pIfaceHal {
                     Log.e(TAG, "ISupplicantP2pIface exception: " + e);
                     supplicantServiceDiedHandler();
                 }
-                if (!resultIsCurrent.isSuccess() || !resultIsCurrent.getResult()) {
-                    Log.i(TAG, "Skipping non current network");
+                /** Skip the current network, if we're somehow getting networks from the p2p GO
+                    interface, instead of p2p mgmt interface*/
+                if (!resultIsCurrent.isSuccess() || resultIsCurrent.getResult()) {
+                    Log.i(TAG, "Skipping current network");
                     continue;
                 }
 
@@ -1796,7 +1793,8 @@ public class SupplicantP2pIfaceHal {
                 }
                 if (resultSsid.isSuccess() && resultSsid.getResult() != null
                         && !resultSsid.getResult().isEmpty()) {
-                    group.setNetworkName(NativeUtil.encodeSsid(resultSsid.getResult()));
+                    group.setNetworkName(NativeUtil.removeEnclosingQuotes(
+                            NativeUtil.encodeSsid(resultSsid.getResult())));
                 }
 
                 SupplicantResult<byte[]> resultBssid =
diff --git a/service/java/com/android/server/wifi/p2p/WifiP2pServiceImpl.java b/service/java/com/android/server/wifi/p2p/WifiP2pServiceImpl.java
index 0c7a91d1c..623a2f0c3 100644
--- a/service/java/com/android/server/wifi/p2p/WifiP2pServiceImpl.java
+++ b/service/java/com/android/server/wifi/p2p/WifiP2pServiceImpl.java
@@ -2364,6 +2364,7 @@ public class WifiP2pServiceImpl extends IWifiP2pManager.Stub {
             pw.println("mWifiP2pInfo " + mWifiP2pInfo);
             pw.println("mGroup " + mGroup);
             pw.println("mSavedPeerConfig " + mSavedPeerConfig);
+            pw.println("mGroups" + mGroups);
             pw.println();
         }
 
diff --git a/tests/wifitests/src/com/android/server/wifi/p2p/SupplicantP2pIfaceHalTest.java b/tests/wifitests/src/com/android/server/wifi/p2p/SupplicantP2pIfaceHalTest.java
index 161296f47..de5f0c0ab 100644
--- a/tests/wifitests/src/com/android/server/wifi/p2p/SupplicantP2pIfaceHalTest.java
+++ b/tests/wifitests/src/com/android/server/wifi/p2p/SupplicantP2pIfaceHalTest.java
@@ -2186,6 +2186,8 @@ public class SupplicantP2pIfaceHalTest {
 
     /**
      * Verify the loading of group info.
+     * Specifically, all groups returned by listNetworks are added as a persistent group, so long as
+     * they are NOT current.
      */
     @Test
     public void testLoadGroups() throws Exception {
@@ -2193,11 +2195,11 @@ public class SupplicantP2pIfaceHalTest {
 
         // Class to hold the P2p group info returned from the HIDL interface.
         class P2pGroupInfo {
-            public ArrayList<Byte> ssid;
+            public String ssid;
             public byte[] bssid;
             public boolean isGo;
             public boolean isCurrent;
-            P2pGroupInfo(ArrayList<Byte> ssid, byte[] bssid, boolean isGo, boolean isCurrent) {
+            P2pGroupInfo(String ssid, byte[] bssid, boolean isGo, boolean isCurrent) {
                 this.ssid = ssid;
                 this.bssid = bssid;
                 this.isGo = isGo;
@@ -2207,16 +2209,20 @@ public class SupplicantP2pIfaceHalTest {
 
         Map<Integer, P2pGroupInfo> groups = new HashMap<>();
         groups.put(0, new P2pGroupInfo(
-                NativeUtil.decodeSsid("\"test_34\""),
+                "test_34",
                 NativeUtil.macAddressToByteArray("56:34:ab:12:12:34"),
                 false, false));
         groups.put(1, new P2pGroupInfo(
-                NativeUtil.decodeSsid("\"test_1234\""),
+                "test_1234",
                 NativeUtil.macAddressToByteArray("16:ed:ab:12:45:34"),
-                true, true));
+                true, false));
         groups.put(2, new P2pGroupInfo(
-                NativeUtil.decodeSsid("\"test_4545\""),
+                "test_4545",
                 NativeUtil.macAddressToByteArray("32:89:23:56:45:34"),
+                true, false));
+        groups.put(3, new P2pGroupInfo(
+                "iShouldntBeHere",
+                NativeUtil.macAddressToByteArray("aa:bb:cc:56:45:34"),
                 true, true));
 
         doAnswer(new AnswerWithArguments() {
@@ -2231,7 +2237,8 @@ public class SupplicantP2pIfaceHalTest {
                 try {
                     doAnswer(new AnswerWithArguments() {
                         public void answer(ISupplicantP2pNetwork.getSsidCallback cb) {
-                            cb.onValues(mStatusSuccess, groups.get(networkId).ssid);
+                            cb.onValues(mStatusSuccess,
+                                    NativeUtil.stringToByteArrayList(groups.get(networkId).ssid));
                             return;
                         }
                     }).when(mISupplicantP2pNetworkMock)
@@ -2268,10 +2275,10 @@ public class SupplicantP2pIfaceHalTest {
         WifiP2pGroupList p2pGroups = new WifiP2pGroupList();
         assertTrue(mDut.loadGroups(p2pGroups));
 
-        assertEquals(2, p2pGroups.getGroupList().size());
+        assertEquals(3, p2pGroups.getGroupList().size());
         for (WifiP2pGroup group : p2pGroups.getGroupList()) {
             int networkId = group.getNetworkId();
-            assertEquals(NativeUtil.encodeSsid(groups.get(networkId).ssid), group.getNetworkName());
+            assertEquals(groups.get(networkId).ssid, group.getNetworkName());
             assertEquals(
                     NativeUtil.macAddressFromByteArray(groups.get(networkId).bssid),
                     group.getOwner().deviceAddress);