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.

Sign up for GitHub

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

Jump to bottom

lib,src: switch Buffer::kMaxLength to size_t #31406

Closed
wants to merge 2 commits into from
Closed

lib,src: switch Buffer::kMaxLength to size_t #31406

wants to merge 2 commits into from

Conversation

bnoordhuis
Copy link
Member

Change the type of Buffer::kMaxLength to size_t because upcoming
changes in V8 will allow typed arrays > 2 GB on 64 bits platforms.

Not all platforms handle file reads and writes > 2 GB though so keep
enforcing the 2 GB typed array limit for I/O operations.

Fixes: #31399
Refs: libuv/libuv#1501

@bnoordhuis
lib,src: switch Buffer::kMaxLength to size_t
967aa51 
Change the type of `Buffer::kMaxLength` to size_t because upcoming
changes in V8 will allow typed arrays > 2 GB on 64 bits platforms.

Not all platforms handle file reads and writes > 2 GB though so keep
enforcing the 2 GB typed array limit for I/O operations.

Fixes: nodejs#31399
Refs: libuv/libuv#1501
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 18, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@jakobkummerow
Copy link
Contributor

I think you'll want to update this line as well: replace Integer::NewFromUnsigned with Number::New; otherwise a limit of 0x100000000 will erroneously be converted to 0. (A strict compiler should warn about the implicit truncation...)

@bnoordhuis
Copy link
Member Author

Good catch. Updated, thanks.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 20, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28546/

@Trott
Copy link
Member

Trott commented Jan 21, 2020

Landed in 5005c3c

@Trott Trott closed this Jan 21, 2020
Trott pushed a commit that referenced this pull request Jan 21, 2020
@bnoordhuis @Trott
lib,src: switch Buffer::kMaxLength to size_t
5005c3c 
Change the type of `Buffer::kMaxLength` to size_t because upcoming
changes in V8 will allow typed arrays > 2 GB on 64 bits platforms.

Not all platforms handle file reads and writes > 2 GB though so keep
enforcing the 2 GB typed array limit for I/O operations.

Fixes: #31399
Refs: libuv/libuv#1501

PR-URL: #31406
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
@bnoordhuis @codebytere
lib,src: switch Buffer::kMaxLength to size_t
7ecf842 
Change the type of `Buffer::kMaxLength` to size_t because upcoming
changes in V8 will allow typed arrays > 2 GB on 64 bits platforms.

Not all platforms handle file reads and writes > 2 GB though so keep
enforcing the 2 GB typed array limit for I/O operations.

Fixes: #31399
Refs: libuv/libuv#1501

PR-URL: #31406
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
@codebytere codebytere mentioned this pull request Feb 17, 2020
Merged
@codebytere
Copy link
Member

@bnoordhuis is this a change that should go to v12.x? It'll need a manual backport if yes - I'll set the label but please swap it out if that's not the case.

@bnoordhuis bnoordhuis added semver-major PRs that contain breaking changes and should be released in the next major version. and removed backport-requested-v12.x labels Mar 15, 2020
@bnoordhuis
Copy link
Member Author

@codebytere I've updated the labels. A back-port shouldn't be necessary unless v12.x updates to a new enough V8 version and this PR is technically a semver-major change.

codebytere added a commit to electron/electron that referenced this pull request Sep 1, 2020
@codebytere
Remove buffer kMaxLength patch
2006f33 
nodejs/node#31406
codebytere added a commit to electron/electron that referenced this pull request Sep 2, 2020
@codebytere
Remove buffer kMaxLength patch
00f354c 
nodejs/node#31406
codebytere added a commit to electron/electron that referenced this pull request Sep 4, 2020
@codebytere
Remove buffer kMaxLength patch
33922f4 
nodejs/node#31406
@jasnell jasnell mentioned this pull request Sep 9, 2020
Closed
3 tasks
jasnell added a commit to jasnell/node that referenced this pull request Sep 9, 2020
@jasnell
crypto: fix regression in Hash, Hmac, SignBase, PublicKeyCipher
d6b4b8a 
nodejs#31406 introduced a regression in
`Hash`, `Hmac`, `SignBase`, and `PublicKeyCipher`

Signed-off-by: James M Snell <[email protected]>
codebytere added a commit to electron/electron that referenced this pull request Sep 14, 2020
@codebytere
Remove buffer kMaxLength patch
d617106 
nodejs/node#31406
codebytere added a commit to electron/electron that referenced this pull request Sep 14, 2020
@codebytere
Remove buffer kMaxLength patch
0034fb5 
nodejs/node#31406
codebytere added a commit to electron/electron that referenced this pull request Sep 14, 2020
@codebytere
Remove buffer kMaxLength patch
c6a06ba 
nodejs/node#31406
codebytere added a commit to electron/electron that referenced this pull request Sep 16, 2020
@codebytere
Remove buffer kMaxLength patch
7743fb6 
nodejs/node#31406
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@devnexen devnexen devnexen approved these changes

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@codebytere codebytere codebytere approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Using "unsigned int" for node::Buffer::kMaxLength prevents V8 from supporting larger TypedArrays