Repository navigation
Fix RawPropsParser ownership of temporary property names - #59479
Closed
Shubham-Padkonde wants to merge 1 commit into
Closed
Shubham-Padkonde wants to merge 1 commit into
Shubham-Padkonde wants to merge 1 commit into
Conversation
Contributor
|
RawProps.at is only supposed to be used with const char* args in constructor methods, for which I believe this is safe. Changing string_view to string significantly changes the memory costs here, and we should avoid it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
Fixes #59327.
RawPropsParser keeps property names after preparation, but its string views can refer to temporary names. In particular, RawProps::at(name, prefix, suffix) passes a view into a local concatenated string. Later duplicate checks and cursor lookups read that string after it has been destroyed.
Store owned strings in the parser and construct each stored key when it is first registered. Lookup still accepts string views; the copy happens during parser preparation. The key map already copies names into its own storage.
Changelog:
[GENERAL] [FIXED] - Retain property names used to prepare RawPropsParser so temporary and composed names remain valid during later lookups.
Test Plan:
Added two GoogleTest regressions covering short and long prefixes/suffixes, duplicate registration, repeated and out-of-order lookups, and temporary unprefixed names across vector growth.
Linux AddressSanitizer validation compiles the actual RawProps, RawPropsParser, RawPropsKeyMap and JSI sources with the new test file and React Native's Folly source configuration:
ASAN_OPTIONS=detect_stack_use_after_return=1 build-rawprops/rawprops_tests: both tests pass.54b5f22c33773a83de8fb6dc9a7091ba180842b4and rebuilding makes each regression fail independently: stack-use-after-return for composed names and stack-use-after-scope for temporary names.clang-format21.1.2 checks the new test file successfully;git diff --checkpasses.The focused harness and its workflow are on a separate fork branch, outside this PR. Full Android/iOS builds and the full native suite were not run.