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

src: make buffers 2**32 index proof #31514

Closed
bnoordhuis opened this issue Jan 26, 2020 · 0 comments
Closed

src: make buffers 2**32 index proof #31514

bnoordhuis opened this issue Jan 26, 2020 · 0 comments
Labels
buffer Issues and PRs related to the buffer subsystem. lib / src Issues and PRs related to general changes in the lib or src directory.

Comments

@bnoordhuis
Copy link
Member

Refs: #31399

Audit src/node_buffer.cc, lib/buffer.js and lib/internal/buffer.js - there are places where assumptions are made about the type of indices, e.g.:

node/src/node_buffer.cc

Lines 593 to 596 in 3ff2aec

uint32_t start;
if (!args[2]->Uint32Value(ctx).To(&start)) return;
uint32_t end;
if (!args[3]->Uint32Value(ctx).To(&end)) return;

I suspect most C++ code can be switched over to ParseArrayIndex().
@bnoordhuis bnoordhuis added buffer Issues and PRs related to the buffer subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 26, 2020
DavenportEmma added a commit to DavenportEmma/node that referenced this issue Feb 3, 2020
@DavenportEmma
src: change Fill() to use ParseArrayIndex()
a60a104 
Changed Fill() to use ParseArrayIndex() when getting start and end of buffers instead of Uint32Value, supporting buffers of greater than 2**32

Fixes: nodejs#31514
DavenportEmma added a commit to DavenportEmma/node that referenced this issue Feb 3, 2020
@DavenportEmma
test: remove buffer overflow test
52b6882 
Removed test for overflowing buffers after changes in node_buffer.cc

Refs: https://github.com/ConorDavenport/node/blob/master/src/node_buffer.cc nodejs#31514
@jasnell jasnell closed this as completed in d4660ab Feb 3, 2020
codebytere pushed a commit that referenced this issue Feb 17, 2020
@DavenportEmma @codebytere
src: change Fill() to use ParseArrayIndex()
43e2c2e 
Changed Fill() to use ParseArrayIndex() when getting start and end
of buffers instead of Uint32Value, supporting buffers of greater
than 2**32

Fixes: #31514
Co-Authored-By: Rich Trott <[email protected]>

PR-URL: #31591
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this issue Mar 15, 2020
@DavenportEmma @codebytere
src: change Fill() to use ParseArrayIndex()
609a7f3 
Changed Fill() to use ParseArrayIndex() when getting start and end
of buffers instead of Uint32Value, supporting buffers of greater
than 2**32

Fixes: #31514
Co-Authored-By: Rich Trott <[email protected]>

PR-URL: #31591
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this issue Mar 17, 2020
@DavenportEmma @codebytere
src: change Fill() to use ParseArrayIndex()
da9e71b 
Changed Fill() to use ParseArrayIndex() when getting start and end
of buffers instead of Uint32Value, supporting buffers of greater
than 2**32

Fixes: #31514
Co-Authored-By: Rich Trott <[email protected]>

PR-URL: #31591
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this issue Mar 30, 2020
@DavenportEmma @codebytere
src: change Fill() to use ParseArrayIndex()
7e52e39 
Changed Fill() to use ParseArrayIndex() when getting start and end
of buffers instead of Uint32Value, supporting buffers of greater
than 2**32

Fixes: #31514
Co-Authored-By: Rich Trott <[email protected]>

PR-URL: #31591
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
buffer Issues and PRs related to the buffer subsystem. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@bnoordhuis