Fix Use After Free With Concurrent Java GC #1279

2023-05-03T20:46:49-07:00

NickGerleman commented

2023-05-03 20:46:49 -07:00

(Migrated from github.com)

Summary:
Fixes https://github.com/facebook/yoga/issues/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.

This bit of behavior is counterintuitive from the API name anyway, so 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

Summary: Fixes https://github.com/facebook/yoga/issues/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. This bit of behavior is counterintuitive from the API name anyway, so 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

facebook-github-bot commented

2023-05-03 20:47:52 -07:00

(Migrated from github.com)