From 3b088c3383e360126e46d7c4449aa7286a1a52e3 Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Wed, 10 May 2023 22:52:48 -0700 Subject: [PATCH] Fix use-after-free if JNI Yoga nodes are garbage collected using multiple threads (#1279) 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 --- java/com/facebook/yoga/YogaNative.java | 2 +- java/com/facebook/yoga/YogaNodeJNIFinalizer.java | 2 +- java/jni/YGJNIVanilla.cpp | 7 +++++-- yoga/Yoga-internal.h | 4 ++++ yoga/Yoga.cpp | 4 ++++ 5 files changed, 15 insertions(+), 4 deletions(-) diff --git a/java/com/facebook/yoga/YogaNative.java b/java/com/facebook/yoga/YogaNative.java index a48184db..74f4a98c 100644 --- a/java/com/facebook/yoga/YogaNative.java +++ b/java/com/facebook/yoga/YogaNative.java @@ -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); diff --git a/java/com/facebook/yoga/YogaNodeJNIFinalizer.java b/java/com/facebook/yoga/YogaNodeJNIFinalizer.java index 424006e9..ab617c7f 100644 --- a/java/com/facebook/yoga/YogaNodeJNIFinalizer.java +++ b/java/com/facebook/yoga/YogaNodeJNIFinalizer.java @@ -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); } } } diff --git a/java/jni/YGJNIVanilla.cpp b/java/jni/YGJNIVanilla.cpp index e32f14b1..c4dedb85 100644 --- a/java/jni/YGJNIVanilla.cpp +++ b/java/jni/YGJNIVanilla.cpp @@ -199,7 +199,10 @@ static void jni_YGConfigSetLoggerJNI( } } -static void jni_YGNodeFreeJNI(JNIEnv* env, jobject obj, jlong nativePointer) { +static void jni_YGNodeDeallocateJNI( + JNIEnv* env, + jobject obj, + jlong nativePointer) { if (nativePointer == 0) { return; } @@ -769,7 +772,7 @@ static JNINativeMethod methods[] = { (void*) jni_YGConfigSetLoggerJNI}, {"jni_YGNodeNewJNI", "()J", (void*) jni_YGNodeNewJNI}, {"jni_YGNodeNewWithConfigJNI", "(J)J", (void*) jni_YGNodeNewWithConfigJNI}, - {"jni_YGNodeFreeJNI", "(J)V", (void*) jni_YGNodeFreeJNI}, + {"jni_YGNodeDeallocateJNI", "(J)V", (void*) jni_YGNodeDeallocateJNI}, {"jni_YGNodeResetJNI", "(J)V", (void*) jni_YGNodeResetJNI}, {"jni_YGNodeInsertChildJNI", "(JJI)V", (void*) jni_YGNodeInsertChildJNI}, {"jni_YGNodeSwapChildJNI", "(JJI)V", (void*) jni_YGNodeSwapChildJNI}, diff --git a/yoga/Yoga-internal.h b/yoga/Yoga-internal.h index 6f60e4e1..5c5f11c7 100644 --- a/yoga/Yoga-internal.h +++ b/yoga/Yoga-internal.h @@ -27,6 +27,10 @@ void YGNodeCalculateLayoutWithContext( YGDirection ownerDirection, void* layoutContext); +// Deallocates a Yoga Node. Unlike YGNodeFree, does not remove the node from +// its parent or children. +void YGNodeDeallocate(YGNodeRef node); + YG_EXTERN_C_END namespace facebook { diff --git a/yoga/Yoga.cpp b/yoga/Yoga.cpp index 8e2fdb4d..bf30d41c 100644 --- a/yoga/Yoga.cpp +++ b/yoga/Yoga.cpp @@ -230,6 +230,10 @@ YOGA_EXPORT void YGNodeFree(const YGNodeRef node) { } node->clearChildren(); + YGNodeDeallocate(node); +} + +YOGA_EXPORT void YGNodeDeallocate(const YGNodeRef node) { Event::publish(node, {node->getConfig()}); delete node; }