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

Buffer.alloc(): uninitialized buffer with an incorrectly encoded fill value #17423

Closed
vic511 opened this issue Dec 2, 2017 · 1 comment
Closed

Buffer.alloc(): uninitialized buffer with an incorrectly encoded fill value #17423

vic511 opened this issue Dec 2, 2017 · 1 comment
Labels
buffer Issues and PRs related to the buffer subsystem.

Comments

@vic511
Copy link

vic511 commented Dec 2, 2017

Executing this script leads to buf being uninitialized.

let buf = Buffer.alloc(0x100, "This is not correctly encoded", "hex");
console.log(buf);

PoC

I highly suspect this is because Buffer.alloc() calls buffer.fill() after allocating memory, which doesn't fill the buffer when it cannot decode the value parameter.
@vic511 vic511 changed the title Buffer.alloc(): uninitialized buffer when giving an incorrectly encoded fill value Buffer.alloc(): uninitialized buffer with an incorrectly encoded fill value Dec 2, 2017
@mscdex mscdex added the buffer Issues and PRs related to the buffer subsystem. label Dec 2, 2017
@cjihrig cjihrig mentioned this issue Dec 2, 2017
Merged
3 tasks
@addaleax addaleax mentioned this issue Dec 2, 2017
Closed
4 tasks
addaleax added a commit to addaleax/node that referenced this issue Dec 2, 2017
@addaleax
buffer: zero-fill buffer allocated with invalid content
19e2e14 
Zero-fill when `Buffer.alloc()` receives invalid fill data.

A solution like nodejs#17427 which switches
to throwing makes sense, but is likely a breaking change.

This suggestion leaves the behaviour of `buffer.fill()` untouched,
since any change to it would be a breaking change, and lets
`Buffer.alloc()` check whether any filling took place or not.

Refs: nodejs#17427
Refs: nodejs#17423
addaleax added a commit that referenced this issue Dec 5, 2017
@addaleax
buffer: zero-fill buffer allocated with invalid content
69a68c0 
Zero-fill when `Buffer.alloc()` receives invalid fill data.

A solution like #17427 which switches
to throwing makes sense, but is likely a breaking change.

This suggestion leaves the behaviour of `buffer.fill()` untouched,
since any change to it would be a breaking change, and lets
`Buffer.alloc()` check whether any filling took place or not.

PR-URL: #17428
Refs: #17427
Refs: #17423
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
addaleax added a commit to addaleax/node that referenced this issue Dec 5, 2017
@addaleax
buffer: zero-fill buffer allocated with invalid content
29f6218 
Zero-fill when `Buffer.alloc()` receives invalid fill data.

A solution like nodejs#17427 which switches
to throwing makes sense, but is likely a breaking change.

This suggestion leaves the behaviour of `buffer.fill()` untouched,
since any change to it would be a breaking change, and lets
`Buffer.alloc()` check whether any filling took place or not.

PR-URL: nodejs#17428
Refs: nodejs#17427
Refs: nodejs#17423
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@nodejs nodejs deleted a comment from mhdawson Dec 5, 2017
@nodejs nodejs deleted a comment from mhdawson Dec 5, 2017
cjihrig added a commit to cjihrig/node that referenced this issue Dec 6, 2017
@cjihrig
buffer: throw on failed fill attempts
cd174df 
If fill() attempts to write a string to a buffer, but fails
silently, then uninitialized memory could be leaked. This commit
causes fill() to throw if the string write operation fails.

Refs: nodejs#17423
PR-URL: nodejs#17427
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
gibfahn pushed a commit that referenced this issue Dec 6, 2017
@addaleax @gibfahn
buffer: zero-fill buffer allocated with invalid content
468562c 
Zero-fill when `Buffer.alloc()` receives invalid fill data.

A solution like #17427 which switches
to throwing makes sense, but is likely a breaking change.

This suggestion leaves the behaviour of `buffer.fill()` untouched,
since any change to it would be a breaking change, and lets
`Buffer.alloc()` check whether any filling took place or not.

PR-URL: #17428
Backport-PR-URL: #17467
Refs: #17427
Refs: #17423
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this issue Dec 7, 2017
@addaleax @MylesBorins
buffer: zero-fill buffer allocated with invalid content
b05ef97 
Zero-fill when `Buffer.alloc()` receives invalid fill data.

A solution like #17427 which switches
to throwing makes sense, but is likely a breaking change.

This suggestion leaves the behaviour of `buffer.fill()` untouched,
since any change to it would be a breaking change, and lets
`Buffer.alloc()` check whether any filling took place or not.

PR-URL: #17428
Backport-PR-URL: #17467
Refs: #17427
Refs: #17423
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this issue Dec 7, 2017
@addaleax @MylesBorins
buffer: zero-fill buffer allocated with invalid content
e511769 
Zero-fill when `Buffer.alloc()` receives invalid fill data.

A solution like #17427 which switches
to throwing makes sense, but is likely a breaking change.

This suggestion leaves the behaviour of `buffer.fill()` untouched,
since any change to it would be a breaking change, and lets
`Buffer.alloc()` check whether any filling took place or not.

PR-URL: #17428
Refs: #17427
Refs: #17423
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
evanlucas pushed a commit that referenced this issue Dec 7, 2017
@addaleax @evanlucas
buffer: zero-fill buffer allocated with invalid content
15bf640 
Zero-fill when `Buffer.alloc()` receives invalid fill data.

A solution like #17427 which switches
to throwing makes sense, but is likely a breaking change.

This suggestion leaves the behaviour of `buffer.fill()` untouched,
since any change to it would be a breaking change, and lets
`Buffer.alloc()` check whether any filling took place or not.

PR-URL: #17428
Refs: #17427
Refs: #17423
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@Trott
Copy link
Member

Trott commented Dec 11, 2017

@vic511 Thanks for reporting the bug. As you probably saw, it's now fixed in the latest release.

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.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mscdex @Trott @vic511