From 0abf9b06b580605486393e1a4dcfcbda4b6715a9 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 2 Apr 2019 04:39:46 +0200 Subject: [PATCH 1/3] buffer: stricter Buffer.from byteOffset check This validates the byteOffset stricter to prevent ambiguous inputs from being handled in unknown ways. --- lib/buffer.js | 2 +- test/parallel/test-buffer-arraybuffer.js | 11 ++++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/buffer.js b/lib/buffer.js index 01b9ea97f95a68..dd4e2952c8b23f 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -453,7 +453,7 @@ function fromArrayBuffer(obj, byteOffset, length) { } else { byteOffset = +byteOffset; if (NumberIsNaN(byteOffset)) - byteOffset = 0; + throw new ERR_INVALID_ARG_VALUE('byteOffset', byteOffset); } const maxLength = obj.byteLength - byteOffset; diff --git a/test/parallel/test-buffer-arraybuffer.js b/test/parallel/test-buffer-arraybuffer.js index bb22b879932571..c69d3ed632c6e6 100644 --- a/test/parallel/test-buffer-arraybuffer.js +++ b/test/parallel/test-buffer-arraybuffer.js @@ -107,11 +107,12 @@ assert.throws(function() { { // If byteOffset is not numeric, it defaults to 0. const ab = new ArrayBuffer(10); - const expected = Buffer.from(ab, 0); - assert.deepStrictEqual(Buffer.from(ab, 'fhqwhgads'), expected); - assert.deepStrictEqual(Buffer.from(ab, NaN), expected); - assert.deepStrictEqual(Buffer.from(ab, {}), expected); - assert.deepStrictEqual(Buffer.from(ab, []), expected); + ['fhqwhgads', NaN, {}].forEach((invalidByteOffset) => { + assert.throws( + () => Buffer.from(ab, invalidByteOffset), + { code: 'ERR_INVALID_ARG_VALUE' } + ); + }); // If byteOffset can be converted to a number, it will be. assert.deepStrictEqual(Buffer.from(ab, [1]), Buffer.from(ab, 1)); From 1ed3193bb3a49382ac2843e9be6a07edca070a1c Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 2 Apr 2019 04:40:25 +0200 Subject: [PATCH 2/3] buffer: remove support for unknown objects So far objects with a `length` property that was not an integer or positive resulted in an empty buffer being created. That is not ideal as it's not really clear what the intention in was. This throws an error from now on instead. --- lib/buffer.js | 7 +++---- test/parallel/test-buffer-alloc.js | 18 ++++++++++++------ test/parallel/test-buffer-arraybuffer.js | 5 ++++- 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/lib/buffer.js b/lib/buffer.js index dd4e2952c8b23f..f34568fe546935 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -28,6 +28,7 @@ const { MathFloor, MathMin, MathTrunc, + NumberIsInteger, NumberIsNaN, NumberMAX_SAFE_INTEGER, NumberMIN_SAFE_INTEGER, @@ -488,10 +489,8 @@ function fromObject(obj) { return b; } - if (obj.length !== undefined || isAnyArrayBuffer(obj.buffer)) { - if (typeof obj.length !== 'number') { - return new FastBuffer(); - } + if ((NumberIsInteger(obj.length) && obj.length >= 0) || + isAnyArrayBuffer(obj.buffer)) { return fromArrayLike(obj); } diff --git a/test/parallel/test-buffer-alloc.js b/test/parallel/test-buffer-alloc.js index b54fd88cc2506c..1812e4bf2dbcc7 100644 --- a/test/parallel/test-buffer-alloc.js +++ b/test/parallel/test-buffer-alloc.js @@ -769,8 +769,14 @@ Buffer.allocUnsafe(3.3).fill().toString(); // Throws bad argument error in commit 43cb4ec Buffer.alloc(3.3).fill().toString(); assert.strictEqual(Buffer.allocUnsafe(3.3).length, 3); -assert.strictEqual(Buffer.from({ length: 3.3 }).length, 3); -assert.strictEqual(Buffer.from({ length: 'BAM' }).length, 0); +assert.throws( + () => Buffer.from({ length: 3.3 }), + { code: 'ERR_INVALID_ARG_TYPE' } +); +assert.throws( + () => Buffer.from({ length: 'BAM' }), + { code: 'ERR_INVALID_ARG_TYPE' } +); // Make sure that strings are not coerced to numbers. assert.strictEqual(Buffer.from('99').length, 2); @@ -993,10 +999,10 @@ assert.strictEqual(SlowBuffer.prototype.offset, undefined); // Test that large negative Buffer length inputs don't affect the pool offset. // Use the fromArrayLike() variant here because it's more lenient // about its input and passes the length directly to allocate(). - assert.deepStrictEqual(Buffer.from({ length: -Buffer.poolSize }), - Buffer.from('')); - assert.deepStrictEqual(Buffer.from({ length: -100 }), - Buffer.from('')); + assert.throws( + () => Buffer.from({ length: -Buffer.poolSize }), + { code: 'ERR_INVALID_ARG_TYPE' } + ); // Check pool offset after that by trying to write string into the pool. Buffer.from('abc'); diff --git a/test/parallel/test-buffer-arraybuffer.js b/test/parallel/test-buffer-arraybuffer.js index c69d3ed632c6e6..0165e1db526a39 100644 --- a/test/parallel/test-buffer-arraybuffer.js +++ b/test/parallel/test-buffer-arraybuffer.js @@ -150,4 +150,7 @@ assert.throws(function() { } // Test an array like entry with the length set to NaN. -assert.deepStrictEqual(Buffer.from({ length: NaN }), Buffer.alloc(0)); +assert.throws( + () => Buffer.from({ length: NaN }), + { code: 'ERR_INVALID_ARG_TYPE' } +); From 99bdafceebfd603827bba8c93afdb01b7f91d2ef Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sun, 12 Jan 2020 23:36:11 +0100 Subject: [PATCH 3/3] doc: remove the buffer example using Symbol.toPrimitive The single buffer API is the only one supporting Symbol.toPrimitive but ideally it should not be used. Remove the documentation to prevent users from potentially using this API. --- doc/api/buffer.md | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/doc/api/buffer.md b/doc/api/buffer.md index c94f03fa065a36..04040e324be39a 100644 --- a/doc/api/buffer.md +++ b/doc/api/buffer.md @@ -884,15 +884,18 @@ appropriate for `Buffer.from()` variants. ### Class Method: `Buffer.from(object[, offsetOrEncoding[, length]])` -* `object` {Object} An object supporting `Symbol.toPrimitive` or `valueOf()`. +* `object` {Object} An object supporting `valueOf()`. * `offsetOrEncoding` {integer|string} A byte-offset or encoding, depending on - the value returned either by `object.valueOf()` or - `object[Symbol.toPrimitive]()`. -* `length` {integer} A length, depending on the value returned either by - `object.valueOf()` or `object[Symbol.toPrimitive]()`. + the value returned by `object.valueOf()`. +* `length` {integer} A length, depending on the value returned by + `object.valueOf()`. For objects whose `valueOf()` function returns a value not strictly equal to `object`, returns `Buffer.from(object.valueOf(), offsetOrEncoding, length)`. @@ -902,20 +905,6 @@ const buf = Buffer.from(new String('this is a test')); // Prints: ``` -For objects that support `Symbol.toPrimitive`, returns -`Buffer.from(object[Symbol.toPrimitive](), offsetOrEncoding, length)`. - -```js -class Foo { - [Symbol.toPrimitive]() { - return 'this is a test'; - } -} - -const buf = Buffer.from(new Foo(), 'utf8'); -// Prints: -``` - A `TypeError` will be thrown if `object` has not mentioned methods or is not of other type appropriate for `Buffer.from()` variants.