Skip to content

Commit 230b85a

Browse files
Abbondanzometa-codesync[bot]
Abbondanzo
authored and
meta-codesync[bot]
committed
Fix undefined behavior in LayoutAnimation sort comparator (#56127)
Summary: Pull Request resolved: #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
1 parent 540120a commit 230b85a

File tree

2 files changed

+285
-1
lines changed

2 files changed

+285
-1
lines changed

‎packages/react-native/ReactCommon/react/renderer/animations/tests/MutationComparatorTest.cpp‎

Lines changed: 273 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,273 @@
1+
/*
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*/
7+
8+
#include <algorithm>
9+
#include <vector>
10+
11+
#include <gtest/gtest.h>
12+
13+
#include <react/renderer/animations/utils.h>
14+
#include <react/renderer/mounting/ShadowViewMutation.h>
15+
16+
namespace facebook::react {
17+
18+
namespace {
19+
20+
ShadowView makeShadowView(Tag tag) {
21+
ShadowView sv{};
22+
sv.tag = tag;
23+
return sv;
24+
}
25+
26+
} // namespace
27+
28+
// Verify strict weak ordering: irreflexivity
29+
// comp(a, a) must be false
30+
TEST(MutationComparatorTest, Irreflexivity) {
31+
auto sv1 = makeShadowView(1);
32+
auto sv2 = makeShadowView(2);
33+
34+
// Same-type mutations where the fallback path is exercised
35+
auto update1 = ShadowViewMutation::UpdateMutation(sv1, sv2, /*parentTag=*/10);
36+
EXPECT_FALSE(shouldFirstComeBeforeSecondMutation(update1, update1));
37+
38+
auto create1 = ShadowViewMutation::CreateMutation(sv1);
39+
EXPECT_FALSE(shouldFirstComeBeforeSecondMutation(create1, create1));
40+
41+
auto insert1 =
42+
ShadowViewMutation::InsertMutation(/*parentTag=*/10, sv1, /*index=*/0);
43+
EXPECT_FALSE(shouldFirstComeBeforeSecondMutation(insert1, insert1));
44+
45+
auto remove1 =
46+
ShadowViewMutation::RemoveMutation(/*parentTag=*/10, sv1, /*index=*/0);
47+
EXPECT_FALSE(shouldFirstComeBeforeSecondMutation(remove1, remove1));
48+
49+
auto del1 = ShadowViewMutation::DeleteMutation(sv1);
50+
EXPECT_FALSE(shouldFirstComeBeforeSecondMutation(del1, del1));
51+
}
52+
53+
// Verify strict weak ordering: asymmetry
54+
// If comp(a, b) then !comp(b, a)
55+
TEST(MutationComparatorTest, Asymmetry) {
56+
auto sv1 = makeShadowView(1);
57+
auto sv2 = makeShadowView(2);
58+
auto sv3 = makeShadowView(3);
59+
60+
// Two updates with same type but different parentTags (fallback path)
61+
auto update1 = ShadowViewMutation::UpdateMutation(sv1, sv2, /*parentTag=*/10);
62+
auto update2 = ShadowViewMutation::UpdateMutation(sv1, sv3, /*parentTag=*/20);
63+
64+
bool a_before_b = shouldFirstComeBeforeSecondMutation(update1, update2);
65+
bool b_before_a = shouldFirstComeBeforeSecondMutation(update2, update1);
66+
67+
// Exactly one must be true (asymmetry)
68+
EXPECT_NE(a_before_b, b_before_a);
69+
}
70+
71+
// Verify strict weak ordering: transitivity
72+
// If comp(a, b) and comp(b, c) then comp(a, c)
73+
TEST(MutationComparatorTest, Transitivity) {
74+
auto sv1 = makeShadowView(1);
75+
auto sv2 = makeShadowView(2);
76+
auto sv3 = makeShadowView(3);
77+
78+
auto update1 = ShadowViewMutation::UpdateMutation(sv1, sv1, /*parentTag=*/10);
79+
auto update2 = ShadowViewMutation::UpdateMutation(sv1, sv1, /*parentTag=*/20);
80+
auto update3 = ShadowViewMutation::UpdateMutation(sv1, sv1, /*parentTag=*/30);
81+
82+
bool a_b = shouldFirstComeBeforeSecondMutation(update1, update2);
83+
bool b_c = shouldFirstComeBeforeSecondMutation(update2, update3);
84+
bool a_c = shouldFirstComeBeforeSecondMutation(update1, update3);
85+
86+
if (a_b && b_c) {
87+
EXPECT_TRUE(a_c) << "Transitivity violated: a<b and b<c but not a<c";
88+
}
89+
}
90+
91+
// Verify type-based ordering rules
92+
TEST(MutationComparatorTest, TypeOrdering) {
93+
auto sv1 = makeShadowView(1);
94+
auto sv2 = makeShadowView(2);
95+
96+
auto create = ShadowViewMutation::CreateMutation(sv1);
97+
auto insert =
98+
ShadowViewMutation::InsertMutation(/*parentTag=*/10, sv1, /*index=*/0);
99+
auto remove =
100+
ShadowViewMutation::RemoveMutation(/*parentTag=*/10, sv1, /*index=*/0);
101+
auto update = ShadowViewMutation::UpdateMutation(sv1, sv2, /*parentTag=*/10);
102+
auto del = ShadowViewMutation::DeleteMutation(sv1);
103+
104+
// Delete always comes last
105+
EXPECT_FALSE(shouldFirstComeBeforeSecondMutation(del, create));
106+
EXPECT_FALSE(shouldFirstComeBeforeSecondMutation(del, insert));
107+
EXPECT_FALSE(shouldFirstComeBeforeSecondMutation(del, remove));
108+
EXPECT_TRUE(shouldFirstComeBeforeSecondMutation(create, del));
109+
EXPECT_TRUE(shouldFirstComeBeforeSecondMutation(insert, del));
110+
EXPECT_TRUE(shouldFirstComeBeforeSecondMutation(remove, del));
111+
112+
// Remove comes before Insert
113+
EXPECT_TRUE(shouldFirstComeBeforeSecondMutation(remove, insert));
114+
EXPECT_FALSE(shouldFirstComeBeforeSecondMutation(insert, remove));
115+
116+
// Create comes before Insert
117+
EXPECT_TRUE(shouldFirstComeBeforeSecondMutation(create, insert));
118+
EXPECT_FALSE(shouldFirstComeBeforeSecondMutation(insert, create));
119+
120+
// Remove comes before Update
121+
EXPECT_TRUE(shouldFirstComeBeforeSecondMutation(remove, update));
122+
EXPECT_FALSE(shouldFirstComeBeforeSecondMutation(update, remove));
123+
}
124+
125+
// Verify removes on same parent are sorted by descending index
126+
TEST(MutationComparatorTest, RemovesSameParentDescendingIndex) {
127+
auto sv1 = makeShadowView(1);
128+
auto sv2 = makeShadowView(2);
129+
130+
auto remove_idx0 =
131+
ShadowViewMutation::RemoveMutation(/*parentTag=*/10, sv1, /*index=*/0);
132+
auto remove_idx5 =
133+
ShadowViewMutation::RemoveMutation(/*parentTag=*/10, sv2, /*index=*/5);
134+
135+
// Higher index should come first
136+
EXPECT_TRUE(shouldFirstComeBeforeSecondMutation(remove_idx5, remove_idx0));
137+
EXPECT_FALSE(shouldFirstComeBeforeSecondMutation(remove_idx0, remove_idx5));
138+
}
139+
140+
// Verify the deterministic fallback uses parentTag, then child tags
141+
TEST(MutationComparatorTest, DeterministicFallbackByParentTag) {
142+
auto sv1 = makeShadowView(1);
143+
auto sv2 = makeShadowView(2);
144+
145+
// Two updates with same type, different parentTags
146+
auto update_p10 =
147+
ShadowViewMutation::UpdateMutation(sv1, sv2, /*parentTag=*/10);
148+
auto update_p20 =
149+
ShadowViewMutation::UpdateMutation(sv1, sv2, /*parentTag=*/20);
150+
151+
// Lower parentTag comes first
152+
EXPECT_TRUE(shouldFirstComeBeforeSecondMutation(update_p10, update_p20));
153+
EXPECT_FALSE(shouldFirstComeBeforeSecondMutation(update_p20, update_p10));
154+
}
155+
156+
TEST(MutationComparatorTest, DeterministicFallbackByNewChildTag) {
157+
auto sv1 = makeShadowView(1);
158+
auto sv2 = makeShadowView(2);
159+
auto sv3 = makeShadowView(3);
160+
161+
// Same parentTag, different newChildShadowView tags
162+
auto update_new2 =
163+
ShadowViewMutation::UpdateMutation(sv1, sv2, /*parentTag=*/10);
164+
auto update_new3 =
165+
ShadowViewMutation::UpdateMutation(sv1, sv3, /*parentTag=*/10);
166+
167+
EXPECT_TRUE(shouldFirstComeBeforeSecondMutation(update_new2, update_new3));
168+
EXPECT_FALSE(shouldFirstComeBeforeSecondMutation(update_new3, update_new2));
169+
}
170+
171+
TEST(MutationComparatorTest, DeterministicFallbackByOldChildTag) {
172+
auto sv1 = makeShadowView(1);
173+
auto sv2 = makeShadowView(2);
174+
auto sv3 = makeShadowView(3);
175+
176+
// Same parentTag, same newChildShadowView tag, different oldChildShadowView
177+
// tags
178+
auto update_old1 =
179+
ShadowViewMutation::UpdateMutation(sv1, sv3, /*parentTag=*/10);
180+
auto update_old2 =
181+
ShadowViewMutation::UpdateMutation(sv2, sv3, /*parentTag=*/10);
182+
183+
EXPECT_TRUE(shouldFirstComeBeforeSecondMutation(update_old1, update_old2));
184+
EXPECT_FALSE(shouldFirstComeBeforeSecondMutation(update_old2, update_old1));
185+
}
186+
187+
// Verify equal mutations return false (stability)
188+
TEST(MutationComparatorTest, EqualMutationsReturnFalse) {
189+
auto sv1 = makeShadowView(1);
190+
auto sv2 = makeShadowView(2);
191+
192+
auto update_a =
193+
ShadowViewMutation::UpdateMutation(sv1, sv2, /*parentTag=*/10);
194+
auto update_b =
195+
ShadowViewMutation::UpdateMutation(sv1, sv2, /*parentTag=*/10);
196+
197+
// Two mutations with identical properties should return false (not less-than)
198+
EXPECT_FALSE(shouldFirstComeBeforeSecondMutation(update_a, update_b));
199+
EXPECT_FALSE(shouldFirstComeBeforeSecondMutation(update_b, update_a));
200+
}
201+
202+
// Verify std::stable_sort doesn't crash with the comparator
203+
// (the original bug was a SIGSEGV during sort)
204+
TEST(MutationComparatorTest, StableSortDoesNotCrash) {
205+
ShadowViewMutation::List mutations;
206+
207+
// Build a list with various mutation types and properties
208+
for (int parent = 0; parent < 5; parent++) {
209+
for (int child = 0; child < 10; child++) {
210+
auto sv_old = makeShadowView(child);
211+
auto sv_new = makeShadowView(child + 100);
212+
213+
mutations.push_back(
214+
ShadowViewMutation::UpdateMutation(
215+
sv_old, sv_new, /*parentTag=*/parent));
216+
mutations.push_back(
217+
ShadowViewMutation::InsertMutation(
218+
/*parentTag=*/parent, sv_new, /*index=*/child));
219+
mutations.push_back(
220+
ShadowViewMutation::RemoveMutation(
221+
/*parentTag=*/parent, sv_old, /*index=*/child));
222+
}
223+
}
224+
mutations.push_back(ShadowViewMutation::CreateMutation(makeShadowView(999)));
225+
mutations.push_back(ShadowViewMutation::DeleteMutation(makeShadowView(998)));
226+
227+
// This should not crash (the original bug was SIGSEGV here)
228+
EXPECT_NO_FATAL_FAILURE(
229+
std::stable_sort(
230+
mutations.begin(),
231+
mutations.end(),
232+
&shouldFirstComeBeforeSecondMutation));
233+
234+
// Verify ordering invariants after sort
235+
bool seen_delete = false;
236+
for (const auto& m : mutations) {
237+
if (m.type == ShadowViewMutation::Type::Delete) {
238+
seen_delete = true;
239+
} else if (seen_delete) {
240+
FAIL() << "Non-delete mutation found after a delete mutation";
241+
}
242+
}
243+
}
244+
245+
// Stress test: sort a large list of mutations with duplicates
246+
TEST(MutationComparatorTest, StableSortLargeListWithDuplicates) {
247+
ShadowViewMutation::List mutations;
248+
249+
// Create many mutations with overlapping properties to stress the comparator
250+
for (int i = 0; i < 200; i++) {
251+
auto sv = makeShadowView(i % 10); // Deliberately create duplicates
252+
auto sv2 = makeShadowView((i + 1) % 10);
253+
int parent = i % 5;
254+
255+
mutations.push_back(ShadowViewMutation::UpdateMutation(sv, sv2, parent));
256+
if (i % 3 == 0) {
257+
mutations.push_back(
258+
ShadowViewMutation::InsertMutation(parent, sv, i % 20));
259+
}
260+
if (i % 4 == 0) {
261+
mutations.push_back(
262+
ShadowViewMutation::RemoveMutation(parent, sv, i % 20));
263+
}
264+
}
265+
266+
EXPECT_NO_FATAL_FAILURE(
267+
std::stable_sort(
268+
mutations.begin(),
269+
mutations.end(),
270+
&shouldFirstComeBeforeSecondMutation));
271+
}
272+
273+
} // namespace facebook::react

‎packages/react-native/ReactCommon/react/renderer/animations/utils.h‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,18 @@ static inline bool shouldFirstComeBeforeSecondMutation(
9696
}
9797
}
9898

99-
return &lhs < &rhs;
99+
// Deterministic fallback: when no type-specific rule applies, order by
100+
// mutation properties to satisfy strict weak ordering.
101+
if (lhs.parentTag != rhs.parentTag) {
102+
return lhs.parentTag < rhs.parentTag;
103+
}
104+
if (lhs.newChildShadowView.tag != rhs.newChildShadowView.tag) {
105+
return lhs.newChildShadowView.tag < rhs.newChildShadowView.tag;
106+
}
107+
if (lhs.oldChildShadowView.tag != rhs.oldChildShadowView.tag) {
108+
return lhs.oldChildShadowView.tag < rhs.oldChildShadowView.tag;
109+
}
110+
return false;
100111
}
101112

102113
std::pair<Float, Float>

0 commit comments

Comments
 (0)