buffer: use stricter range checks by BridgeAR · Pull Request #27045 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation14 Commits2 Checks0 Files changed

Conversation

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

[ Show hidden characters]({{ revealButtonHref }})

BridgeAR

This adds support to use offset and length arguments above uint32 and it validates the input to make sure the arguments do not overflow.
Before, if the input would overflow, it would cause the write to be
performt in the wrong spot / result in unexpected behavior.
Instead, just use a strict number validation.

Fixes: #27043

CITGM https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1790/

Checklist

@BridgeAR BridgeAR added the semver-major

PRs that contain breaking changes and should be released in the next major version.

label

Apr 1, 2019

@nodejs-github-bot

@nodejs-github-bot nodejs-github-bot added buffer

Issues and PRs related to the buffer subsystem.

errors

Issues and PRs related to JavaScript errors originated in Node.js core.

labels

Apr 1, 2019

@BridgeAR

This adds support to use offset and length arguments above uint32 and it validates the input to make sure the arguments do not overflow. Before, if the input would overflow, it would cause the write to be performt in the wrong spot / result in unexpected behavior. Instead, just use a strict number validation.

Fixes: nodejs#27043

addaleax

@BridgeAR

@nodejs-github-bot

@BridgeAR

@nodejs/buffer @nodejs/tsc PTAL

@BridgeAR

This could use some reviews.

mcollina

Choose a reason for hiding this comment

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

LGTM with a clean CITGM

@BridgeAR

@BridgeAR BridgeAR added the author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

label

Apr 5, 2019

@Trott

Does it make sense to benchmark this change?

@BridgeAR

@Trott performance wise this shouldn't make a big difference and validation is IMO more important than squeezing out a few percent more.

@BridgeAR

@nodejs/buffer @nodejs/tsc PTAL. This requires one more LG from the TSC.

@BridgeAR

Trott

BridgeAR added a commit to BridgeAR/node that referenced this pull request

Apr 15, 2019

@BridgeAR

This validates the input to make sure the arguments do not overflow. Before, if the input would overflow, it would cause the write to be performt in the wrong spot / result in unexpected behavior. Instead, just use a strict number validation.

PR-URL: nodejs#27045 Fixes: nodejs#27043 Reviewed-By: Matteo Collina matteo.collina@gmail.com Reviewed-By: Rich Trott rtrott@gmail.com

@BridgeAR

BethGriggs added a commit that referenced this pull request

Apr 22, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

BethGriggs added a commit that referenced this pull request

Apr 23, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

BethGriggs added a commit that referenced this pull request

Apr 23, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

This was referenced

Apr 23, 2019

This was referenced

Apr 23, 2019

@BridgeAR BridgeAR deleted the improve-buffer-input-checks branch

January 20, 2020 12:01

Labels

author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

buffer

Issues and PRs related to the buffer subsystem.

errors

Issues and PRs related to JavaScript errors originated in Node.js core.

review wanted

PRs that need reviews.

semver-major

PRs that contain breaking changes and should be released in the next major version.