From cf94cb9413416e0051f57396032e83fbffe8041a Mon Sep 17 00:00:00 2001 From: James M Snell Date: Mon, 21 Mar 2016 12:38:08 -0700 Subject: [PATCH 1/3] buffer: add Buffer.allocUnsafeSlow(size) Aligns the functionality of SlowBuffer with the new Buffer constructor API. Next step is to docs-only deprecate SlowBuffer. Replace the internal uses of SlowBuffer with `Buffer.allocUnsafeSlow(size)` --- benchmark/buffers/buffer-creation.js | 8 +++ doc/api/buffer.markdown | 94 +++++++++++++++++++++------- lib/buffer.js | 29 +++++++-- lib/fs.js | 5 +- test/parallel/test-buffer-alloc.js | 20 +++--- 5 files changed, 116 insertions(+), 40 deletions(-) diff --git a/benchmark/buffers/buffer-creation.js b/benchmark/buffers/buffer-creation.js index e0e64d6934dcf3..d1acd2694e4228 100644 --- a/benchmark/buffers/buffer-creation.js +++ b/benchmark/buffers/buffer-creation.js @@ -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], @@ -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++) { diff --git a/doc/api/buffer.markdown b/doc/api/buffer.markdown index c1779b0d319ccf..bab334629cb106 100644 --- a/doc/api/buffer.markdown +++ b/doc/api/buffer.markdown @@ -87,27 +87,30 @@ 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 -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. +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. ``` $ node --zero-fill-buffers @@ -115,14 +118,14 @@ $ node --zero-fill-buffers ``` -### 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 @@ -466,6 +469,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. If a `size` less than 0 +is specified, a zero-length `Buffer` will be created. + +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} @@ -1805,7 +1854,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 diff --git a/lib/buffer.js b/lib/buffer.js index f672dae558d320..1d0963bb509544 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -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') { + 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) { @@ -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) { diff --git a/lib/fs.js b/lib/fs.js index bfb7d874f7d71f..3479b3c1296018 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -3,7 +3,6 @@ 'use strict'; -const SlowBuffer = require('buffer').SlowBuffer; const util = require('util'); const pathModule = require('path'); @@ -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 { @@ -389,7 +388,7 @@ function readFileAfterStat(err, st) { return context.close(err); } - context.buffer = SlowBuffer(size); + context.buffer = Buffer.allocUnsafeSlow(size); context.read(); } diff --git a/test/parallel/test-buffer-alloc.js b/test/parallel/test-buffer-alloc.js index 733fa105043c1c..131fcb112f5641 100644 --- a/test/parallel/test-buffer-alloc.js +++ b/test/parallel/test-buffer-alloc.js @@ -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; @@ -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); @@ -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); })(); // Regression test for #5482: should throw but not assert in C++ land. @@ -1336,7 +1335,7 @@ assert.throws(function() { }, RangeError); assert.throws(function() { - SlowBuffer((-1 >>> 0) + 1); + Buffer.allocUnsafeSlow((-1 >>> 0) + 1); }, RangeError); if (common.hasCrypto) { @@ -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); From 09923c6bdd24c9828e05654b29251a37b43d7a8e Mon Sep 17 00:00:00 2001 From: James M Snell Date: Mon, 21 Mar 2016 12:39:57 -0700 Subject: [PATCH 2/3] buffer: docs-only deprecate SlowBuffer With the addition of `Buffer.allocUnsafeSlow(size)` `SlowBuffer` can be deprecated... but docs-only for now. --- doc/api/buffer.markdown | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/doc/api/buffer.markdown b/doc/api/buffer.markdown index bab334629cb106..38ef0d2e1fbfed 100644 --- a/doc/api/buffer.markdown +++ b/doc/api/buffer.markdown @@ -1783,6 +1783,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 @@ -1813,6 +1816,9 @@ 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 From e40c4f9df06c0ab80c8437a4a9e172fe843f6b6d Mon Sep 17 00:00:00 2001 From: James M Snell Date: Tue, 12 Apr 2016 10:40:56 -0700 Subject: [PATCH 3/3] doc: minor copy improvement in buffer.markdown --- doc/api/buffer.markdown | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/doc/api/buffer.markdown b/doc/api/buffer.markdown index 38ef0d2e1fbfed..798ecbfcc977a3 100644 --- a/doc/api/buffer.markdown +++ b/doc/api/buffer.markdown @@ -103,14 +103,13 @@ 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)`, -`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. +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. ``` $ node --zero-fill-buffers @@ -342,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 @@ -400,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. @@ -434,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 @@ -476,8 +475,8 @@ additional performance that `Buffer.allocUnsafe(size)` provides. 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. 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. The underlying memory for `Buffer` instances created in this way is *not initialized*. The contents of the newly created `Buffer` are unknown and @@ -1824,8 +1823,8 @@ has observed undue memory retention in their applications. 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