From 9aac13d4dc95925b57f03e7964fc7add6834e518 Mon Sep 17 00:00:00 2001
From: Ruslan Shestopalyuk <rshest@meta.com>
Date: Fri, 17 Feb 2023 15:19:44 -0800
Subject: [PATCH] Fix race condition in ReadableNativeMap (#36201)

Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/36201

[Changelog][Internal]

Guard call to the C++ ReadableNAtiveMap.importValues with a lock.

Note that all the occurrences in this class (together with importTypes) already were protected by a lock, except of this one, which with the very high chance caused crashes in T145271136.

My corresponding comment from the task,  for justification:
> If callstack to be trusted, the crash happens on the C++ side, in ReadableNativeMap::importValues().
It throws ArrayIndexOutOfBoundsException, which, looking at the code, seems to be only possible due to a corrupted data or race conditions.

> Now, looking at the Java side of ReadableNativeMap, and the particular call site... it's very dodgy, since all other occurrences of calling to native importTypes/importValues are guarded by locks, but the one crashing isn't.

NOTE: A couple of `importKeys()` instances appears to suffer from the same problem as well.

Reviewed By: javache

Differential Revision: D43398416

fbshipit-source-id: 0402de5dc723a2fba7d0247c8ad4aeff150d8340
---
 .../react/bridge/ReadableNativeMap.java       | 88 ++++++++++---------
 1 file changed, 45 insertions(+), 43 deletions(-)

diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReadableNativeMap.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReadableNativeMap.java
index 0b787014524b06..3dae1f0ba165d8 100644
--- a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReadableNativeMap.java
+++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReadableNativeMap.java
@@ -39,15 +39,21 @@ public static int getJNIPassCounter() {
     return mJniCallCounter;
   }
 
-  private HashMap<String, Object> getLocalMap() {
-    if (mLocalMap != null) {
-      return mLocalMap;
-    }
+  private void ensureKeysAreImported() {
     synchronized (this) {
       if (mKeys == null) {
         mKeys = Assertions.assertNotNull(importKeys());
         mJniCallCounter++;
       }
+    }
+  }
+
+  private HashMap<String, Object> getLocalMap() {
+    if (mLocalMap != null) {
+      return mLocalMap;
+    }
+    ensureKeysAreImported();
+    synchronized (this) {
       if (mLocalMap == null) {
         Object[] values = Assertions.assertNotNull(importValues());
         mJniCallCounter++;
@@ -69,11 +75,8 @@ private HashMap<String, Object> getLocalMap() {
     if (mLocalTypeMap != null) {
       return mLocalTypeMap;
     }
+    ensureKeysAreImported();
     synchronized (this) {
-      if (mKeys == null) {
-        mKeys = Assertions.assertNotNull(importKeys());
-        mJniCallCounter++;
-      }
       // check that no other thread has already updated
       if (mLocalTypeMap == null) {
         Object[] types = Assertions.assertNotNull(importTypes());
@@ -187,48 +190,47 @@ public int getInt(@NonNull String name) {
 
   @Override
   public @NonNull Iterator<Map.Entry<String, Object>> getEntryIterator() {
-    if (mKeys == null) {
-      mKeys = Assertions.assertNotNull(importKeys());
-    }
+    ensureKeysAreImported();
     final String[] iteratorKeys = mKeys;
-    final Object[] iteratorValues = Assertions.assertNotNull(importValues());
-    return new Iterator<Map.Entry<String, Object>>() {
-      int currentIndex = 0;
+    synchronized (this) {
+      final Object[] iteratorValues = Assertions.assertNotNull(importValues());
 
-      @Override
-      public boolean hasNext() {
-        return currentIndex < iteratorKeys.length;
-      }
+      return new Iterator<Map.Entry<String, Object>>() {
+        int currentIndex = 0;
 
-      @Override
-      public Map.Entry<String, Object> next() {
-        final int index = currentIndex++;
-        return new Map.Entry<String, Object>() {
-          @Override
-          public String getKey() {
-            return iteratorKeys[index];
-          }
-
-          @Override
-          public Object getValue() {
-            return iteratorValues[index];
-          }
-
-          @Override
-          public Object setValue(Object value) {
-            throw new UnsupportedOperationException(
-                "Can't set a value while iterating over a ReadableNativeMap");
-          }
-        };
-      }
-    };
+        @Override
+        public boolean hasNext() {
+          return currentIndex < iteratorKeys.length;
+        }
+
+        @Override
+        public Map.Entry<String, Object> next() {
+          final int index = currentIndex++;
+          return new Map.Entry<String, Object>() {
+            @Override
+            public String getKey() {
+              return iteratorKeys[index];
+            }
+
+            @Override
+            public Object getValue() {
+              return iteratorValues[index];
+            }
+
+            @Override
+            public Object setValue(Object value) {
+              throw new UnsupportedOperationException(
+                  "Can't set a value while iterating over a ReadableNativeMap");
+            }
+          };
+        }
+      };
+    }
   }
 
   @Override
   public @NonNull ReadableMapKeySetIterator keySetIterator() {
-    if (mKeys == null) {
-      mKeys = Assertions.assertNotNull(importKeys());
-    }
+    ensureKeysAreImported();
     final String[] iteratorKeys = mKeys;
     return new ReadableMapKeySetIterator() {
       int currentIndex = 0;