Made AccessPoint's scan result list thread safe

If wifi verbose logging is enabled, the main thread may iterate through
the scan result list while the worker thread updates it, resulting in
an ArrayIndexOutOfBounds exception. This change adds a lock to every
iteration through mScanResults and mExtraScanResults.

Bug: 134098126
Test: atest WifiTrackerTest && atest AccessPointTest
Change-Id: If42a9ad8031ff66b1732a79772ba441185f7a18f

1 files changed, 66 insertions, 39 deletions

diff --git a/packages/SettingsLib/src/com/android/settingslib/wifi/AccessPoint.java b/packages/SettingsLib/src/com/android/settingslib/wifi/AccessPoint.java
index e28c612453b4..fdba64c314ae 100644
--- a/packages/SettingsLib/src/com/android/settingslib/wifi/AccessPoint.java
+++ b/packages/SettingsLib/src/com/android/settingslib/wifi/AccessPoint.java
@@ -55,6 +55,7 @@ import android.util.ArraySet;
 import android.util.Log;
 import android.util.Pair;
 
+import androidx.annotation.GuardedBy;
 import androidx.annotation.NonNull;
 
 import com.android.internal.annotations.VisibleForTesting;
@@ -108,6 +109,14 @@ public class AccessPoint implements Comparable<AccessPoint> {
     /** The key which identifies this AccessPoint grouping. */
     private String mKey;
 
+    /**
+     * Synchronization lock for managing concurrency between main and worker threads.
+     *
+     * <p>This lock should be held for all modifications to {@link #mScanResults} and
+     * {@link #mExtraScanResults}.
+     */
+    private final Object mLock = new Object();
+
     @IntDef({Speed.NONE, Speed.SLOW, Speed.MODERATE, Speed.FAST, Speed.VERY_FAST})
     @Retention(RetentionPolicy.SOURCE)
     public @interface Speed {
@@ -134,12 +143,14 @@ public class AccessPoint implements Comparable<AccessPoint> {
     }
 
     /** The underlying set of scan results comprising this AccessPoint. */
+    @GuardedBy("mLock")
     private final ArraySet<ScanResult> mScanResults = new ArraySet<>();
 
     /**
      * Extra set of unused scan results corresponding to this AccessPoint for verbose logging
      * purposes, such as a set of Passpoint roaming scan results when home scans are available.
      */
+    @GuardedBy("mLock")
     private final ArraySet<ScanResult> mExtraScanResults = new ArraySet<>();
 
     /**
@@ -489,8 +500,10 @@ public class AccessPoint implements Comparable<AccessPoint> {
 
         if (isVerboseLoggingEnabled()) {
             builder.append(",rssi=").append(mRssi);
-            builder.append(",scan cache size=").append(mScanResults.size()
-                    + mExtraScanResults.size());
+            synchronized (mLock) {
+                builder.append(",scan cache size=").append(mScanResults.size()
+                        + mExtraScanResults.size());
+            }
         }
 
         return builder.append(')').toString();
@@ -532,18 +545,20 @@ public class AccessPoint implements Comparable<AccessPoint> {
      */
     private boolean updateScores(WifiNetworkScoreCache scoreCache, long maxScoreCacheAgeMillis) {
         long nowMillis = SystemClock.elapsedRealtime();
-        for (ScanResult result : mScanResults) {
-            ScoredNetwork score = scoreCache.getScoredNetwork(result);
-            if (score == null) {
-                continue;
-            }
-            TimestampedScoredNetwork timedScore = mScoredNetworkCache.get(result.BSSID);
-            if (timedScore == null) {
-                mScoredNetworkCache.put(
-                        result.BSSID, new TimestampedScoredNetwork(score, nowMillis));
-            } else {
-                // Update data since the has been seen in the score cache
-                timedScore.update(score, nowMillis);
+        synchronized (mLock) {
+            for (ScanResult result : mScanResults) {
+                ScoredNetwork score = scoreCache.getScoredNetwork(result);
+                if (score == null) {
+                    continue;
+                }
+                TimestampedScoredNetwork timedScore = mScoredNetworkCache.get(result.BSSID);
+                if (timedScore == null) {
+                    mScoredNetworkCache.put(
+                            result.BSSID, new TimestampedScoredNetwork(score, nowMillis));
+                } else {
+                    // Update data since the has been seen in the score cache
+                    timedScore.update(score, nowMillis);
+                }
             }
         }
 
@@ -619,12 +634,14 @@ public class AccessPoint implements Comparable<AccessPoint> {
                 mIsScoredNetworkMetered |= score.meteredHint;
             }
         } else {
-            for (ScanResult result : mScanResults) {
-                ScoredNetwork score = scoreCache.getScoredNetwork(result);
-                if (score == null) {
-                    continue;
+            synchronized (mLock) {
+                for (ScanResult result : mScanResults) {
+                    ScoredNetwork score = scoreCache.getScoredNetwork(result);
+                    if (score == null) {
+                        continue;
+                    }
+                    mIsScoredNetworkMetered |= score.meteredHint;
                 }
-                mIsScoredNetworkMetered |= score.meteredHint;
             }
         }
         return oldMetering == mIsScoredNetworkMetered;
@@ -741,8 +758,10 @@ public class AccessPoint implements Comparable<AccessPoint> {
      */
     public Set<ScanResult> getScanResults() {
         Set<ScanResult> allScans = new ArraySet<>();
-        allScans.addAll(mScanResults);
-        allScans.addAll(mExtraScanResults);
+        synchronized (mLock) {
+            allScans.addAll(mScanResults);
+            allScans.addAll(mExtraScanResults);
+        }
         return allScans;
     }
 
@@ -766,10 +785,12 @@ public class AccessPoint implements Comparable<AccessPoint> {
 
         ScanResult bestResult = null;
         int bestRssi = UNREACHABLE_RSSI;
-        for (ScanResult result : mScanResults) {
-            if (result.level > bestRssi) {
-                bestRssi = result.level;
-                bestResult = result;
+        synchronized (mLock) {
+            for (ScanResult result : mScanResults) {
+                if (result.level > bestRssi) {
+                    bestRssi = result.level;
+                    bestResult = result;
+                }
             }
         }
 
@@ -1211,9 +1232,11 @@ public class AccessPoint implements Comparable<AccessPoint> {
         savedState.putInt(KEY_EAPTYPE, mEapType);
         if (mConfig != null) savedState.putParcelable(KEY_CONFIG, mConfig);
         savedState.putParcelable(KEY_WIFIINFO, mInfo);
-        savedState.putParcelableArray(KEY_SCANRESULTS,
-                mScanResults.toArray(new Parcelable[mScanResults.size()
-                        + mExtraScanResults.size()]));
+        synchronized (mLock) {
+            savedState.putParcelableArray(KEY_SCANRESULTS,
+                    mScanResults.toArray(new Parcelable[mScanResults.size()
+                            + mExtraScanResults.size()]));
+        }
         savedState.putParcelableArrayList(KEY_SCOREDNETWORKCACHE,
                 new ArrayList<>(mScoredNetworkCache.values()));
         if (mNetworkInfo != null) {
@@ -1292,8 +1315,10 @@ public class AccessPoint implements Comparable<AccessPoint> {
         }
 
         int oldLevel = getLevel();
-        mScanResults.clear();
-        mScanResults.addAll(scanResults);
+        synchronized (mLock) {
+            mScanResults.clear();
+            mScanResults.addAll(scanResults);
+        }
         updateBestRssiInfo();
         int newLevel = getLevel();
 
@@ -1324,16 +1349,18 @@ public class AccessPoint implements Comparable<AccessPoint> {
     void setScanResultsPasspoint(
             @Nullable Collection<ScanResult> homeScans,
             @Nullable Collection<ScanResult> roamingScans) {
-        mExtraScanResults.clear();
-        if (!CollectionUtils.isEmpty(homeScans)) {
-            if (!CollectionUtils.isEmpty(roamingScans)) {
-                mExtraScanResults.addAll(roamingScans);
+        synchronized (mLock) {
+            mExtraScanResults.clear();
+            if (!CollectionUtils.isEmpty(homeScans)) {
+                mIsRoaming = false;
+                if (!CollectionUtils.isEmpty(roamingScans)) {
+                    mExtraScanResults.addAll(roamingScans);
+                }
+                setScanResults(homeScans);
+            } else if (!CollectionUtils.isEmpty(roamingScans)) {
+                mIsRoaming = true;
+                setScanResults(roamingScans);
             }
-            mIsRoaming = false;
-            setScanResults(homeScans);
-        } else if (!CollectionUtils.isEmpty(roamingScans)) {
-            mIsRoaming = true;
-            setScanResults(roamingScans);
         }
     }