buffer: throw on failed fill attempts #17427

cjihrig · 2017-12-02T15:17:43Z

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

apapirovski · 2017-12-02T15:22:18Z

src/node_buffer.cc

    if (str_length == 0)
-      return;
+      return env->ThrowError("Buffer could not be filled");


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

jasnell · 2017-12-05T00:27:36Z

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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

cjihrig · 2017-12-05T15:23:20Z

Updated to throw in JavaScript.

BridgeAR

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.

BridgeAR · 2017-12-05T16:04:38Z

lib/buffer.js


-  return bindingFill(buf, val, start, end, encoding);
+  if (fillLength === -1)
+    throw new errors.Error('ERR_INVALID_ARG_VALUE', 'value', val);


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

+1... TypeError would be better here.

BridgeAR · 2017-12-05T16:11:26Z

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.

apapirovski · 2017-12-05T16:13:04Z

@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?

cjihrig · 2017-12-05T16:15:26Z

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.

BridgeAR · 2017-12-05T16:31:03Z

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.

jasnell

LGTM if changed to use TypeError

cjihrig · 2017-12-06T15:54:37Z

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 <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

BridgeAR · 2017-12-06T17:19:36Z

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

Trott · 2017-12-06T17:32:14Z

doc/api/buffer.md


 ```js
 const buf = Buffer.allocUnsafe(5);
 // Prints: <Buffer 61 61 61 61 61>
 console.log(buf.fill('a'));
 // Prints: <Buffer aa aa aa aa aa>
 console.log(buf.fill('aazz', 'hex'));
-// Prints: <Buffer aa aa aa aa aa>
+// Throws a exception.


Micro-nit: a -> an

Trott · 2017-12-06T17:35:40Z

@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

cjihrig · 2017-12-06T17:36:11Z

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.

Trott · 2017-12-06T17:48:46Z

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.

cjihrig · 2017-12-06T17:50:42Z

Requiring an @ mention sounds fair enough.

Trott

After-the-fact LGTM.

Refs: nodejs#17427 PR-URL: nodejs#17501 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]>

MylesBorins · 2017-12-06T18:16:51Z

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)

BridgeAR · 2017-12-06T18:40:40Z

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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

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 <[email protected]> Reviewed-By: Rich Trott <[email protected]>

cjihrig mentioned this pull request Jan 14, 2018

buffer: throw when filling with empty buffers #18129

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buffer: throw on failed fill attempts #17427

buffer: throw on failed fill attempts #17427

cjihrig commented Dec 2, 2017

apapirovski Dec 2, 2017 •

edited

Loading

jasnell Dec 5, 2017

jasnell commented Dec 5, 2017

cjihrig commented Dec 5, 2017

BridgeAR left a comment

BridgeAR Dec 5, 2017

jasnell Dec 5, 2017

BridgeAR commented Dec 5, 2017

apapirovski commented Dec 5, 2017

cjihrig commented Dec 5, 2017

BridgeAR commented Dec 5, 2017

jasnell left a comment

cjihrig commented Dec 6, 2017

BridgeAR commented Dec 6, 2017

Trott Dec 6, 2017

Trott commented Dec 6, 2017

cjihrig commented Dec 6, 2017

Trott commented Dec 6, 2017

cjihrig commented Dec 6, 2017

Trott left a comment •

edited

Loading

MylesBorins commented Dec 6, 2017

BridgeAR commented Dec 6, 2017

buffer: throw on failed fill attempts #17427

buffer: throw on failed fill attempts #17427

Conversation

cjihrig commented Dec 2, 2017

Checklist

Affected core subsystem(s)

apapirovski Dec 2, 2017 • edited Loading

Choose a reason for hiding this comment

jasnell Dec 5, 2017

Choose a reason for hiding this comment

jasnell commented Dec 5, 2017

cjihrig commented Dec 5, 2017

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR Dec 5, 2017

Choose a reason for hiding this comment

jasnell Dec 5, 2017

Choose a reason for hiding this comment

BridgeAR commented Dec 5, 2017

apapirovski commented Dec 5, 2017

cjihrig commented Dec 5, 2017

BridgeAR commented Dec 5, 2017

jasnell left a comment

Choose a reason for hiding this comment

cjihrig commented Dec 6, 2017

BridgeAR commented Dec 6, 2017

Trott Dec 6, 2017

Choose a reason for hiding this comment

Trott commented Dec 6, 2017

cjihrig commented Dec 6, 2017

Trott commented Dec 6, 2017

cjihrig commented Dec 6, 2017

Trott left a comment • edited Loading

Choose a reason for hiding this comment

MylesBorins commented Dec 6, 2017

BridgeAR commented Dec 6, 2017

apapirovski Dec 2, 2017 •

edited

Loading

Trott left a comment •

edited

Loading