Skip to content

Commit

Permalink
Guard against seg fault if replaceChild index is out of bounds (faceb…
Browse files Browse the repository at this point in the history
…ook#47644)

Summary:
X-link: facebook/yoga#1744


We are seeing some seg faults after the new display: contents logic was added. This is either because we are passing in an out of bounds index - in which case we try to read `display_` from protected memory. Or, the `Node *` was deleted at some point without removing it from this array. I think its the out of bounds issue mainly because I am not sure where this deletion would occur

I added an if to revert to the legacy, undefined behavior in this case. This is not ideal and we should find the root cause that is calling into this function improperly but for now it stops apps from crashing on the `replaceChild` call

Changelog: [Internal]

Reviewed By: rozele

Differential Revision: D66038645
  • Loading branch information
joevilches authored and facebook-github-bot committed Nov 16, 2024
1 parent 38fb83c commit 85a017d
Showing 1 changed file with 14 additions and 8 deletions.
22 changes: 14 additions & 8 deletions packages/react-native/ReactCommon/yoga/yoga/node/Node.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -117,14 +117,20 @@ void Node::setMeasureFunc(YGMeasureFunc measureFunc) {
}

void Node::replaceChild(Node* child, size_t index) {
auto previousChild = children_[index];
if (previousChild->style().display() == Display::Contents &&
child->style().display() != Display::Contents) {
contentsChildrenCount_--;
} else if (
previousChild->style().display() != Display::Contents &&
child->style().display() == Display::Contents) {
contentsChildrenCount_++;
// Without this conditional, if the index is out of bounds this will seg
// fault, so we are guarding against that here. Writing to this index
// afterwards is undefined behavior, and we ideally don't do that, but it is
// legacy behavior that we are keeping for now.
if (index < children_.size()) {
auto previousChild = children_[index];
if (previousChild->style().display() == Display::Contents &&
child->style().display() != Display::Contents) {
contentsChildrenCount_--;
} else if (
previousChild->style().display() != Display::Contents &&
child->style().display() == Display::Contents) {
contentsChildrenCount_++;
}
}

children_[index] = child;
Expand Down

0 comments on commit 85a017d

Please sign in to comment.