buffer: zero fill Buffer(num) by default by jasnell · Pull Request #12141 · nodejs/node

jasnell · 2017-03-31T03:14:54Z

Zero-fill Buffer(num) and new Buffer(num) by default.

Refs: nodejs/CTC#89

@nodejs/ctc

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)

buffer

jasnell · 2017-03-31T03:30:58Z

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

Trott · 2017-03-31T03:31:57Z

doc/api/buffer.md

-  a fast-but-uninitialized `Buffer` versus creating a slower-but-safer `Buffer`.
+  allocates a new `Buffer` object of the specified size. Prior to Node.js 8.0.0,
+  the memory allocated for such `Buffer` instances is *not* initialized and
+  *can contain sensitive data*. Such `Buffer` instances *must* be initialized


Nit: may contain is more idiomatic than can contain.

Trott · 2017-03-31T03:32:55Z

doc/api/buffer.md

+  allocates a new `Buffer` object of the specified size. Prior to Node.js 8.0.0,
+  the memory allocated for such `Buffer` instances is *not* initialized and
+  *can contain sensitive data*. Such `Buffer` instances *must* be initialized
+  *manually* by using either [`buf.fill(0)`][`buf.fill()`] or by writing to the


Nit: remove manually. If you want to emphasize that the user must do it--that it doesn't happen automatically--maybe replace with subsequently and remove the emphasis markers.

Trott · 2017-03-31T03:33:53Z

doc/api/buffer.md

+  the memory allocated for such `Buffer` instances is *not* initialized and
+  *can contain sensitive data*. Such `Buffer` instances *must* be initialized
+  *manually* by using either [`buf.fill(0)`][`buf.fill()`] or by writing to the
+  `Buffer` completely. While this behavior is *intentional* to improve


Nit: remove the entire sentence starting with While this behavior... It's not relevant here. In this case, reader doesn't need to know why it was changed, just that it was changed.

I disagree with this as it is useful context to understand why the new APIs exist. That said, this change wouldn't be related to the zero-fill change so should likely be made separately if at all.

Trott · 2017-03-31T03:34:26Z

doc/api/buffer.md

-[`Buffer.alloc(size)`][`Buffer.alloc()`] instead to initialize a `Buffer` to zeroes.
+Prior to Node.js 8.0.0, the underlying memory for `Buffer` instances
+created in this way is *not initialized*. The contents of a newly created
+`Buffer` are unknown and *could contain sensitive data*. Use


Nit: could -> may

Trott

Left a bunch of nits, any and all of which can be ignored if you disagree with them. LGTM if CI is green.

ChALkeR · 2017-03-31T06:52:58Z

I'm not sure how this can be semver-major if we were considering backporting it to 4/6/7 branches. Perhaps it's not semver-major?

bnoordhuis

LGTM with nits.

bnoordhuis · 2017-03-31T09:20:49Z

doc/api/buffer.md

-buf.fill(0);
-
-// Prints: <Buffer 00 00 00 00 00 00 00 00 00 00>
+// Prints: (contents may vary): <Buffer 00 00 00 00 00 00 00 00 00 00>


"contents may vary"? All zeroes are equal but some are more equal than others?

ha! that's what I get for not paying attention

bnoordhuis · 2017-03-31T09:24:03Z

test/parallel/test-buffer-zero-fill.js

+const buf1 = Buffer(100);
+const buf2 = new Buffer(100);
+
+let n = 0;


Fold the let into the for statements?

jasnell · 2017-03-31T16:22:40Z

Marked it as major defensively. The decision about whether or not to backport is not yet finalized.

jasnell · 2017-03-31T16:35:42Z

@nodejs/ctc ... I will land this PR on Monday if there are no objections

Trott · 2017-03-31T23:54:22Z

Note that we can land semver-major changes in LTS branches under some circumstances. The LTS README says:

Note that while it is possible that critical security and bug fixes may lead to semver-major changes landing within an LTS stream, such situations will be rare and will land as semver-minor bumps in the LTS covered version.

IMO, it is a stretch to call this a "critical security fix", but I also think it's a stretch to call this semver-major (although I appreciate @jasnell being conservative and labeling it semver-major just in case).

seishun · 2017-04-03T10:42:51Z

Benchmark results:

                                                             improvement confidence      p.value
 buffers\\buffer-creation.js n=1024 len=10 type="buffer()"      -73.79 %        *** 6.917922e-41
 buffers\\buffer-creation.js n=1024 len=1024 type="buffer()"    -60.20 %        *** 2.567890e-38
 buffers\\buffer-creation.js n=1024 len=2048 type="buffer()"    -52.65 %        *** 1.856146e-33
 buffers\\buffer-creation.js n=1024 len=4096 type="buffer()"     -6.18 %        *** 5.343071e-08
 buffers\\buffer-creation.js n=1024 len=8192 type="buffer()"    -40.21 %        *** 2.315718e-35

bnoordhuis · 2017-04-03T11:13:30Z

Marked it as major defensively.

It could be argued that, since some allocation patterns already result in buffers that are exclusively all zeroes, there is no real observable change in behavior and it's therefore not semver-major.

(It could also be argued that the previous sentence borders on sophistry. And the performance drop is real, of course.)

PR-URL: #12141 Ref: nodejs/CTC#89 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: Evan Lucas <[email protected]>

jasnell · 2017-04-03T16:59:55Z

Landed in 7eb1b46.
We can have a conversation during the CTC call about the semver level and backporting.

Trott · 2017-04-03T18:05:02Z

We can have a conversation during the CTC call about the semver level and backporting.

Just so we're all on the same page: Backporting decision was made (we won't backport) in nodejs/CTC#91. That said, the vote was close and there were a lot of abstentions, so if you have information that you think will persuade people to change their votes, we can call for another vote. Personally, though, I am wary of re-visiting a decision so soon.

jasnell · 2017-04-03T18:58:57Z

I'm not keen to reopen it any time soon. Marking it as semver-major, however, rather precludes the discussion. I'd just like to get a consensus opinion on the semver level

Up until now, test/callback.js assumed that the `cb.cif` object would not be garbage collected and was available to `Callback::Invoke`. That has never been a valid assumption, but as of nodejs/node#12141 Buffers created with `new Buffer(n)` each have their own `ArrayBuffer` which gets garbage-collected much more easily, which in turn would crash the test suite here. To (lazy-)fix this, assign `cb._cif` to some global variable that is guaranteed to stay alive.

addaleax · 2017-04-13T19:19:54Z

We probably should get into the habit of running citgm more frequently for these kinds of things, this just exposed a bug in the ffi test suite.

* **Async Hooks** * The `async_hooks` module has landed in core [[`4a7233c178`](nodejs@4a7233c178)] [nodejs#12892](nodejs#12892). * **Buffer** * Using the `--pending-deprecation` flag will cause Node.js to emit a deprecation warning when using `new Buffer(num)` or `Buffer(num)`. [[`d2d32ea5a2`](nodejs@d2d32ea5a2)] [nodejs#11968](nodejs#11968). * `new Buffer(num)` and `Buffer(num)` will zero-fill new `Buffer` instances [[`7eb1b4658e`](nodejs@7eb1b4658e)] [nodejs#12141](nodejs#12141). * Many `Buffer` methods now accept `Uint8Array` as input [[`beca3244e2`](nodejs@beca3244e2)] [nodejs#10236](nodejs#10236). * **Child Process** * Argument and kill signal validations have been improved [[`97a77288ce`](nodejs@97a77288ce)] [nodejs#12348](nodejs#12348), [[`d75fdd96aa`](nodejs@d75fdd96aa)] [nodejs#10423](nodejs#10423). * Child Process methods accept `Uint8Array` as input [[`627ecee9ed`](nodejs@627ecee9ed)] [nodejs#10653](nodejs#10653). * **Console** * Error events emitted when using `console` methods are now supressed. [[`f18e08d820`](nodejs@f18e08d820)] [nodejs#9744](nodejs#9744). * **Dependencies** * The npm client has been updated to 5.0.0 [[`3c3b36af0f`](nodejs@3c3b36af0f)] [nodejs#12936](nodejs#12936). * V8 has been updated to 5.8 with forward ABI stability to 6.0 [[`60d1aac8d2`](nodejs@60d1aac8d2)] [nodejs#12784](nodejs#12784). * **Domains** * Native `Promise` instances are now `Domain` aware [[`84dabe8373`](nodejs@84dabe8373)] [nodejs#12489](nodejs#12489). * **Errors** * We have started assigning static error codes to errors generated by Node.js. This has been done through multiple commits and is still a work in progress. * **File System** * The utility class `fs.SyncWriteStream` has been deprecated [[`7a55e34ef4`](nodejs@7a55e34ef4)] [nodejs#10467](nodejs#10467). * The deprecated `fs.read()` string interface has been removed [[`3c2a9361ff`](nodejs@3c2a9361ff)] [nodejs#9683](nodejs#9683). * **HTTP** * Improved support for userland implemented Agents [[`90403dd1d0`](nodejs@90403dd1d0)] [nodejs#11567](nodejs#11567). * Outgoing Cookie headers are concatenated into a single string [[`d3480776c7`](nodejs@d3480776c7)] [nodejs#11259](nodejs#11259). * The `httpResponse.writeHeader()` method has been deprecated [[`fb71ba4921`](nodejs@fb71ba4921)] [nodejs#11355](nodejs#11355). * New methods for accessing HTTP headers have been added to `OutgoingMessage` [[`3e6f1032a4`](nodejs@3e6f1032a4)] [nodejs#10805](nodejs#10805). * **Lib** * All deprecation messages have been assigned static identifiers [[`5de3cf099c`](nodejs@5de3cf099c)] [nodejs#10116](nodejs#10116). * The legacy `linkedlist` module has been removed [[`84a23391f6`](nodejs@84a23391f6)] [nodejs#12113](nodejs#12113). * **N-API** * Experimental support for the new N-API API has been added [[`56e881d0b0`](nodejs@56e881d0b0)] [nodejs#11975](nodejs#11975). * **Process** * Process warning output can be redirected to a file using the `--redirect-warnings` command-line argument [[`03e89b3ff2`](nodejs@03e89b3ff2)] [nodejs#10116](nodejs#10116). * Process warnings may now include additional detail [[`dd20e68b0f`](nodejs@dd20e68b0f)] [nodejs#12725](nodejs#12725). * **REPL** * REPL magic mode has been deprecated [[`3f27f02da0`](nodejs@3f27f02da0)] [nodejs#11599](nodejs#11599). * **Src** * `NODE_MODULE_VERSION` has been updated to 57 (nodejs@ec7cbaf266)] [nodejs#12995](nodejs#12995). * Add `--pending-deprecation` command-line argument and `NODE_PENDING_DEPRECATION` environment variable [[`a16b570f8c`](nodejs@a16b570f8c)] [nodejs#11968](nodejs#11968). * The `--debug` command-line argument has been deprecated. Note that using `--debug` will enable the *new* Inspector-based debug protocol as the legacy Debugger protocol previously used by Node.js has been removed. [[`010f864426`](nodejs@010f864426)] [nodejs#12949](nodejs#12949). * Throw when the `-c` and `-e` command-line arguments are used at the same time [[`a5f91ab230`](nodejs@a5f91ab230)] [nodejs#11689](nodejs#11689). * Throw when the `--use-bundled-ca` and `--use-openssl-ca` command-line arguments are used at the same time. [[`8a7db9d4b5`](nodejs@8a7db9d4b5)] [nodejs#12087](nodejs#12087). * **Stream** * `Stream` now supports `destroy()` and `_destroy()` APIs [[`b6e1d22fa6`](nodejs@b6e1d22fa6)] [nodejs#12925](nodejs#12925). * `Stream` now supports the `_final()` API [[`07c7f198db`](nodejs@07c7f198db)] [nodejs#12828](nodejs#12828). * **TLS** * The `rejectUnauthorized` option now defaults to `true` [[`348cc80a3c`](nodejs@348cc80a3c)] [nodejs#5923](nodejs#5923). * The `tls.createSecurePair()` API now emits a runtime deprecation [[`a2ae08999b`](nodejs@a2ae08999b)] [nodejs#11349](nodejs#11349). * A runtime deprecation will now be emitted when `dhparam` is less than 2048 bits [[`d523eb9c40`](nodejs@d523eb9c40)] [nodejs#11447](nodejs#11447). * **URL** * The WHATWG URL implementation is now a fully-supported Node.js API [[`d080ead0f9`](nodejs@d080ead0f9)] [nodejs#12710](nodejs#12710). * **Util** * `Symbol` keys are now displayed by default when using `util.inspect()` [[`5bfd13b81e`](nodejs@5bfd13b81e)] [nodejs#9726](nodejs#9726). * `toJSON` errors will be thrown when formatting `%j` [[`455e6f1dd8`](nodejs@455e6f1dd8)] [nodejs#11708](nodejs#11708). * Convert `inspect.styles` and `inspect.colors` to prototype-less objects [[`aab0d202f8`](nodejs@aab0d202f8)] [nodejs#11624](nodejs#11624). * The new `util.promisify()` API has been added [[`99da8e8e02`](nodejs@99da8e8e02)] [nodejs#12442](nodejs#12442). * **Zlib** * Support `Uint8Array` in Zlib convenience methods [[`91383e47fd`](nodejs@91383e47fd)] [nodejs#12001](nodejs#12001). * Zlib errors now use `RangeError` and `TypeError` consistently [[`b514bd231e`](nodejs@b514bd231e)] [nodejs#11391](nodejs#11391).

* **Async Hooks** * The `async_hooks` module has landed in core [[`4a7233c178`](4a7233c178)] [#12892](#12892). * **Buffer** * Using the `--pending-deprecation` flag will cause Node.js to emit a deprecation warning when using `new Buffer(num)` or `Buffer(num)`. [[`d2d32ea5a2`](d2d32ea5a2)] [#11968](#11968). * `new Buffer(num)` and `Buffer(num)` will zero-fill new `Buffer` instances [[`7eb1b4658e`](7eb1b4658e)] [#12141](#12141). * Many `Buffer` methods now accept `Uint8Array` as input [[`beca3244e2`](beca3244e2)] [#10236](#10236). * **Child Process** * Argument and kill signal validations have been improved [[`97a77288ce`](97a77288ce)] [#12348](#12348), [[`d75fdd96aa`](d75fdd96aa)] [#10423](#10423). * Child Process methods accept `Uint8Array` as input [[`627ecee9ed`](627ecee9ed)] [#10653](#10653). * **Console** * Error events emitted when using `console` methods are now supressed. [[`f18e08d820`](f18e08d820)] [#9744](#9744). * **Dependencies** * The npm client has been updated to 5.0.0 [[`3c3b36af0f`](3c3b36af0f)] [#12936](#12936). * V8 has been updated to 5.8 with forward ABI stability to 6.0 [[`60d1aac8d2`](60d1aac8d2)] [#12784](#12784). * **Domains** * Native `Promise` instances are now `Domain` aware [[`84dabe8373`](84dabe8373)] [#12489](#12489). * **Errors** * We have started assigning static error codes to errors generated by Node.js. This has been done through multiple commits and is still a work in progress. * **File System** * The utility class `fs.SyncWriteStream` has been deprecated [[`7a55e34ef4`](7a55e34ef4)] [#10467](#10467). * The deprecated `fs.read()` string interface has been removed [[`3c2a9361ff`](3c2a9361ff)] [#9683](#9683). * **HTTP** * Improved support for userland implemented Agents [[`90403dd1d0`](90403dd1d0)] [#11567](#11567). * Outgoing Cookie headers are concatenated into a single string [[`d3480776c7`](d3480776c7)] [#11259](#11259). * The `httpResponse.writeHeader()` method has been deprecated [[`fb71ba4921`](fb71ba4921)] [#11355](#11355). * New methods for accessing HTTP headers have been added to `OutgoingMessage` [[`3e6f1032a4`](3e6f1032a4)] [#10805](#10805). * **Lib** * All deprecation messages have been assigned static identifiers [[`5de3cf099c`](5de3cf099c)] [#10116](#10116). * The legacy `linkedlist` module has been removed [[`84a23391f6`](84a23391f6)] [#12113](#12113). * **N-API** * Experimental support for the new N-API API has been added [[`56e881d0b0`](56e881d0b0)] [#11975](#11975). * **Process** * Process warning output can be redirected to a file using the `--redirect-warnings` command-line argument [[`03e89b3ff2`](03e89b3ff2)] [#10116](#10116). * Process warnings may now include additional detail [[`dd20e68b0f`](dd20e68b0f)] [#12725](#12725). * **REPL** * REPL magic mode has been deprecated [[`3f27f02da0`](3f27f02da0)] [#11599](#11599). * **Src** * `NODE_MODULE_VERSION` has been updated to 57 (ec7cbaf266)] [#12995](#12995). * Add `--pending-deprecation` command-line argument and `NODE_PENDING_DEPRECATION` environment variable [[`a16b570f8c`](a16b570f8c)] [#11968](#11968). * The `--debug` command-line argument has been deprecated. Note that using `--debug` will enable the *new* Inspector-based debug protocol as the legacy Debugger protocol previously used by Node.js has been removed. [[`010f864426`](010f864426)] [#12949](#12949). * Throw when the `-c` and `-e` command-line arguments are used at the same time [[`a5f91ab230`](a5f91ab230)] [#11689](#11689). * Throw when the `--use-bundled-ca` and `--use-openssl-ca` command-line arguments are used at the same time. [[`8a7db9d4b5`](8a7db9d4b5)] [#12087](#12087). * **Stream** * `Stream` now supports `destroy()` and `_destroy()` APIs [[`b6e1d22fa6`](b6e1d22fa6)] [#12925](#12925). * `Stream` now supports the `_final()` API [[`07c7f198db`](07c7f198db)] [#12828](#12828). * **TLS** * The `rejectUnauthorized` option now defaults to `true` [[`348cc80a3c`](348cc80a3c)] [#5923](#5923). * The `tls.createSecurePair()` API now emits a runtime deprecation [[`a2ae08999b`](a2ae08999b)] [#11349](#11349). * A runtime deprecation will now be emitted when `dhparam` is less than 2048 bits [[`d523eb9c40`](d523eb9c40)] [#11447](#11447). * **URL** * The WHATWG URL implementation is now a fully-supported Node.js API [[`d080ead0f9`](d080ead0f9)] [#12710](#12710). * **Util** * `Symbol` keys are now displayed by default when using `util.inspect()` [[`5bfd13b81e`](5bfd13b81e)] [#9726](#9726). * `toJSON` errors will be thrown when formatting `%j` [[`455e6f1dd8`](455e6f1dd8)] [#11708](#11708). * Convert `inspect.styles` and `inspect.colors` to prototype-less objects [[`aab0d202f8`](aab0d202f8)] [#11624](#11624). * The new `util.promisify()` API has been added [[`99da8e8e02`](99da8e8e02)] [#12442](#12442). * **Zlib** * Support `Uint8Array` in Zlib convenience methods [[`91383e47fd`](91383e47fd)] [#12001](#12001). * Zlib errors now use `RangeError` and `TypeError` consistently [[`b514bd231e`](b514bd231e)] [#11391](#11391).

Up until now, test/callback.js assumed that the `cb.cif` object would not be garbage collected and was available to `Callback::Invoke`. That has never been a valid assumption, but as of nodejs/node#12141 Buffers created with `new Buffer(n)` each have their own `ArrayBuffer` which gets garbage-collected much more easily, which in turn would crash the test suite here. To (lazy-)fix this, assign `cb._cif` to some global variable that is guaranteed to stay alive. PR-URL: node-ffi#380

buffer: zero fill Buffer(num) by default

82a6028

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 31, 2017

nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Mar 31, 2017

doc: update the buffer.md to reflect change to zero-fill

283fb1b

Trott reviewed Mar 31, 2017

View reviewed changes

Trott approved these changes Mar 31, 2017

View reviewed changes

bnoordhuis approved these changes Mar 31, 2017

View reviewed changes

jasnell added this to the 8.0.0 milestone Mar 31, 2017

Address nits

76c9c12

cjihrig approved these changes Apr 1, 2017

View reviewed changes

princejwesley approved these changes Apr 3, 2017

View reviewed changes

evanlucas approved these changes Apr 3, 2017

View reviewed changes

jasnell closed this Apr 3, 2017

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

addaleax mentioned this pull request Apr 13, 2017

test: keep cif structs alive during gc node-ffi/node-ffi#380

Closed

seishun mentioned this pull request Jun 14, 2017

src: replace macro with lambda #13676

Closed

2 tasks

sam-github mentioned this pull request Aug 1, 2017

doc: describe what security issues are #14485

Merged

2 tasks

seishun mentioned this pull request Sep 11, 2017

buffer: runtime-deprecate Buffer ctor by default #15346

Closed

4 tasks

seishun mentioned this pull request Jun 15, 2018

buffer: runtime-deprecate Buffer constructor everywhere by default #21351

Closed

4 tasks

Uh oh!

Conversation

jasnell commented Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

jasnell commented Mar 31, 2017

Uh oh!

Trott Mar 31, 2017

Choose a reason for hiding this comment

Uh oh!

Trott Mar 31, 2017

Choose a reason for hiding this comment

Uh oh!

Trott Mar 31, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell Mar 31, 2017

Choose a reason for hiding this comment

Uh oh!

Trott Mar 31, 2017

Choose a reason for hiding this comment

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!

ChALkeR commented Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Mar 31, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell Mar 31, 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!

bnoordhuis Mar 31, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 31, 2017

Uh oh!

jasnell commented Mar 31, 2017

Uh oh!

Trott commented Mar 31, 2017

Uh oh!

seishun commented Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis commented Apr 3, 2017

Uh oh!

jasnell commented Apr 3, 2017

Uh oh!

Trott commented Apr 3, 2017

Uh oh!

jasnell commented Apr 3, 2017

Uh oh!

addaleax commented Apr 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

jasnell commented Mar 31, 2017 •

edited

Loading

Trott left a comment •

edited

Loading

ChALkeR commented Mar 31, 2017 •

edited

Loading

jasnell Mar 31, 2017 •

edited

Loading

seishun commented Apr 3, 2017 •

edited

Loading