From 9574a1beb4a71d4662a88a91834a81e56a77a2e7 Mon Sep 17 00:00:00 2001 From: Quang Luong Date: Thu, 6 Jun 2019 16:51:53 -0700 Subject: [PATCH] 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 --- .../android/settingslib/wifi/AccessPoint.java | 105 +++++++++++------- 1 file 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 e28c612453b4e..fdba64c314ae4 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 { /** The key which identifies this AccessPoint grouping. */ private String mKey; + /** + * Synchronization lock for managing concurrency between main and worker threads. + * + *

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 { } /** The underlying set of scan results comprising this AccessPoint. */ + @GuardedBy("mLock") private final ArraySet 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 mExtraScanResults = new ArraySet<>(); /** @@ -489,8 +500,10 @@ public class AccessPoint implements Comparable { 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 { */ 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 { 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 { */ public Set getScanResults() { Set 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 { 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 { 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 { } 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 { void setScanResultsPasspoint( @Nullable Collection homeScans, @Nullable Collection 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); } }