Skip to content

Commit

Permalink
Fix race condition in ReadableNativeMap
Browse files Browse the repository at this point in the history
Summary:
[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: 5ea3301857517d99a9007ddf2739e49ca8136743
  • Loading branch information
rshest authored and facebook-github-bot committed Feb 17, 2023
1 parent ed8a3e0 commit 7948343
Showing 1 changed file with 10 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -188,10 +188,15 @@ public int getInt(@NonNull String name) {
@Override
public @NonNull Iterator<Map.Entry<String, Object>> getEntryIterator() {
if (mKeys == null) {
mKeys = Assertions.assertNotNull(importKeys());
synchronized (this) {
mKeys = Assertions.assertNotNull(importKeys());
}
}
final String[] iteratorKeys = mKeys;
final Object[] iteratorValues = Assertions.assertNotNull(importValues());
Object[] iteratorValues;
synchronized (this) {
iteratorValues = Assertions.assertNotNull(importValues());
}
return new Iterator<Map.Entry<String, Object>>() {
int currentIndex = 0;

Expand Down Expand Up @@ -227,7 +232,9 @@ public Object setValue(Object value) {
@Override
public @NonNull ReadableMapKeySetIterator keySetIterator() {
if (mKeys == null) {
mKeys = Assertions.assertNotNull(importKeys());
synchronized (this) {
mKeys = Assertions.assertNotNull(importKeys());
}
}
final String[] iteratorKeys = mKeys;
return new ReadableMapKeySetIterator() {
Expand Down

0 comments on commit 7948343

Please sign in to comment.