Skip to content
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

Fix unsafe cast and detect resize overflow. #31106

Closed
wants to merge 2 commits into from

Conversation

rectified95
Copy link

Summary

Removing unsafe cast from int to uint16_t.
Also, adding code to detect multiplication overflow during buffer resize.

Changelog

[General] [Fix] - Fix unsafe cast and detect overflow in MapBuffer.

Test Plan

Code compiles in Visual Studio 2019 without the unsafe cast warning (or error depending on the configuration).

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 6, 2021
@rectified95 rectified95 added p: Microsoft Partner: Microsoft Partner labels Mar 6, 2021
@pull-bot
Copy link

pull-bot commented Mar 6, 2021

Messages
📖

📋 Verify Changelog Format - A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

CATEGORY may be:
  • General
  • iOS
  • Android
  • JavaScript
  • Internal (for changes that do not need to be called out in the release notes)

TYPE may be:

  • Added, for new features.
  • Changed, for changes in existing functionality.
  • Deprecated, for soon-to-be removed features.
  • Removed, for now removed features.
  • Fixed, for any bug fixes.
  • Security, in case of vulnerabilities.

MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Generated by 🚫 dangerJS against 76d08ce

@analysis-bot
Copy link

analysis-bot commented Mar 6, 2021

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 00959ff

@analysis-bot
Copy link

analysis-bot commented Mar 6, 2021

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,904,290 +250
android hermes armeabi-v7a 8,401,482 +372
android hermes x86 9,393,530 +475
android hermes x86_64 9,337,568 +400
android jsc arm64-v8a 10,638,342 +263
android jsc armeabi-v7a 10,118,641 +368
android jsc x86 10,689,084 +473
android jsc x86_64 11,273,386 +404

Base commit: 00959ff

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.

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

ReactCommon/react/renderer/mapbuffer/MapBuffer.h Outdated Show resolved Hide resolved
ReactCommon/react/renderer/mapbuffer/MapBuffer.cpp Outdated Show resolved Hide resolved
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.

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

@facebook-github-bot
Copy link
Contributor

@rozele merged this pull request in e69f1c9.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Mar 9, 2021
facebook-github-bot pushed a commit that referenced this pull request May 3, 2021
Summary:
Recent changes to `MapBuffer` have broken the compilation on Windows.
This fix is similar to this recently-merged change: #31106

Side note - this PR only addresses a build break, but doesn't address the unsafe casting semantics in `MapBuffer`, which can still cause overflows.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[General] [Fixed] - Fix compilation errors on Windows.

Pull Request resolved: #31363

Test Plan: RN now builds in Visual Studio on Windows.

Reviewed By: mdvacca

Differential Revision: D28028342

Pulled By: rozele

fbshipit-source-id: 77d8d4870c59b77acfc0ab2f4c3b7df40b59851d
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. p: Microsoft Partner: Microsoft Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants