Skip to content

Better Fabric ShadowNode memory safety#36258

Closed
NickGerleman wants to merge 1 commit into
facebook:mainfrom
NickGerleman:export-D43271779
Closed

Better Fabric ShadowNode memory safety#36258
NickGerleman wants to merge 1 commit into
facebook:mainfrom
NickGerleman:export-D43271779

Conversation

@NickGerleman
Copy link
Copy Markdown
Contributor

@NickGerleman NickGerleman commented Feb 23, 2023

Summary:
This fixes a few instances where YogaLayoutableShadowNode (or general shadownode casting) could offer better memory safety.

  1. The reference form of traitCast() now terminates on invalid cast, instead of debug assert, since it is better to crash in production than to corrupt memory (which will crash somewhere later, in a much more confusing way).
  2. We use traitCast() in more places where we previously would static_cast. This means needing to formally add a mutable version.
  3. We bounds-check yoga children access in a single place by using std::vector at() instead of [].
  4. Removed Trait::UnreservedTrait1 API, since multiple libraries using it can collide and we lose the memory safety benefits of traitCast.

This change is in response to a bug where YogaLayoutableShadowNode may perform an invalid static_cast of RawTextShadowNode if a text or number is rendered directly inside of a <View> (instead of a <Text> element).

This does not yet fix the underlying logic of YogaLayoutableShadowNode to act gracefully when a RawTextShadowNode makes its way into children. We just terminate, instead of corrupting memory.

Changelog:
[General][Breaking] - Better Fabric ShadowNode Memory Safety (Removes Trait::UnreservedTrait API)

Differential Revision: D43271779

Summary:
This fixes a few instances where YogaLayoutableShadowNode (or general shadownode casting) could offer better memory safety.

1. The reference form of traitCast() now terminates on invalid cast, instead of debug assert, since it is better to crash in production than to corrupt memory (which will crash somewhere later, in a much more confusing way).
2. We use traitCast() in more places where we previously would static_cast. This means needing to formally add a mutable version.
3. We bounds-check yoga children access in a single place by using `std::vector` `at()` instead of `[]`.

This change is in response to a bug where `YogaLayoutableShadowNode` may perform an invalid `static_cast` of `RawTextShadowNode` if a text or number is rendered directly inside of a `<View>` (instead of a `<Text>` element).

This does not yet fix the underlying logic of YogaLayoutableShadowNode to act gracefully when a RawTextShadowNode makes its way into children. We just terminate, instead of corrupting memory.

Changelog:
[Internal]

Differential Revision: D43271779

fbshipit-source-id: 31b28abe1a8754599f12cba098ebccd96881fd59
@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner fb-exported labels Feb 23, 2023
@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D43271779

@NickGerleman NickGerleman changed the title Better ShadowNode memory safety Better Fabric ShadowNode memory safety Feb 23, 2023
@analysis-bot
Copy link
Copy Markdown

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,459,768 +1,134
android hermes armeabi-v7a 7,783,237 +1,342
android hermes x86 8,936,020 +1,485
android hermes x86_64 8,793,168 +1,544
android jsc arm64-v8a 9,093,922 +1,150
android jsc armeabi-v7a 8,292,213 +1,344
android jsc x86 9,145,075 +1,485
android jsc x86_64 9,403,993 +1,538

Base commit: 9718c17
Branch: main

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Feb 27, 2023
@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request has been merged in c2a089f.

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. p: Facebook Partner: Facebook Partner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants