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.

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

buffer: zero fill Buffer(num) by default #12141

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
36 changes: 19 additions & 17 deletions doc/api/buffer.md
Expand Up @@ -52,13 +52,16 @@ In versions of Node.js prior to v6, `Buffer` instances were created using the
differently based on what arguments are provided:

* Passing a number as the first argument to `Buffer()` (e.g. `new Buffer(10)`),
allocates a new `Buffer` object of the specified size. 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 performance, development experience
has demonstrated that a more explicit distinction is required between creating
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: may contain is more idiomatic than can contain.

*manually* by using either [`buf.fill(0)`][`buf.fill()`] or by writing to the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

`Buffer` completely. While this behavior is *intentional* to improve
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

performance, development experience has demonstrated that a more explicit
distinction is required between creating a fast-but-uninitialized `Buffer`
versus creating a slower-but-safer `Buffer`. Starting in Node.js 8.0.0,
`Buffer(num)` and `new Buffer(num)` will return a `Buffer` with initialized
memory.
* Passing a string, array, or `Buffer` as the first argument copies the
passed object's data into the `Buffer`.
* Passing an [`ArrayBuffer`] returns a `Buffer` that shares allocated memory with
Expand Down Expand Up @@ -427,6 +430,9 @@ console.log(buf2.toString());
<!-- YAML
deprecated: v6.0.0
changes:
- version: v8.0.0
pr-url: https://github.com/nodejs/node/pull/12141
description: new Buffer(size) will return zero-filled memory by default.
- version: v7.2.1
pr-url: https://github.com/nodejs/node/pull/9529
description: Calling this constructor no longer emits a deprecation warning.
Expand All @@ -444,22 +450,18 @@ Allocates a new `Buffer` of `size` bytes. If the `size` is larger than
[`buffer.kMaxLength`] or smaller than 0, a [`RangeError`] will be thrown.
A zero-length `Buffer` will be created if `size` is 0.

Unlike [`ArrayBuffers`][`ArrayBuffer`], 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
[`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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: could -> may

[`Buffer.alloc(size)`][`Buffer.alloc()`] instead to initialize a `Buffer`
to zeroes.

Example:

```js
const buf = new Buffer(10);

// Prints: (contents may vary): <Buffer 48 21 4b 00 00 00 00 00 30 dd>
console.log(buf);

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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@jasnell jasnell Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

console.log(buf);
```

Expand Down
2 changes: 1 addition & 1 deletion lib/buffer.js
Expand Up @@ -102,7 +102,7 @@ function Buffer(arg, encodingOrOffset, length) {
'If encoding is specified then the first argument must be a string'
);
}
return Buffer.allocUnsafe(arg);
return Buffer.alloc(arg);
}
return Buffer.from(arg, encodingOrOffset, length);
}
Expand Down
16 changes: 16 additions & 0 deletions test/parallel/test-buffer-zero-fill.js
@@ -0,0 +1,16 @@
'use strict';

require('../common');
const assert = require('assert');
const Buffer = require('buffer').Buffer;

const buf1 = Buffer(100);
const buf2 = new Buffer(100);

let n = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fold the let into the for statements?


for (n = 0; n < buf1.length; n++)
assert.strictEqual(buf1[n], 0);

for (n = 0; n < buf2.length; n++)
assert.strictEqual(buf2[n], 0);