DaddyFrosty/yoga
1
0
Fork 0
Code Issues 96 Pull Requests 31 Actions Packages Projects Releases 33 Wiki Activity

Fix use-after-free if JNI Yoga nodes are garbage collected using multiple threads (#1279)

Browse Source 
Summary:
X-link: https://github.com/facebook/react-native/pull/37243

X-link: https://github.com/facebook/litho/pull/944

Pull Request resolved: https://github.com/facebook/yoga/pull/1279

Java bindings for Yoga rely solely on garbage collection for memory management. Each Java `YogaNode` has references to its children and parent Java Nodes. This means, for a node to be garbage collected, it cannot be reachable from any user accessible node. Each node then has single ownership of a `YGNodeRef`. When the `YogaNode` is garbage collected, a finalizer is run to call `YGNodeFree` and free the underlying native Yoga Node.

This may cause a use-after-free if finalizers are run from multiple threads. This is because `YGNodeFree` does more than just freeing, but instead also interacts with its parent and children nodes to detach itself, and remove any dangling pointers. If multiple threads run finalizers at once, one may traverse and try to mutate a node which another is freeing.

Because we know the entire connected tree is dead, there is no need to remove dangling pointers, so I want to expose a way to just free a Yoga Node, without it mutating the tree as a side effect.

This adds a currently private `YGNodeDeallocate` that frees without traversal. Ideally from naming this is what `YGNodeFree` would do, but we think changing the behavior of that might be too disruptive to OSS. At the same time there may be other memory safety related API changes we would like to eventually make, so this isn't made public beyond the JNI bindings to prevent needing to transition more APIs.

Changelog: [Internal]

Reviewed By: rshest

Differential Revision: D45556206

fbshipit-source-id: 62a1394c6f6bdc2b437b388098ea362a0fbcd0f7
This commit is contained in:
Nick Gerleman
2023-05-10 22:52:48 -07:00
committed by Facebook GitHub Bot
parent 149849d22b
commit 3b088c3383
5 changed files with 15 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
java/com/facebook/yoga/YogaNative.java
View File

@@ -32,7 +32,7 @@ public class YogaNative {
// YGNode related
static native long jni_YGNodeNewJNI();
static native long jni_YGNodeNewWithConfigJNI(long configPointer);
static native void jni_YGNodeFreeJNI(long nativePointer);
static native void jni_YGNodeDeallocateJNI(long nativePointer);
static native void jni_YGNodeResetJNI(long nativePointer);
static native void jni_YGNodeInsertChildJNI(long nativePointer, long childPointer, int index);
static native void jni_YGNodeSwapChildJNI(long nativePointer, long childPointer, int index);

2
java/com/facebook/yoga/YogaNodeJNIFinalizer.java
View File

@@ -29,7 +29,7 @@ public class YogaNodeJNIFinalizer extends YogaNodeJNIBase {
if (mNativePointer != 0) {
long nativePointer = mNativePointer;
mNativePointer = 0;
YogaNative.jni_YGNodeFreeJNI(nativePointer);
YogaNative.jni_YGNodeDeallocateJNI(nativePointer);
}
}
}