Skip to content

Commit d8c933c

Browse files
committed
[ASDisplayNode] Fix a crash in insertSubnode
If a node is already a subnode to a supernode, Inserting it again can lead to a crash. Here is a simple repro of the crash: ``` ASDisplayNode *subnode = [[ASDisplayNode alloc] init]; ASDisplayNode *supernode = [[ASDisplayNode alloc] init]; [supernode addSubnode:subnode]; // Crash on next line [supernode insertSubnode:subnode atIndex:1]; ``` The issue is that all the checks around subnode array boundaries are done BEFORE `subnode` is removed from its `supernode`. If it happens that the `supernode` is self, then removing the `subnode` causes all our index checks to no longer be valid. Here is the relevant code: ``` __instanceLock__.lock(); NSUInteger subnodesCount = _subnodes.count; __instanceLock__.unlock(); ////// Here we check our indexes if (subnodeIndex > subnodesCount || subnodeIndex < 0) { ASDisplayNodeFailAssert(@"Cannot insert a subnode at index %ld. Count is %ld", (long)subnodeIndex, (long)subnodesCount); return; } … ///////// Here our indexes could invalidate if self subnode’s supernode [subnode removeFromSupernode]; [oldSubnode removeFromSupernode]; __instanceLock__.lock(); if (_subnodes == nil) { _subnodes = [[NSMutableArray alloc] init]; } ////// Here would can crash if our index is too big [_subnodes insertObject:subnode atIndex:subnodeIndex]; _cachedSubnodes = nil; __instanceLock__.unlock(); ```
1 parent 83c6ff8 commit d8c933c

File tree

2 files changed

+16
-0
lines changed

2 files changed

+16
-0
lines changed

Source/ASDisplayNode.mm

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2104,6 +2104,11 @@ - (void)_insertSubnode:(ASDisplayNode *)subnode atSubnodeIndex:(NSInteger)subnod
21042104
ASDisplayNodeFailAssert(@"Cannot add a view-backed node as a subnode of a layer-backed node. Supernode: %@, subnode: %@", self, subnode);
21052105
return;
21062106
}
2107+
2108+
if (subnode.supernode == self) {
2109+
ASDisplayNodeFailAssert(@"`subnode` %@ is already a subnode to `supernode` %@. You may be mixing addSubnode and ASM", subnode, self);
2110+
return;
2111+
}
21072112

21082113
BOOL isRasterized = subtreeIsRasterized(self);
21092114
if (isRasterized && subnode.nodeLoaded) {

Tests/ASDisplayNodeTests.mm

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2783,4 +2783,15 @@ - (void)testPlaceholder
27832783
XCTAssertTrue(hasPlaceholderLayer);
27842784
}
27852785

2786+
- (void)testInsertExistingSubnode
2787+
{
2788+
ASDisplayNode *subnode = [[ASDisplayNode alloc] init];
2789+
ASDisplayNode *supernode = [[ASDisplayNode alloc] init];
2790+
2791+
[supernode addSubnode:subnode];
2792+
// Previously this would cause a crash. We now protect inserting an already existing subnode
2793+
XCTAssertThrows([supernode insertSubnode:subnode atIndex:1]);
2794+
XCTAssertTrue(supernode.subnodes.count == 1);
2795+
}
2796+
27862797
@end

0 commit comments

Comments
 (0)