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: replace SlowBuffer with Buffer.allocUnsafeSlow(size) #5833

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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions benchmark/buffers/buffer-creation.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const bench = common.createBenchmark(main, {
'fast-alloc',
'fast-alloc-fill',
'fast-allocUnsafe',
'slow-allocUnsafe',
'slow',
'buffer()'],
len: [10, 1024, 2048, 4096, 8192],
Expand Down Expand Up @@ -39,6 +40,13 @@ function main(conf) {
}
bench.end(n);
break;
case 'slow-allocUnsafe':
bench.start();
for (let i = 0; i < n * 1024; i++) {
Buffer.allocUnsafeSlow(len);
}
bench.end(n);
break;
case 'slow':
bench.start();
for (let i = 0; i < n * 1024; i++) {
Expand Down
111 changes: 83 additions & 28 deletions doc/api/buffer.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -87,24 +87,26 @@ to one of these new APIs.*
containing a *copy* of the provided string.
* [`Buffer.alloc(size[, fill[, encoding]])`][buffer_alloc] returns a "filled"
`Buffer` instance of the specified size. This method can be significantly
slower than [`Buffer.allocUnsafe(size)`][buffer_allocunsafe] but ensures that
newly created `Buffer` instances never contain old and potentially sensitive
data.
* [`Buffer.allocUnsafe(size)`][buffer_allocunsafe] returns a new `Buffer` of
the specified `size` whose content *must* be initialized using either
[`buf.fill(0)`][] or written to completely.
slower than [`Buffer.allocUnsafe(size)`][buffer_allocunsafe] but ensures
that newly created `Buffer` instances never contain old and potentially
sensitive data.
* [`Buffer.allocUnsafe(size)`][buffer_allocunsafe] and
[`Buffer.allocUnsafeSlow(size)`][buffer_allocunsafeslow] each return a
new `Buffer` of the specified `size` whose content *must* be initialized
using either [`buf.fill(0)`][] or written to completely.

`Buffer` instances returned by `Buffer.allocUnsafe(size)` *may* be allocated
off a shared internal memory pool if the `size` is less than or equal to half
`Buffer.poolSize`.
off a shared internal memory pool if `size` is less than or equal to half
`Buffer.poolSize`. Instances returned by `Buffer.allocUnsafeSlow(size)` *never*
use the shared internal memory pool.

### The `--zero-fill-buffers` command line option

Node.js can be started using the `--zero-fill-buffers` command line option to
force all newly allocated `Buffer` and `SlowBuffer` instances created using
either `new Buffer(size)`, `Buffer.allocUnsafe(size)`, or
`new SlowBuffer(size)` to be *automatically zero-filled* upon creation. Use of
this flag *changes the default behavior* of these methods and *can have a
force all newly allocated `Buffer` instances created using either
`new Buffer(size)`, `Buffer.allocUnsafe(size)`, `Buffer.allocUnsafeSlow(size)`
or `new SlowBuffer(size)` to be *automatically zero-filled* upon creation. Use
of this flag *changes the default behavior* of these methods and *can have a
significant impact* on performance. Use of the `--zero-fill-buffers` option is
recommended only when absolutely necessary to enforce that newly allocated
`Buffer` instances cannot contain potentially sensitive data.
Expand All @@ -115,14 +117,14 @@ $ node --zero-fill-buffers
<Buffer 00 00 00 00 00>
```

### What makes `Buffer.allocUnsafe(size)` "unsafe"?
### What makes `Buffer.allocUnsafe(size)` and `Buffer.allocUnsafeSlow(size)` "unsafe"?

When calling `Buffer.allocUnsafe()`, the segment of allocated memory is
*uninitialized* (it is not zeroed-out). While this design makes the allocation
of memory quite fast, the allocated segment of memory might contain old data
that is potentially sensitive. Using a `Buffer` created by
`Buffer.allocUnsafe(size)` without *completely* overwriting the memory can
allow this old data to be leaked when the `Buffer` memory is read.
When calling `Buffer.allocUnsafe()` (and `Buffer.allocUnsafeSlow()`), the
segment of allocated memory is *uninitialized* (it is not zeroed-out). While
this design makes the allocation of memory quite fast, the allocated segment of
memory might contain old data that is potentially sensitive. Using a `Buffer`
created by `Buffer.allocUnsafe()` without *completely* overwriting the memory
can allow this old data to be leaked when the `Buffer` memory is read.

While there are clear performance advantages to using `Buffer.allocUnsafe()`,
extra care *must* be taken in order to avoid introducing security
Expand Down Expand Up @@ -339,8 +341,8 @@ console.log(buf);
Allocates a new `Buffer` of `size` bytes. The `size` must be less than
or equal to the value of `require('buffer').kMaxLength` (on 64-bit
architectures, `kMaxLength` is `(2^31)-1`). Otherwise, a [`RangeError`][] is
thrown. If a `size` less than 0 is specified, a zero-length Buffer will be
created.
thrown. A zero-length Buffer will be created if a `size` less than or equal to
0 is specified.

Unlike `ArrayBuffers`, the underlying memory for `Buffer` instances created in
this way is *not initialized*. The contents of a newly created `Buffer` are
Expand Down Expand Up @@ -397,8 +399,8 @@ console.log(buf);

The `size` must be less than or equal to the value of
`require('buffer').kMaxLength` (on 64-bit architectures, `kMaxLength` is
`(2^31)-1`). Otherwise, a [`RangeError`][] is thrown. If a `size` less than 0
is specified, a zero-length `Buffer` will be created.
`(2^31)-1`). Otherwise, a [`RangeError`][] is thrown. A zero-length Buffer will
be created if a `size` less than or equal to 0 is specified.

If `fill` is specified, the allocated `Buffer` will be initialized by calling
`buf.fill(fill)`. See [`buf.fill()`][] for more information.
Expand Down Expand Up @@ -431,8 +433,8 @@ A `TypeError` will be thrown if `size` is not a number.
Allocates a new *non-zero-filled* `Buffer` of `size` bytes. The `size` must
be less than or equal to the value of `require('buffer').kMaxLength` (on 64-bit
architectures, `kMaxLength` is `(2^31)-1`). Otherwise, a [`RangeError`][] is
thrown. If a `size` less than 0 is specified, a zero-length `Buffer` will be
created.
thrown. A zero-length Buffer will be created if a `size` less than or equal to
0 is specified.

The underlying memory for `Buffer` instances created in this way is *not
initialized*. The contents of the newly created `Buffer` are unknown and
Expand Down Expand Up @@ -466,6 +468,52 @@ Buffer pool if `size` is less than or equal to half `Buffer.poolSize`. The
difference is subtle but can be important when an application requires the
additional performance that `Buffer.allocUnsafe(size)` provides.

### Class Method: Buffer.allocUnsafeSlow(size)

* `size` {Number}

Allocates a new *non-zero-filled* and non-pooled `Buffer` of `size` bytes. The
`size` must be less than or equal to the value of
`require('buffer').kMaxLength` (on 64-bit architectures, `kMaxLength` is
`(2^31)-1`). Otherwise, a [`RangeError`][] is thrown. A zero-length Buffer will
be created if a `size` less than or equal to 0 is specified.

The underlying memory for `Buffer` instances created in this way is *not
initialized*. The contents of the newly created `Buffer` are unknown and
*may contain sensitive data*. Use [`buf.fill(0)`][] to initialize such
`Buffer` instances to zeroes.

When using `Buffer.allocUnsafe()` to allocate new `Buffer` instances,
allocations under 4KB are, by default, sliced from a single pre-allocated
`Buffer`. This allows applications to avoid the garbage collection overhead of
creating many individually allocated Buffers. This approach improves both
performance and memory usage by eliminating the need to track and cleanup as
many `Persistent` objects.

However, in the case where a developer may need to retain a small chunk of
memory from a pool for an indeterminate amount of time, it may be appropriate
to create an un-pooled Buffer instance using `Buffer.allocUnsafeSlow()` then
copy out the relevant bits.

```js
// need to keep around a few small chunks of memory
const store = [];

socket.on('readable', () => {
const data = socket.read();
// allocate for retained data
const sb = Buffer.allocUnsafeSlow(10);
// copy the data into the new allocation
data.copy(sb, 0, 0, 10);
store.push(sb);
});
```

Use of `Buffer.allocUnsafeSlow()` should be used only as a last resort *after*
a developer has observed undue memory retention in their applications.

A `TypeError` will be thrown if `size` is not a number.

### Class Method: Buffer.byteLength(string[, encoding])

* `string` {String | Buffer | TypedArray | DataView | ArrayBuffer}
Expand Down Expand Up @@ -1734,6 +1782,9 @@ Note that this is a property on the `buffer` module as returned by

## Class: SlowBuffer

Stability: 0 - Deprecated: Use
[`Buffer.allocUnsafeSlow(size)`][buffer_allocunsafeslow] instead.

Returns an un-pooled `Buffer`.

In order to avoid the garbage collection overhead of creating many individually
Expand Down Expand Up @@ -1764,13 +1815,16 @@ has observed undue memory retention in their applications.

### new SlowBuffer(size)

Stability: 0 - Deprecated: Use
[`Buffer.allocUnsafeSlow(size)`][buffer_allocunsafeslow] instead.

* `size` Number

Allocates a new `SlowBuffer` of `size` bytes. The `size` must be less than
or equal to the value of `require('buffer').kMaxLength` (on 64-bit
architectures, `kMaxLength` is `(2^31)-1`). Otherwise, a [`RangeError`][] is
thrown. If a `size` less than 0 is specified, a zero-length `SlowBuffer` will be
created.
thrown. A zero-length Buffer will be created if a `size` less than or equal to
0 is specified.

The underlying memory for `SlowBuffer` instances is *not initialized*. The
contents of a newly created `SlowBuffer` are unknown and could contain
Expand Down Expand Up @@ -1805,7 +1859,8 @@ console.log(buf);
[buffer_from_buffer]: #buffer_class_method_buffer_from_buffer
[buffer_from_arraybuf]: #buffer_class_method_buffer_from_arraybuffer
[buffer_from_string]: #buffer_class_method_buffer_from_str_encoding
[buffer_allocunsafe]: #buffer_class_method_buffer_allocraw_size
[buffer_allocunsafe]: #buffer_class_method_buffer_allocunsafe_size
[buffer_allocunsafeslow]: #buffer_class_method_buffer_allocunsafeslow_size
[buffer_alloc]: #buffer_class_method_buffer_alloc_size_fill_encoding
[`TypedArray.from()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray/from
[`DataView`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/DataView
Expand Down
29 changes: 25 additions & 4 deletions lib/buffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -133,13 +133,23 @@ Buffer.from = function(value, encodingOrOffset, length) {
Object.setPrototypeOf(Buffer.prototype, Uint8Array.prototype);
Object.setPrototypeOf(Buffer, Uint8Array);

function assertSize(size) {
if (typeof size !== 'number') {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can be stricter here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can be but I'm not sure we need to be.

const err = new TypeError('"size" argument must be a number');
// The following hides the 'assertSize' method from the
// callstack. This is done simply to hide the internal
// details of the implementation from bleeding out to users.
Error.captureStackTrace(err, assertSize);
throw err;
}
}

/**
* Creates a new filled Buffer instance.
* alloc(size[, fill[, encoding]])
**/
Buffer.alloc = function(size, fill, encoding) {
if (typeof size !== 'number')
throw new TypeError('"size" argument must be a number');
assertSize(size);
if (size <= 0)
return createBuffer(size);
if (fill !== undefined) {
Expand All @@ -161,11 +171,22 @@ Buffer.alloc = function(size, fill, encoding) {
* instance. If `--zero-fill-buffers` is set, will zero-fill the buffer.
**/
Buffer.allocUnsafe = function(size) {
if (typeof size !== 'number')
throw new TypeError('"size" argument must be a number');
assertSize(size);
return allocate(size);
};

/**
* Equivalent to SlowBuffer(num), by default creates a non-zero-filled
* Buffer instance that is not allocated off the pre-initialized pool.
* If `--zero-fill-buffers` is set, will zero-fill the buffer.
**/
Buffer.allocUnsafeSlow = function(size) {
assertSize(size);
if (size > 0)
flags[kNoZeroFill] = 1;
return createBuffer(size);
};

// If --zero-fill-buffers command line argument is set, a zero-filled
// buffer is returned.
function SlowBuffer(length) {
Expand Down
5 changes: 2 additions & 3 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

'use strict';

const SlowBuffer = require('buffer').SlowBuffer;
const util = require('util');
const pathModule = require('path');

Expand Down Expand Up @@ -321,7 +320,7 @@ ReadFileContext.prototype.read = function() {
var length;

if (this.size === 0) {
buffer = this.buffer = SlowBuffer(kReadFileBufferLength);
buffer = this.buffer = Buffer.allocUnsafeSlow(kReadFileBufferLength);
offset = 0;
length = kReadFileBufferLength;
} else {
Expand Down Expand Up @@ -389,7 +388,7 @@ function readFileAfterStat(err, st) {
return context.close(err);
}

context.buffer = SlowBuffer(size);
context.buffer = Buffer.allocUnsafeSlow(size);
context.read();
}

Expand Down
20 changes: 9 additions & 11 deletions test/parallel/test-buffer-alloc.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ var common = require('../common');
var assert = require('assert');

var Buffer = require('buffer').Buffer;
var SlowBuffer = require('buffer').SlowBuffer;

// counter to ensure unique value is always copied
var cntr = 0;
Expand Down Expand Up @@ -428,7 +427,7 @@ for (let i = 0; i < Buffer.byteLength(utf8String); i++) {

{
// also from a non-pooled instance
const b = new SlowBuffer(5);
const b = Buffer.allocUnsafeSlow(5);
const c = b.slice(0, 4);
const d = c.slice(0, 2);
assert.equal(c.parent, d.parent);
Expand Down Expand Up @@ -1305,7 +1304,7 @@ assert.throws(function() {

// try to slice a zero length Buffer
// see https://github.com/joyent/node/issues/5881
SlowBuffer(0).slice(0, 1);
Buffer.alloc(0).slice(0, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably may not make any difference but this is the only place which doesn't use allocUnsafeSlow

Copy link
Member Author

Choose a reason for hiding this comment

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

It makes no difference. alloc, allocUnsafe, and allocUnsafeSlow all do the same thing when the size is zero.

Copy link
Contributor

Choose a reason for hiding this comment

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

True, just documenting the observation :)

})();

// Regression test for #5482: should throw but not assert in C++ land.
Expand Down Expand Up @@ -1336,7 +1335,7 @@ assert.throws(function() {
}, RangeError);

assert.throws(function() {
SlowBuffer((-1 >>> 0) + 1);
Buffer.allocUnsafeSlow((-1 >>> 0) + 1);
}, RangeError);

if (common.hasCrypto) {
Expand Down Expand Up @@ -1435,14 +1434,13 @@ assert.throws(function() {
}, regErrorMsg);


// Test prototype getters don't throw
assert.equal(Buffer.prototype.parent, undefined);
assert.equal(Buffer.prototype.offset, undefined);
assert.equal(SlowBuffer.prototype.parent, undefined);
assert.equal(SlowBuffer.prototype.offset, undefined);


// Test that ParseArrayIndex handles full uint32
assert.throws(function() {
Buffer.from(new ArrayBuffer(0), -1 >>> 0);
}, /RangeError: 'offset' is out of bounds/);

// Unpooled buffer (replaces SlowBuffer)
const ubuf = Buffer.allocUnsafeSlow(10);
assert(ubuf);
assert(ubuf.buffer);
assert.equal(ubuf.buffer.byteLength, 10);