From 5c5b485013e88e20119aa17d68ae6b3dd8b306bc Mon Sep 17 00:00:00 2001
From: Benedict Wong <benedictwong@google.com>
Date: Thu, 20 May 2021 16:42:42 -0700
Subject: [PATCH] Prevent concurrent modification exceptions in
 VcnManagementService

In order to prevent concurrent modifications from triggering an
exception in the dump, proxy it to the handler thread.

Adding locks was considered, but would require auditing all callbacks
and proxying them to the handler threads in order to ensure lock
inversion and deadlocks never occur.

Bug: 188840899
Test: FrameworksVcnTests
Test: adb shell dumpsys vcn_management
Change-Id: Ibcc4bdc06301dd77260adaa0c086529f8f524679
Merged-In: Ibcc4bdc06301dd77260adaa0c086529f8f524679
(cherry picked from commit f8ce665d9fd85e1daadc251f905058ad5f88b44c)
---
 .../android/server/VcnManagementService.java  | 69 ++++++++++---------
 1 file changed, 37 insertions(+), 32 deletions(-)

diff --git a/services/core/java/com/android/server/VcnManagementService.java b/services/core/java/com/android/server/VcnManagementService.java
index bcbd692a2f7d4..be9221c43611e 100644
--- a/services/core/java/com/android/server/VcnManagementService.java
+++ b/services/core/java/com/android/server/VcnManagementService.java
@@ -148,6 +148,7 @@ import java.util.concurrent.TimeUnit;
 // TODO(b/180451994): ensure all incoming + outgoing calls have a cleared calling identity
 public class VcnManagementService extends IVcnManagementService.Stub {
     @NonNull private static final String TAG = VcnManagementService.class.getSimpleName();
+    private static final long DUMP_TIMEOUT_MILLIS = TimeUnit.SECONDS.toMillis(5);
 
     public static final boolean VDBG = false; // STOPSHIP: if true
 
@@ -1001,46 +1002,50 @@ public class VcnManagementService extends IVcnManagementService.Stub {
 
         final IndentingPrintWriter pw = new IndentingPrintWriter(writer, "  ");
 
-        pw.println("VcnManagementService dump:");
-        pw.increaseIndent();
-
-        pw.println("mNetworkProvider:");
-        pw.increaseIndent();
-        mNetworkProvider.dump(pw);
-        pw.decreaseIndent();
-        pw.println();
-
-        pw.println("mTrackingNetworkCallback:");
-        pw.increaseIndent();
-        mTrackingNetworkCallback.dump(pw);
-        pw.decreaseIndent();
-        pw.println();
-
-        synchronized (mLock) {
-            pw.println("mLastSnapshot:");
+        // Post to handler thread to prevent ConcurrentModificationExceptions, and avoid lock-hell.
+        mHandler.runWithScissors(() -> {
+            pw.println("VcnManagementService dump:");
             pw.increaseIndent();
-            mLastSnapshot.dump(pw);
+
+            pw.println("mNetworkProvider:");
+            pw.increaseIndent();
+            mNetworkProvider.dump(pw);
             pw.decreaseIndent();
             pw.println();
 
-            pw.println("mConfigs:");
+            pw.println("mTrackingNetworkCallback:");
             pw.increaseIndent();
-            for (Entry<ParcelUuid, VcnConfig> entry : mConfigs.entrySet()) {
-                pw.println(entry.getKey() + ": " + entry.getValue().getProvisioningPackageName());
+            mTrackingNetworkCallback.dump(pw);
+            pw.decreaseIndent();
+            pw.println();
+
+            synchronized (mLock) {
+                pw.println("mLastSnapshot:");
+                pw.increaseIndent();
+                mLastSnapshot.dump(pw);
+                pw.decreaseIndent();
+                pw.println();
+
+                pw.println("mConfigs:");
+                pw.increaseIndent();
+                for (Entry<ParcelUuid, VcnConfig> entry : mConfigs.entrySet()) {
+                    pw.println(entry.getKey() + ": "
+                            + entry.getValue().getProvisioningPackageName());
+                }
+                pw.decreaseIndent();
+                pw.println();
+
+                pw.println("mVcns:");
+                pw.increaseIndent();
+                for (Vcn vcn : mVcns.values()) {
+                    vcn.dump(pw);
+                }
+                pw.decreaseIndent();
+                pw.println();
             }
-            pw.decreaseIndent();
-            pw.println();
 
-            pw.println("mVcns:");
-            pw.increaseIndent();
-            for (Vcn vcn : mVcns.values()) {
-                vcn.dump(pw);
-            }
             pw.decreaseIndent();
-            pw.println();
-        }
-
-        pw.decreaseIndent();
+        }, DUMP_TIMEOUT_MILLIS);
     }
 
     // TODO(b/180452282): Make name more generic and implement directly with VcnManagementService