From ce63e16ca85fd24fba7be148b13e2da04f4eddd1 Mon Sep 17 00:00:00 2001 From: Yan Yan Date: Thu, 9 Jan 2025 11:11:56 -0800 Subject: [PATCH] Update VcnNetworkProvider to use thread safe Sets This patch is to fix a race condition when two threads access VcnNetworkProvider.mRequests at the same time. The issue happened when the VCN thread is removing elements and the binder thread is iterating the Set. It causes the binder thread to end up with a Null element and throw NPE This patch fixes this threading issue by changing the Set to be a thread-safe Set Bug: 382106220 Test: atest CtsVcnTestCases && atest FrameworksVcnTests Test: Set up test environment to make the race condition 90% reproducible; verified the fix by running the test for 100 times on forrest Flag: EXEMPT; low risk and small scope bug fix Change-Id: Id7598a861368454efcd0aa49721c54e11aaebdf1 --- Vcn/service-b/src/com/android/server/vcn/Vcn.java | 2 ++ .../src/com/android/server/vcn/VcnNetworkProvider.java | 8 +++++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/Vcn/service-b/src/com/android/server/vcn/Vcn.java b/Vcn/service-b/src/com/android/server/vcn/Vcn.java index 369ef6ae6a..97f86b1bff 100644 --- a/Vcn/service-b/src/com/android/server/vcn/Vcn.java +++ b/Vcn/service-b/src/com/android/server/vcn/Vcn.java @@ -209,6 +209,8 @@ public class Vcn extends Handler { this(vcnContext, subscriptionGroup, config, snapshot, vcnCallback, new Dependencies()); } + // WARNING: This constructor executes on the binder thread. Thread safety MUST be ensured when + // accessing data within this constructor and any methods called from here. @VisibleForTesting(visibility = Visibility.PRIVATE) public Vcn( @NonNull VcnContext vcnContext, diff --git a/Vcn/service-b/src/com/android/server/vcn/VcnNetworkProvider.java b/Vcn/service-b/src/com/android/server/vcn/VcnNetworkProvider.java index 99c848f53c..38fcf09145 100644 --- a/Vcn/service-b/src/com/android/server/vcn/VcnNetworkProvider.java +++ b/Vcn/service-b/src/com/android/server/vcn/VcnNetworkProvider.java @@ -36,7 +36,6 @@ import android.net.vcn.VcnGatewayConnectionConfig; import android.os.Build; import android.os.Handler; import android.os.Looper; -import android.util.ArraySet; import android.util.IndentingPrintWriter; import android.util.Slog; @@ -46,6 +45,7 @@ import com.android.modules.utils.HandlerExecutor; import java.util.Objects; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.Executor; /** @@ -56,12 +56,14 @@ import java.util.concurrent.Executor; * * @hide */ +// TODO(b/388919146): Implement a more generic solution to prevent concurrent modifications on +// mListeners and mRequests // TODO(b/374174952): Replace VANILLA_ICE_CREAM with BAKLAVA after Android B finalization @TargetApi(Build.VERSION_CODES.VANILLA_ICE_CREAM) public class VcnNetworkProvider extends NetworkProvider { private static final String TAG = VcnNetworkProvider.class.getSimpleName(); - private final Set mListeners = new ArraySet<>(); + private final Set mListeners = ConcurrentHashMap.newKeySet(); private final Context mContext; private final Handler mHandler; @@ -72,7 +74,7 @@ public class VcnNetworkProvider extends NetworkProvider { * *

NetworkRequests are immutable once created, and therefore can be used as stable keys. */ - private final Set mRequests = new ArraySet<>(); + private final Set mRequests = ConcurrentHashMap.newKeySet(); public VcnNetworkProvider(@NonNull Context context, @NonNull Looper looper) { this(context, looper, new Dependencies());