[v8.x] buffer: zero-fill buffer allocated with invalid content #17467

addaleax · 2017-12-05T11:59:13Z

Checklist

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

Affected core subsystem(s)

`test-inspector-async-hook-setup-at-signal` is also flaky on VS2017

PR-URL: nodejs#17428 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>

addaleax · 2017-12-05T12:01:20Z

CI: https://ci.nodejs.org/job/node-test-commit/14571/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1125/

@gibfahn It would be great if this could go into today’s release or the security release

gibfahn · 2017-12-05T20:58:17Z

@gibfahn It would be great if this could go into today’s release or the security release

I think putting it in the security release makes sense.

BridgeAR · 2017-12-06T01:25:13Z

test/sequential/sequential.status

@@ -10,6 +10,7 @@ prefix sequential
 test-inspector-async-call-stack       : PASS, FLAKY
 test-inspector-bindings               : PASS, FLAKY
 test-inspector-debug-end              : PASS, FLAKY
+test-inspector-async-hook-setup-at-signal:  PASS, FLAKY


Hm, is this related to the change here?

No, this snuck its way in here because v8.x-staging was rebased.

The commits should still apply cleanly; I can rebase this PR if @gibfahn likes.

fhinkel · 2017-12-06T16:02:40Z

doc/api/buffer.md

+// 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'));


I'm wondering if we should use something like bbzz here. With a it's unclear if no filling is performed or if the string was truncated. Also, should the last example have a // Prints: ...?

fhinkel · 2017-12-06T16:06:42Z

lib/buffer.js

@@ -238,7 +238,9 @@ Buffer.alloc = function(size, fill, encoding) {
    // be interpreted as a start offset.
    if (typeof encoding !== 'string')
      encoding = undefined;
-    return createUnsafeBuffer(size).fill(fill, encoding);
+    const ret = createUnsafeBuffer(size);
+    if (fill_(ret, fill, encoding) > 0)


Maybe leave a comment here?

// Invalid data, no filling.

fhinkel · 2017-12-06T16:08:02Z

lib/buffer.js

@@ -832,19 +839,17 @@ Buffer.prototype.fill = function fill(val, start, end, encoding) {
  }

  // Invalid ranges are not set to a default, so can range check early.


Should it say "so one can range check early."?

Or maybe

// Early range check because invalid ranges are not set to a default.

fhinkel

LGTM with nits

addaleax · 2017-12-06T19:49:18Z

@fhinkel Would it be okay with you if I resolved your comments in a separate PR against master, not in the backport?

PR-URL: #17428 Backport-PR-URL: #17467 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 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>

gibfahn

Thanks @addaleax

gibfahn · 2017-12-06T20:59:45Z

Landed in 1e9cc5e and 468562c

PR-URL: #17428 Backport-PR-URL: #17467 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 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>

refack and others added 3 commits November 28, 2017 13:17

test: flag known flake

3668a82

`test-inspector-async-hook-setup-at-signal` is also flaky on VS2017

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++. v8.x labels Dec 5, 2017

addaleax added the security Issues and PRs related to security. label Dec 5, 2017

gibfahn force-pushed the v8.x-staging branch from 3668a82 to 73188a3 Compare December 5, 2017 22:14

BridgeAR reviewed Dec 6, 2017

View reviewed changes

fhinkel reviewed Dec 6, 2017

View reviewed changes

fhinkel approved these changes Dec 6, 2017

View reviewed changes

gibfahn approved these changes Dec 6, 2017

View reviewed changes

gibfahn closed this Dec 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v8.x] buffer: zero-fill buffer allocated with invalid content #17467

[v8.x] buffer: zero-fill buffer allocated with invalid content #17467

addaleax commented Dec 5, 2017 •

edited

Loading

addaleax commented Dec 5, 2017

gibfahn commented Dec 5, 2017

BridgeAR Dec 6, 2017

addaleax Dec 6, 2017

fhinkel Dec 6, 2017 •

edited

Loading

fhinkel Dec 6, 2017

fhinkel Dec 6, 2017

fhinkel Dec 6, 2017

fhinkel left a comment

addaleax commented Dec 6, 2017

gibfahn left a comment

gibfahn commented Dec 6, 2017

		@@ -832,19 +839,17 @@ Buffer.prototype.fill = function fill(val, start, end, encoding) {
		}

		// Invalid ranges are not set to a default, so can range check early.

[v8.x] buffer: zero-fill buffer allocated with invalid content #17467

[v8.x] buffer: zero-fill buffer allocated with invalid content #17467

Conversation

addaleax commented Dec 5, 2017 • edited Loading

Checklist

Affected core subsystem(s)

addaleax commented Dec 5, 2017

gibfahn commented Dec 5, 2017

BridgeAR Dec 6, 2017

Choose a reason for hiding this comment

addaleax Dec 6, 2017

Choose a reason for hiding this comment

fhinkel Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

fhinkel Dec 6, 2017

Choose a reason for hiding this comment

fhinkel Dec 6, 2017

Choose a reason for hiding this comment

fhinkel Dec 6, 2017

Choose a reason for hiding this comment

fhinkel left a comment

Choose a reason for hiding this comment

addaleax commented Dec 6, 2017

gibfahn left a comment

Choose a reason for hiding this comment

gibfahn commented Dec 6, 2017

addaleax commented Dec 5, 2017 •

edited

Loading

fhinkel Dec 6, 2017 •

edited

Loading