From 94dfe1cc5974c59359a013bf2a182ff417ce604f Mon Sep 17 00:00:00 2001
From: Guy Hershenbaum <hershi@meta.com>
Date: Thu, 8 Feb 2024 08:09:06 -0800
Subject: [PATCH] Fix NetworkMonitor race condition when dispatching native
 observers

There is a race condition in NetworkMonitor where native observers may be removed concurrently with a notification being dispatched, leading to a dangling pointer dereference (trying to dispatch an observer that was already removed and destroyed), and from there a crash with access violation.

By ensuring dispatching to native observers is done within the synchronization lock that guards additions/removals of native observers protects against this race condition.
Since native observers callbacks are posted to the networking thread in the C++ side anyway, there should be no risk of deadlock/starvation due to long-running observers.

Bug: webrtc:15837
Change-Id: Id2b788f102dbd25de76ceed434c4cd68aa9a569e
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/338643
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#42256}
---
 .../api/org/webrtc/NetworkMonitor.java        | 38 +++++++++----------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/sdk/android/api/org/webrtc/NetworkMonitor.java b/sdk/android/api/org/webrtc/NetworkMonitor.java
index 0bc461df18..ca7e4d5656 100644
--- a/sdk/android/api/org/webrtc/NetworkMonitor.java
+++ b/sdk/android/api/org/webrtc/NetworkMonitor.java
@@ -227,10 +227,13 @@ public class NetworkMonitor {
   /** Alerts all observers of a connection change. */
   private void notifyObserversOfConnectionTypeChange(
       NetworkChangeDetector.ConnectionType newConnectionType) {
-    List<Long> nativeObservers = getNativeNetworkObserversSync();
-    for (Long nativeObserver : nativeObservers) {
-      nativeNotifyConnectionTypeChanged(nativeObserver);
+
+    synchronized (nativeNetworkObservers) {
+      for (Long nativeObserver : nativeNetworkObservers) {
+        nativeNotifyConnectionTypeChanged(nativeObserver);
+      }
     }
+
     // This avoids calling external methods while locking on an object.
     List<NetworkObserver> javaObservers;
     synchronized (networkObservers) {
@@ -243,25 +246,28 @@ public class NetworkMonitor {
 
   private void notifyObserversOfNetworkConnect(
       NetworkChangeDetector.NetworkInformation networkInfo) {
-    List<Long> nativeObservers = getNativeNetworkObserversSync();
-    for (Long nativeObserver : nativeObservers) {
-      nativeNotifyOfNetworkConnect(nativeObserver, networkInfo);
+    synchronized (nativeNetworkObservers) {
+      for (Long nativeObserver : nativeNetworkObservers) {
+        nativeNotifyOfNetworkConnect(nativeObserver, networkInfo);
+      }
     }
   }
 
   private void notifyObserversOfNetworkDisconnect(long networkHandle) {
-    List<Long> nativeObservers = getNativeNetworkObserversSync();
-    for (Long nativeObserver : nativeObservers) {
-      nativeNotifyOfNetworkDisconnect(nativeObserver, networkHandle);
+    synchronized (nativeNetworkObservers) {
+      for (Long nativeObserver : nativeNetworkObservers) {
+        nativeNotifyOfNetworkDisconnect(nativeObserver, networkHandle);
+      }
     }
   }
 
   private void notifyObserversOfNetworkPreference(
       List<NetworkChangeDetector.ConnectionType> types, int preference) {
-    List<Long> nativeObservers = getNativeNetworkObserversSync();
-    for (NetworkChangeDetector.ConnectionType type : types) {
-      for (Long nativeObserver : nativeObservers) {
-        nativeNotifyOfNetworkPreference(nativeObserver, type, preference);
+    synchronized(nativeNetworkObservers) {
+      for (NetworkChangeDetector.ConnectionType type : types) {
+        for (Long nativeObserver : nativeNetworkObservers) {
+          nativeNotifyOfNetworkPreference(nativeObserver, type, preference);
+        }
       }
     }
   }
@@ -282,12 +288,6 @@ public class NetworkMonitor {
     nativeNotifyOfActiveNetworkList(nativeObserver, networkInfos);
   }
 
-  private List<Long> getNativeNetworkObserversSync() {
-    synchronized (nativeNetworkObservers) {
-      return new ArrayList<>(nativeNetworkObservers);
-    }
-  }
-
   /**
    * Adds an observer for any connection type changes.
    *