Skip to content

Fix undefined behavior in LayoutAnimation sort comparator (#56127)#56127

Closed
Abbondanzo wants to merge 1 commit intofacebook:mainfrom
Abbondanzo:export-D95909371
Closed

Fix undefined behavior in LayoutAnimation sort comparator (#56127)#56127
Abbondanzo wants to merge 1 commit intofacebook:mainfrom
Abbondanzo:export-D95909371

Conversation

@Abbondanzo
Copy link
Contributor

@Abbondanzo Abbondanzo commented Mar 17, 2026

Summary:

shouldFirstComeBeforeSecondMutation in utils.h uses return &lhs < &rhs as a fallback when mutation types don't determine ordering. That's undefined behavior per C++ standard [expr.rel]/4, comparing pointers from different allocations (which happens during std::stable_sort's merge phase).

This wasn't a problem until the LLVM 19.x NDK upgrade changed libc++'s sort implementation. The new insertion sort phase is more aggressive about exploiting comparator correctness, and the UB now manifests as SIGSEGV in the insertion sort phase on Android.

The fix replaces the pointer comparison with a deterministic fallback using parentTag, newChildShadowView.tag, and oldChildShadowView.tag. When all properties are equal, returns false to maintain stable ordering.

Also adds MutationComparatorTest.cpp with tests covering:

  • Strict weak ordering properties (irreflexivity, asymmetry, transitivity)
  • Type-based ordering rules (deletes last, removes before inserts, etc.)
  • Deterministic fallback by parentTag, newChildTag, oldChildTag
  • Equal mutations return false (stability)
  • Stress test: std::stable_sort on large mutation lists with duplicates

Changelog: [Internal]

Reviewed By: christophpurrer

Differential Revision: D95909371

@meta-codesync
Copy link

meta-codesync bot commented Mar 17, 2026

@Abbondanzo has exported this pull request. If you are a Meta employee, you can view the originating Diff in D95909371.

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 17, 2026
@meta-codesync meta-codesync bot changed the title Fix undefined behavior in LayoutAnimation sort comparator Fix undefined behavior in LayoutAnimation sort comparator (#56127) Mar 19, 2026
Abbondanzo added a commit to Abbondanzo/react-native that referenced this pull request Mar 19, 2026
…6127)

Summary:

`shouldFirstComeBeforeSecondMutation` in `utils.h` uses `return &lhs < &rhs` as a fallback when mutation types don't determine ordering. That's undefined behavior per C++ standard [expr.rel]/4, comparing pointers from different allocations (which happens during `std::stable_sort`'s merge phase).

This wasn't a problem until the LLVM 19.x NDK upgrade changed libc++'s sort implementation. The new insertion sort phase is more aggressive about exploiting comparator correctness, and the UB now manifests as SIGSEGV in the insertion sort phase on Android.

The fix replaces the pointer comparison with a deterministic fallback using `parentTag`, `newChildShadowView.tag`, and `oldChildShadowView.tag`. When all properties are equal, returns `false` to maintain stable ordering.

Also adds `MutationComparatorTest.cpp` with tests covering:
- Strict weak ordering properties (irreflexivity, asymmetry, transitivity)
- Type-based ordering rules (deletes last, removes before inserts, etc.)
- Deterministic fallback by parentTag, newChildTag, oldChildTag
- Equal mutations return false (stability)
- Stress test: `std::stable_sort` on large mutation lists with duplicates

Changelog: [Internal]

Reviewed By: christophpurrer

Differential Revision: D95909371
Abbondanzo added a commit to Abbondanzo/react-native that referenced this pull request Mar 19, 2026
…6127)

Summary:
Pull Request resolved: facebook#56127

`shouldFirstComeBeforeSecondMutation` in `utils.h` uses `return &lhs < &rhs` as a fallback when mutation types don't determine ordering. That's undefined behavior per C++ standard [expr.rel]/4, comparing pointers from different allocations (which happens during `std::stable_sort`'s merge phase).

This wasn't a problem until the LLVM 19.x NDK upgrade changed libc++'s sort implementation. The new insertion sort phase is more aggressive about exploiting comparator correctness, and the UB now manifests as SIGSEGV in the insertion sort phase on Android.

The fix replaces the pointer comparison with a deterministic fallback using `parentTag`, `newChildShadowView.tag`, and `oldChildShadowView.tag`. When all properties are equal, returns `false` to maintain stable ordering.

Also adds `MutationComparatorTest.cpp` with tests covering:
- Strict weak ordering properties (irreflexivity, asymmetry, transitivity)
- Type-based ordering rules (deletes last, removes before inserts, etc.)
- Deterministic fallback by parentTag, newChildTag, oldChildTag
- Equal mutations return false (stability)
- Stress test: `std::stable_sort` on large mutation lists with duplicates

Changelog: [Internal]

Reviewed By: christophpurrer

Differential Revision: D95909371
Abbondanzo added a commit to Abbondanzo/react-native that referenced this pull request Mar 19, 2026
…6127)

Summary:

`shouldFirstComeBeforeSecondMutation` in `utils.h` uses `return &lhs < &rhs` as a fallback when mutation types don't determine ordering. That's undefined behavior per C++ standard [expr.rel]/4, comparing pointers from different allocations (which happens during `std::stable_sort`'s merge phase).

This wasn't a problem until the LLVM 19.x NDK upgrade changed libc++'s sort implementation. The new insertion sort phase is more aggressive about exploiting comparator correctness, and the UB now manifests as SIGSEGV in the insertion sort phase on Android.

The fix replaces the pointer comparison with a deterministic fallback using `parentTag`, `newChildShadowView.tag`, and `oldChildShadowView.tag`. When all properties are equal, returns `false` to maintain stable ordering.

Also adds `MutationComparatorTest.cpp` with tests covering:
- Strict weak ordering properties (irreflexivity, asymmetry, transitivity)
- Type-based ordering rules (deletes last, removes before inserts, etc.)
- Deterministic fallback by parentTag, newChildTag, oldChildTag
- Equal mutations return false (stability)
- Stress test: `std::stable_sort` on large mutation lists with duplicates

Changelog: [Internal]

Reviewed By: christophpurrer

Differential Revision: D95909371
…6127)

Summary:
Pull Request resolved: facebook#56127

`shouldFirstComeBeforeSecondMutation` in `utils.h` uses `return &lhs < &rhs` as a fallback when mutation types don't determine ordering. That's undefined behavior per C++ standard [expr.rel]/4, comparing pointers from different allocations (which happens during `std::stable_sort`'s merge phase).

This wasn't a problem until the LLVM 19.x NDK upgrade changed libc++'s sort implementation. The new insertion sort phase is more aggressive about exploiting comparator correctness, and the UB now manifests as SIGSEGV in the insertion sort phase on Android.

The fix replaces the pointer comparison with a deterministic fallback using `parentTag`, `newChildShadowView.tag`, and `oldChildShadowView.tag`. When all properties are equal, returns `false` to maintain stable ordering.

Also adds `MutationComparatorTest.cpp` with tests covering:
- Strict weak ordering properties (irreflexivity, asymmetry, transitivity)
- Type-based ordering rules (deletes last, removes before inserts, etc.)
- Deterministic fallback by parentTag, newChildTag, oldChildTag
- Equal mutations return false (stability)
- Stress test: `std::stable_sort` on large mutation lists with duplicates

Changelog: [Internal]

Reviewed By: christophpurrer

Differential Revision: D95909371
@meta-codesync meta-codesync bot closed this in 1fdd167 Mar 19, 2026
@facebook-github-tools facebook-github-tools bot added the Merged This PR has been merged. label Mar 19, 2026
@meta-codesync
Copy link

meta-codesync bot commented Mar 19, 2026

This pull request has been merged in 1fdd167.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @Abbondanzo in 1fdd167

When will my fix make it into a release? | How to file a pick request?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged This PR has been merged. meta-exported p: Facebook Partner: Facebook Partner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants