Skip to content

Commit

Permalink
Fix Use After Free With Concurrent Java GC
Browse files Browse the repository at this point in the history
Summary:
Fixes facebook/yoga#1271

Java bindings for Yoga rely solely on garbage collection for memory management. Each Java `YogaNode` has references to its children and parent, along with an `YGNodeRef` which it owns. 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 point to trying to remove dangling pointers, and this bit of behavior is counterintuitive from the API name anyway. This diff makes a breaking change to `YGNodeFree` to instead mostly just be a `free()` (with a tracing step whose publishing should be thread safe). The existing behavior is exposed in an added function `YGNodeDetachAndFree()`.

The majority of existing `YGNodeFree` usages will probably still work under the new behavior, but we don't have that many in fbsource (many instead call `YGNodeFreeRecursive` to free a whole tree at once). So for safety, I replaced existing usages of `YGNodeFree` outside of Yoga with `YGNodeDetachAndFree` to preserve existing behavior.

JavaScript and C# bindings also use `YGNodeFree()`, and both allow explicit freeing. For now I switched both of their behavior to detachment, to avoid exposing the ability to managed languages to corrupt memory.

Differential Revision: D45556206

fbshipit-source-id: 0d1f52a4195c169c5ab43f6f417e22ce1f998d8c
  • Loading branch information
NickGerleman authored and facebook-github-bot committed May 4, 2023
1 parent 0d19b4c commit b7f710f
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 0 deletions.
5 changes: 5 additions & 0 deletions lib/yoga/src/main/cpp/yoga/Yoga.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,11 @@ YOGA_EXPORT YGNodeRef YGNodeClone(YGNodeRef oldNode) {
}

YOGA_EXPORT void YGNodeFree(const YGNodeRef node) {
Event::publish<Event::NodeDeallocation>(node, {node->getConfig()});
delete node;
}

YOGA_EXPORT void YGNodeDetachAndFree(const YGNodeRef node) {
if (YGNodeRef owner = node->getOwner()) {
owner->removeChild(node);
node->setOwner(nullptr);
Expand Down
1 change: 1 addition & 0 deletions lib/yoga/src/main/cpp/yoga/Yoga.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ WIN_EXPORT YGNodeRef YGNodeNew(void);
WIN_EXPORT YGNodeRef YGNodeNewWithConfig(YGConfigRef config);
WIN_EXPORT YGNodeRef YGNodeClone(YGNodeRef node);
WIN_EXPORT void YGNodeFree(YGNodeRef node);
WIN_EXPORT void YGNodeDeatchAndFree(YGNodeRef node);
WIN_EXPORT void YGNodeFreeRecursiveWithCleanupFunc(
YGNodeRef node,
YGNodeCleanupFunc cleanup);
Expand Down

0 comments on commit b7f710f

Please sign in to comment.