Skip to content

Fix crash when calling substring() on a string containing emoji. #23609

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 7 commits into from

Conversation

ericlewis
Copy link
Contributor

@ericlewis ericlewis commented Feb 23, 2019

Summary

Fixes #23459. It is not legal to write the character array of a std::string, and can result in undefined behavior.

Changelog

[General] [Fixed] - Crash when substring intersects with emoji

Test Plan

Updated RNTester TextExample.ios.js & TextExample.android.js with a section containing the below:

<Text>{'test🙃'.substring(0, 5)}</Text>`

Also updated the test snapshot for iOS. So long as it doesn't crash tho, the test technically is passed.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mdvacca has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mdvacca has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ericlewis
Copy link
Contributor Author

@cpojer @hramos anything I need to do to get unstuck?

@hramos
Copy link
Contributor

hramos commented Feb 26, 2019

@ericlewis I followed up on the imported diff and it looks like David is seeking additional review from some fellow team members. No action needed on your part for now. Thanks for the PR!

@mdvacca
Copy link
Contributor

mdvacca commented Feb 28, 2019

@ericlewis the PR is approved, I tried to land this but got conflicts on the png. can you rebase it? I will land it after. Thanks

@ericlewis
Copy link
Contributor Author

@mdvacca done! (tho I seem to have messed up the history a bit 🤔)

@cpojer
Copy link
Contributor

cpojer commented Mar 1, 2019

@ericlewis yeah it seems like the PR was messed up. Could you rebase on top of master and force push to your branch with only the changes you made?

@ericlewis
Copy link
Contributor Author

@cpojer fixed!

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Mar 1, 2019
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mdvacca is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @ericlewis in 58c3a4c.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Mar 1, 2019
@react-native-bot react-native-bot removed the Import Started This pull request has been imported. This does not imply the PR has been approved. label Mar 1, 2019
grabbou pushed a commit that referenced this pull request Mar 11, 2019
)

Summary:
Fixes #23459. It is not legal to write the character array of a std::string, and can result in undefined behavior.

[General] [Fixed] - Crash when substring intersects with emoji
Pull Request resolved: #23609

Differential Revision: D14198159

Pulled By: mdvacca

fbshipit-source-id: 71060b1b99ddab89793c98c09f99ec9974479e62
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. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants