From 9aac13d4dc95925b57f03e7964fc7add6834e518 Mon Sep 17 00:00:00 2001 From: Ruslan Shestopalyuk 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 getLocalMap() { - if (mLocalMap != null) { - return mLocalMap; - } + private void ensureKeysAreImported() { synchronized (this) { if (mKeys == null) { mKeys = Assertions.assertNotNull(importKeys()); mJniCallCounter++; } + } + } + + private HashMap getLocalMap() { + if (mLocalMap != null) { + return mLocalMap; + } + ensureKeysAreImported(); + synchronized (this) { if (mLocalMap == null) { Object[] values = Assertions.assertNotNull(importValues()); mJniCallCounter++; @@ -69,11 +75,8 @@ private HashMap 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> getEntryIterator() { - if (mKeys == null) { - mKeys = Assertions.assertNotNull(importKeys()); - } + ensureKeysAreImported(); final String[] iteratorKeys = mKeys; - final Object[] iteratorValues = Assertions.assertNotNull(importValues()); - return new Iterator>() { - int currentIndex = 0; + synchronized (this) { + final Object[] iteratorValues = Assertions.assertNotNull(importValues()); - @Override - public boolean hasNext() { - return currentIndex < iteratorKeys.length; - } + return new Iterator>() { + int currentIndex = 0; - @Override - public Map.Entry next() { - final int index = currentIndex++; - return new Map.Entry() { - @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 next() { + final int index = currentIndex++; + return new Map.Entry() { + @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;