Skip to content

Commit

Permalink
Fix race condition in ReadableNativeMap (#36201)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: #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
  • Loading branch information
rshest authored and kelset committed Mar 28, 2023
1 parent a413e3e commit 0d3c70b
Showing 1 changed file with 45 additions and 43 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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++;
Expand All @@ -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());
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit 0d3c70b

Please sign in to comment.