buffer: throw on failed fill attempts by cjihrig · Pull Request #17427 · nodejs/node · GitHub

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.

This also addresses an existing TODO comment.

Fixes: #17423

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

buffer

I'm not super familiar with this code but could this return a non-undefined value (that is, not from this return statement but the usual args.GetReturnValue().Set()) and then throw in JS? Then we could have an error code for it and document it?

Throwing from JavaScript would be ideal

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

I'm good with this but the new error needs to have an internal/errors error code assigned.

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>

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>

Updated to throw in JavaScript.

The documentation in buffer.md has to be updated as well and it would be good to switch to a TypeError.

Besides that this is the right approach out of my perspective and LGTM and we should do this, even if it is a BC.

This should be a TypeError as far as I see it.

+1... TypeError would be better here.

Oh, it seems the alternative already landed. I personally think this is actually the better approach but as that was already decided, I am closing this.

@BridgeAR I think this was still open because this might be the direction for 10.x while the other PR takes care of all other active release lines. I think it should be re-opened potentially?

buffer.md was already updated. I don't know that a TypeError is necessarily correct in this case. And yes, this is intended for Node 10.x.

Thanks, seems like I did not look closely enough in the other one. The documentation needs another update though. There is a example that would throw now in buffer.fill.

LGTM if changed to use TypeError

Updated to a TypeError, fixed the documentation example.

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

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>

@cjihrig even though I guess this is a uncontroversial commit, it did not get two TSC approvals yet.

Micro-nit: a -> an

@cjihrig even though I guess this is a uncontroversial commit, it did not get two TSC approvals yet.

Heh, we were just talking about stuff like this (sort of) in Build WG yesterday. One more data point/argument for moving to more automation. @maclover7

I'd like to propose dropping that rule. Or at least provide some type of time boxing where it no longer applies if there are no objections.

I'd like to propose dropping that rule. Or at least provide some type of time boxing where it no longer applies if there are no objections.

Sounds like re-opening nodejs/TSC#378.

I strongly opposed such a move, although I was in the minority. I still oppose it although I am probably still in the minority.

If two TSC members (out of 20!!!) can't be moved to review a change in a timely fashion, fix the TSC. Our first impulse is always to accommodate disengaged folks. We need to stop that.

In fairness to TSC folks, there wasn't any ping on this one so it may have slipped under the radar for lots of folks. If we want to change the rules for semver-major changes, let's make that change: Require @-mentioning @nodejs/tsc.

Requiring an @ mention sounds fair enough.

After-the-fact LGTM.

Refs: nodejs#17427 PR-URL: nodejs#17501 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

rather than @ mentioning I find adding @nodejs/tsc as a reviewer is a much better approach, that way it ends up in peoples github list of issues to review (as well as pinging them)

That could be both a requirement.

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>

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>

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Dec 2, 2017

apapirovski reviewed Dec 2, 2017

View reviewed changes

addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 2, 2017

addaleax mentioned this pull request Dec 2, 2017

buffer: zero-fill buffer allocated with invalid content #17428

Closed

4 tasks

cjihrig force-pushed the 17423 branch 5 times, most recently from 7209946 to 8b7ad01 Compare December 5, 2017 15:21

BridgeAR requested changes Dec 5, 2017

View reviewed changes

BridgeAR closed this Dec 5, 2017

cjihrig reopened this Dec 5, 2017

jasnell approved these changes Dec 5, 2017

View reviewed changes

cjihrig force-pushed the 17423 branch from 8b7ad01 to dfd1067 Compare December 6, 2017 15:52

BridgeAR approved these changes Dec 6, 2017

View reviewed changes

cjihrig force-pushed the 17423 branch from dfd1067 to cd174df Compare December 6, 2017 17:03

cjihrig merged commit cd174df into nodejs:master Dec 6, 2017

cjihrig deleted the 17423 branch December 6, 2017 17:04

Trott reviewed Dec 6, 2017

View reviewed changes

cjihrig mentioned this pull request Dec 6, 2017

doc: fix typo in Buffer.prototype.fill() #17501

Merged

3 tasks

cjihrig added a commit to cjihrig/node that referenced this pull request Dec 6, 2017

doc: fix typo in Buffer.prototype.fill()

70f23ec

Refs: nodejs#17427 PR-URL: nodejs#17501 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

cjihrig mentioned this pull request Jan 14, 2018

buffer: throw when filling with empty buffers #18129

Merged

4 tasks

               // Prints: <Buffer aa aa aa aa aa>
               console.log(buf.fill('aazz', 'hex'));
-              // Prints: <Buffer aa aa aa aa aa>
+              // Throws a exception.

buffer: throw on failed fill attempts by cjihrig · Pull Request #17427 · nodejs/node · GitHub

Uh oh!

Uh oh!

buffer: throw on failed fill attempts - #17427

buffer: throw on failed fill attempts#17427cjihrig merged 1 commit intonodejs:masternodejs/node:masterfrom cjihrig:17423cjihrig/node:17423Copy head branch name to clipboard

Conversation

cjihrig commented Dec 2, 2017

Checklist

Affected core subsystem(s)

Uh oh!

apapirovski Dec 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell Dec 5, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell commented Dec 5, 2017

Uh oh!

cjihrig commented Dec 5, 2017

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR Dec 5, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell Dec 5, 2017

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Dec 5, 2017

Uh oh!

apapirovski commented Dec 5, 2017

Uh oh!

cjihrig commented Dec 5, 2017

Uh oh!

BridgeAR commented Dec 5, 2017

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Dec 6, 2017

Uh oh!

BridgeAR commented Dec 6, 2017

Uh oh!

Trott Dec 6, 2017

Choose a reason for hiding this comment

Uh oh!

Trott commented Dec 6, 2017

Uh oh!

cjihrig commented Dec 6, 2017

Uh oh!

Trott commented Dec 6, 2017

Uh oh!

cjihrig commented Dec 6, 2017

Uh oh!

Trott left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Dec 6, 2017

Uh oh!

BridgeAR commented Dec 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

buffer: throw on failed fill attempts#17427
cjihrig merged 1 commit into
nodejs:masterfrom
cjihrig:17423

apapirovski Dec 2, 2017 •
edited
Loading

Trott left a comment •
edited
Loading