From 45f50642c84bf2ab8457d591fdca3d3ab9176be1 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 10 Dec 2016 22:28:37 +0100 Subject: [PATCH 1/3] buffer: allow Uint8Array input to methods Allow all methods on `buffer` and `Buffer` to take `Uint8Array` arguments where it makes sense. On the native side, there is effectively no difference, and as a bonus the `isUint8Array` check is faster than `instanceof Buffer`. --- doc/api/buffer.md | 24 ++++++++++++------------ lib/buffer.js | 22 +++++++++++----------- lib/internal/buffer.js | 5 +++-- src/node_util.cc | 3 ++- test/parallel/test-buffer-compare.js | 5 +++++ test/parallel/test-buffer-concat.js | 4 ++++ test/parallel/test-buffer-equals.js | 1 + test/parallel/test-buffer-indexof.js | 8 ++++++++ test/parallel/test-icu-transcode.js | 8 ++++++++ 9 files changed, 54 insertions(+), 26 deletions(-) diff --git a/doc/api/buffer.md b/doc/api/buffer.md index b9f8369335a0d6..33da664c27e939 100644 --- a/doc/api/buffer.md +++ b/doc/api/buffer.md @@ -635,8 +635,8 @@ actual byte length is returned. added: v0.11.13 --> -* `buf1` {Buffer} -* `buf2` {Buffer} +* `buf1` {Buffer|Uint8Array} +* `buf2` {Buffer|Uint8Array} * Returns: {Integer} Compares `buf1` to `buf2` typically for the purpose of sorting arrays of @@ -660,7 +660,7 @@ console.log(arr.sort(Buffer.compare)); added: v0.7.11 --> -* `list` {Array} List of `Buffer` instances to concat +* `list` {Array} List of `Buffer` or [`Uint8Array`] instances to concat * `totalLength` {Integer} Total length of the `Buffer` instances in `list` when concatenated * Returns: {Buffer} @@ -882,7 +882,7 @@ console.log(buf.toString('ascii')); added: v0.11.13 --> -* `target` {Buffer} A `Buffer` to compare to +* `target` {Buffer|Uint8Array} A `Buffer` or [`Uint8Array`] to compare to * `targetStart` {Integer} The offset within `target` at which to begin comparison. **Default:** `0` * `targetEnd` {Integer} The offset with `target` at which to end comparison @@ -1037,7 +1037,7 @@ for (const pair of buf.entries()) { added: v0.11.13 --> -* `otherBuffer` {Buffer} A `Buffer` to compare to +* `otherBuffer` {Buffer} A `Buffer` or [`Uint8Array`] to compare to * Returns: {Boolean} Returns `true` if both `buf` and `otherBuffer` have exactly the same bytes, @@ -1099,7 +1099,7 @@ console.log(Buffer.allocUnsafe(3).fill('\u0222')); added: v1.5.0 --> -* `value` {String | Buffer | Integer} What to search for +* `value` {String | Buffer | Uint8Array | Integer} What to search for * `byteOffset` {Integer} Where to begin searching in `buf`. **Default:** `0` * `encoding` {String} If `value` is a string, this is its encoding. **Default:** `'utf8'` @@ -1110,8 +1110,8 @@ If `value` is: * a string, `value` is interpreted according to the character encoding in `encoding`. - * a `Buffer`, `value` will be used in its entirety. To compare a partial - `Buffer` use [`buf.slice()`]. + * a `Buffer` or [`Uint8Array`], `value` will be used in its entirety. + To compare a partial `Buffer`, use [`buf.slice()`]. * a number, `value` will be interpreted as an unsigned 8-bit integer value between `0` and `255`. @@ -1221,7 +1221,7 @@ for (const key of buf.keys()) { added: v6.0.0 --> -* `value` {String | Buffer | Integer} What to search for +* `value` {String | Buffer | Uint8Array | Integer} What to search for * `byteOffset` {Integer} Where to begin searching in `buf`. **Default:** [`buf.length`]` - 1` * `encoding` {String} If `value` is a string, this is its encoding. @@ -2313,12 +2313,12 @@ Note that this is a property on the `buffer` module returned by added: v7.1.0 --> -* `source` {Buffer} A `Buffer` instance +* `source` {Buffer|Uint8Array} A `Buffer` or `Uint8Array` instance * `fromEnc` {String} The current encoding * `toEnc` {String} To target encoding -Re-encodes the given `Buffer` instance from one character encoding to another. -Returns a new `Buffer` instance. +Re-encodes the given `Buffer` or `Uint8Array` instance from one character +encoding to another. Returns a new `Buffer` instance. Throws if the `fromEnc` or `toEnc` specify invalid character encodings or if conversion from `fromEnc` to `toEnc` is not permitted. diff --git a/lib/buffer.js b/lib/buffer.js index 557ac867e2e0fc..bc4e23512a748f 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -2,7 +2,8 @@ 'use strict'; const binding = process.binding('buffer'); -const { isArrayBuffer, isSharedArrayBuffer } = process.binding('util'); +const { isArrayBuffer, isSharedArrayBuffer, isUint8Array } = + process.binding('util'); const bindingObj = {}; const internalUtil = require('internal/util'); @@ -247,13 +248,13 @@ function fromArrayBuffer(obj, byteOffset, length) { } function fromObject(obj) { - if (obj instanceof Buffer) { + if (isUint8Array(obj)) { const b = allocate(obj.length); if (b.length === 0) return b; - obj.copy(b, 0, 0, obj.length); + Buffer.prototype.copy.call(obj, b, 0, 0, obj.length); return b; } @@ -283,8 +284,7 @@ Buffer.isBuffer = function isBuffer(b) { Buffer.compare = function compare(a, b) { - if (!(a instanceof Buffer) || - !(b instanceof Buffer)) { + if (!isUint8Array(a) || !isUint8Array(b)) { throw new TypeError('Arguments must be Buffers'); } @@ -322,9 +322,9 @@ Buffer.concat = function(list, length) { var pos = 0; for (i = 0; i < list.length; i++) { var buf = list[i]; - if (!Buffer.isBuffer(buf)) + if (!isUint8Array(buf)) throw new TypeError('"list" argument must be an Array of Buffers'); - buf.copy(buffer, pos); + Buffer.prototype.copy.call(buf, buffer, pos); pos += buf.length; } @@ -506,7 +506,7 @@ Buffer.prototype.toString = function() { Buffer.prototype.equals = function equals(b) { - if (!(b instanceof Buffer)) + if (!isUint8Array(b)) throw new TypeError('Argument must be a Buffer'); if (this === b) @@ -535,7 +535,7 @@ Buffer.prototype.compare = function compare(target, thisStart, thisEnd) { - if (!(target instanceof Buffer)) + if (!isUint8Array(target)) throw new TypeError('Argument must be a Buffer'); if (start === undefined) @@ -600,7 +600,7 @@ function bidirectionalIndexOf(buffer, val, byteOffset, encoding, dir) { return binding.indexOfString(buffer, val, byteOffset, encoding, dir); } return slowIndexOf(buffer, val, byteOffset, encoding, dir); - } else if (val instanceof Buffer) { + } else if (isUint8Array(val)) { return binding.indexOfBuffer(buffer, val, byteOffset, encoding, dir); } else if (typeof val === 'number') { return binding.indexOfNumber(buffer, val, byteOffset, dir); @@ -1033,7 +1033,7 @@ Buffer.prototype.readDoubleBE = function readDoubleBE(offset, noAssert) { function checkInt(buffer, value, offset, ext, max, min) { - if (!(buffer instanceof Buffer)) + if (!isUint8Array(buffer)) throw new TypeError('"buffer" argument must be a Buffer instance'); if (value > max || value < min) throw new TypeError('"value" argument is out of bounds'); diff --git a/lib/internal/buffer.js b/lib/internal/buffer.js index fdb34b9efdb092..a4af5f937b3692 100644 --- a/lib/internal/buffer.js +++ b/lib/internal/buffer.js @@ -8,18 +8,19 @@ const normalizeEncoding = require('internal/util').normalizeEncoding; const Buffer = require('buffer').Buffer; const icu = process.binding('icu'); +const { isUint8Array } = process.binding('util'); // Transcodes the Buffer from one encoding to another, returning a new // Buffer instance. exports.transcode = function transcode(source, fromEncoding, toEncoding) { - if (!Buffer.isBuffer(source)) + if (!isUint8Array(source)) throw new TypeError('"source" argument must be a Buffer'); if (source.length === 0) return Buffer.alloc(0); fromEncoding = normalizeEncoding(fromEncoding) || fromEncoding; toEncoding = normalizeEncoding(toEncoding) || toEncoding; const result = icu.transcode(source, fromEncoding, toEncoding); - if (Buffer.isBuffer(result)) + if (typeof result !== 'number') return result; const code = icu.icuErrName(result); diff --git a/src/node_util.cc b/src/node_util.cc index c231983e57a2df..a1387353e3d9a5 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -29,7 +29,8 @@ using v8::Value; V(isSet, IsSet) \ V(isSetIterator, IsSetIterator) \ V(isSharedArrayBuffer, IsSharedArrayBuffer) \ - V(isTypedArray, IsTypedArray) + V(isTypedArray, IsTypedArray) \ + V(isUint8Array, IsUint8Array) #define V(_, ucname) \ diff --git a/test/parallel/test-buffer-compare.js b/test/parallel/test-buffer-compare.js index c0db39a6e3c8bc..c9e50081edfc5f 100644 --- a/test/parallel/test-buffer-compare.js +++ b/test/parallel/test-buffer-compare.js @@ -6,10 +6,12 @@ const assert = require('assert'); const b = Buffer.alloc(1, 'a'); const c = Buffer.alloc(1, 'c'); const d = Buffer.alloc(2, 'aa'); +const e = new Uint8Array([ 0x61, 0x61 ]); // ASCII 'aa', same as d assert.strictEqual(b.compare(c), -1); assert.strictEqual(c.compare(d), 1); assert.strictEqual(d.compare(b), 1); +assert.strictEqual(d.compare(e), 0); assert.strictEqual(b.compare(d), -1); assert.strictEqual(b.compare(b), 0); @@ -18,6 +20,9 @@ assert.strictEqual(Buffer.compare(c, d), 1); assert.strictEqual(Buffer.compare(d, b), 1); assert.strictEqual(Buffer.compare(b, d), -1); assert.strictEqual(Buffer.compare(c, c), 0); +assert.strictEqual(Buffer.compare(e, e), 0); +assert.strictEqual(Buffer.compare(d, e), 0); +assert.strictEqual(Buffer.compare(d, b), 1); assert.strictEqual(Buffer.compare(Buffer.alloc(0), Buffer.alloc(0)), 0); assert.strictEqual(Buffer.compare(Buffer.alloc(0), Buffer.alloc(1)), -1); diff --git a/test/parallel/test-buffer-concat.js b/test/parallel/test-buffer-concat.js index 800f1055aa6222..3e7f58e55c4bc4 100644 --- a/test/parallel/test-buffer-concat.js +++ b/test/parallel/test-buffer-concat.js @@ -60,3 +60,7 @@ assert.deepStrictEqual(Buffer.concat([empty], 4096), Buffer.alloc(4096)); assert.deepStrictEqual( Buffer.concat([random10], 40), Buffer.concat([random10, Buffer.alloc(30)])); + +assert.deepStrictEqual(Buffer.concat([new Uint8Array([0x41, 0x42]), + new Uint8Array([0x43, 0x44])]), + Buffer.from('ABCD')); diff --git a/test/parallel/test-buffer-equals.js b/test/parallel/test-buffer-equals.js index 2b460c5c6aea1b..cc7c71ed487d75 100644 --- a/test/parallel/test-buffer-equals.js +++ b/test/parallel/test-buffer-equals.js @@ -12,5 +12,6 @@ assert.ok(b.equals(c)); assert.ok(!c.equals(d)); assert.ok(!d.equals(e)); assert.ok(d.equals(d)); +assert.ok(d.equals(new Uint8Array([0x61, 0x62, 0x63, 0x64, 0x65]))); assert.throws(() => Buffer.alloc(1).equals('abc')); diff --git a/test/parallel/test-buffer-indexof.js b/test/parallel/test-buffer-indexof.js index 746a2723167a60..6aab628fe9939a 100644 --- a/test/parallel/test-buffer-indexof.js +++ b/test/parallel/test-buffer-indexof.js @@ -524,3 +524,11 @@ assert.equal(0, reallyLong.lastIndexOf(pattern)); assert.strictEqual(buf.indexOf(0xff), -1); assert.strictEqual(buf.indexOf(0xffff), -1); } + +// Test that Uint8Array arguments are okay. +{ + const needle = new Uint8Array([ 0x66, 0x6f, 0x6f ]); + const haystack = Buffer.from('a foo b foo'); + assert.strictEqual(haystack.indexOf(needle), 2); + assert.strictEqual(haystack.lastIndexOf(needle), haystack.length - 3); +} diff --git a/test/parallel/test-icu-transcode.js b/test/parallel/test-icu-transcode.js index c099e754ca55d6..82877da71cf1d8 100644 --- a/test/parallel/test-icu-transcode.js +++ b/test/parallel/test-icu-transcode.js @@ -62,3 +62,11 @@ assert.deepStrictEqual( assert.deepStrictEqual( buffer.transcode(Buffer.from('hä', 'latin1'), 'latin1', 'utf16le'), Buffer.from('hä', 'utf16le')); + +// Test that Uint8Array arguments are okay. +{ + const uint8array = new Uint8Array([...Buffer.from('hä', 'latin1')]); + assert.deepStrictEqual( + buffer.transcode(uint8array, 'latin1', 'utf16le'), + Buffer.from('hä', 'utf16le')); +} From 3b4c3e788766c2e78f1f5f8f786c3a637a04191d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 13 Dec 2016 03:50:20 +0100 Subject: [PATCH 2/3] [squash] use binding.copy directly --- lib/buffer.js | 7 +++++-- src/node_buffer.cc | 18 +++++++++--------- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/lib/buffer.js b/lib/buffer.js index bc4e23512a748f..7db593c2ff2d84 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -254,7 +254,7 @@ function fromObject(obj) { if (b.length === 0) return b; - Buffer.prototype.copy.call(obj, b, 0, 0, obj.length); + binding.copy(obj, b, 0, 0, obj.length); return b; } @@ -324,7 +324,7 @@ Buffer.concat = function(list, length) { var buf = list[i]; if (!isUint8Array(buf)) throw new TypeError('"list" argument must be an Array of Buffers'); - Buffer.prototype.copy.call(buf, buffer, pos); + binding.copy(buf, buffer, pos); pos += buf.length; } @@ -491,6 +491,9 @@ function slowToString(encoding, start, end) { } } +Buffer.prototype.copy = function(target, targetStart, sourceStart, sourceEnd) { + return binding.copy(this, target, targetStart, sourceStart, sourceEnd); +}; Buffer.prototype.toString = function() { let result; diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 0e96fd6deed8d3..1f86cf8b3f7ae5 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -519,23 +519,24 @@ void Base64Slice(const FunctionCallbackInfo& args) { } -// bytesCopied = buffer.copy(target[, targetStart][, sourceStart][, sourceEnd]); +// bytesCopied = copy(buffer, target[, targetStart][, sourceStart][, sourceEnd]) void Copy(const FunctionCallbackInfo &args) { Environment* env = Environment::GetCurrent(args); - THROW_AND_RETURN_UNLESS_BUFFER(env, args.This()); THROW_AND_RETURN_UNLESS_BUFFER(env, args[0]); - Local target_obj = args[0].As(); - SPREAD_BUFFER_ARG(args.This(), ts_obj); + THROW_AND_RETURN_UNLESS_BUFFER(env, args[1]); + Local buffer_obj = args[0].As(); + Local target_obj = args[1].As(); + SPREAD_BUFFER_ARG(buffer_obj, ts_obj); SPREAD_BUFFER_ARG(target_obj, target); size_t target_start; size_t source_start; size_t source_end; - THROW_AND_RETURN_IF_OOB(ParseArrayIndex(args[1], 0, &target_start)); - THROW_AND_RETURN_IF_OOB(ParseArrayIndex(args[2], 0, &source_start)); - THROW_AND_RETURN_IF_OOB(ParseArrayIndex(args[3], ts_obj_length, &source_end)); + THROW_AND_RETURN_IF_OOB(ParseArrayIndex(args[2], 0, &target_start)); + THROW_AND_RETURN_IF_OOB(ParseArrayIndex(args[3], 0, &source_start)); + THROW_AND_RETURN_IF_OOB(ParseArrayIndex(args[4], ts_obj_length, &source_end)); // Copy 0 bytes; we're done if (target_start >= target_length || source_start >= source_end) @@ -1203,8 +1204,6 @@ void SetupBufferJS(const FunctionCallbackInfo& args) { env->SetMethod(proto, "ucs2Write", Ucs2Write); env->SetMethod(proto, "utf8Write", Utf8Write); - env->SetMethod(proto, "copy", Copy); - if (auto zero_fill_field = env->isolate_data()->zero_fill_field()) { CHECK(args[1]->IsObject()); auto binding_object = args[1].As(); @@ -1227,6 +1226,7 @@ void Initialize(Local target, env->SetMethod(target, "createFromString", CreateFromString); env->SetMethod(target, "byteLengthUtf8", ByteLengthUtf8); + env->SetMethod(target, "copy", Copy); env->SetMethod(target, "compare", Compare); env->SetMethod(target, "compareOffset", CompareOffset); env->SetMethod(target, "fill", Fill); From d2e93cb406831cbe5c73936f5216c1f2f4006222 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 13 Dec 2016 18:44:24 +0100 Subject: [PATCH 3/3] [squash] update typecheck error messages --- lib/buffer.js | 18 +++++++++++------- lib/internal/buffer.js | 2 +- test/parallel/test-buffer-concat.js | 3 ++- test/parallel/test-icu-transcode.js | 2 +- 4 files changed, 15 insertions(+), 10 deletions(-) diff --git a/lib/buffer.js b/lib/buffer.js index 7db593c2ff2d84..295a4edb7b8dbc 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -285,7 +285,7 @@ Buffer.isBuffer = function isBuffer(b) { Buffer.compare = function compare(a, b) { if (!isUint8Array(a) || !isUint8Array(b)) { - throw new TypeError('Arguments must be Buffers'); + throw new TypeError('Arguments must be Buffers or Uint8Arrays'); } if (a === b) { @@ -302,10 +302,13 @@ Buffer.isEncoding = function(encoding) { }; Buffer[internalUtil.kIsEncodingSymbol] = Buffer.isEncoding; +const kConcatErrMsg = '"list" argument must be an Array ' + + 'of Buffer or Uint8Array instances'; + Buffer.concat = function(list, length) { var i; if (!Array.isArray(list)) - throw new TypeError('"list" argument must be an Array of Buffers'); + throw new TypeError(kConcatErrMsg); if (list.length === 0) return new FastBuffer(); @@ -323,7 +326,7 @@ Buffer.concat = function(list, length) { for (i = 0; i < list.length; i++) { var buf = list[i]; if (!isUint8Array(buf)) - throw new TypeError('"list" argument must be an Array of Buffers'); + throw new TypeError(kConcatErrMsg); binding.copy(buf, buffer, pos); pos += buf.length; } @@ -510,7 +513,7 @@ Buffer.prototype.toString = function() { Buffer.prototype.equals = function equals(b) { if (!isUint8Array(b)) - throw new TypeError('Argument must be a Buffer'); + throw new TypeError('Argument must be a Buffer or Uint8Array'); if (this === b) return true; @@ -539,7 +542,7 @@ Buffer.prototype.compare = function compare(target, thisEnd) { if (!isUint8Array(target)) - throw new TypeError('Argument must be a Buffer'); + throw new TypeError('Argument must be a Buffer or Uint8Array'); if (start === undefined) start = 0; @@ -609,7 +612,8 @@ function bidirectionalIndexOf(buffer, val, byteOffset, encoding, dir) { return binding.indexOfNumber(buffer, val, byteOffset, dir); } - throw new TypeError('"val" argument must be string, number or Buffer'); + throw new TypeError('"val" argument must be string, number, Buffer ' + + 'or Uint8Array'); } @@ -1037,7 +1041,7 @@ Buffer.prototype.readDoubleBE = function readDoubleBE(offset, noAssert) { function checkInt(buffer, value, offset, ext, max, min) { if (!isUint8Array(buffer)) - throw new TypeError('"buffer" argument must be a Buffer instance'); + throw new TypeError('"buffer" argument must be a Buffer or Uint8Array'); if (value > max || value < min) throw new TypeError('"value" argument is out of bounds'); if (offset + ext > buffer.length) diff --git a/lib/internal/buffer.js b/lib/internal/buffer.js index a4af5f937b3692..744bf2dcab6ec4 100644 --- a/lib/internal/buffer.js +++ b/lib/internal/buffer.js @@ -14,7 +14,7 @@ const { isUint8Array } = process.binding('util'); // Buffer instance. exports.transcode = function transcode(source, fromEncoding, toEncoding) { if (!isUint8Array(source)) - throw new TypeError('"source" argument must be a Buffer'); + throw new TypeError('"source" argument must be a Buffer or Uint8Array'); if (source.length === 0) return Buffer.alloc(0); fromEncoding = normalizeEncoding(fromEncoding) || fromEncoding; diff --git a/test/parallel/test-buffer-concat.js b/test/parallel/test-buffer-concat.js index 3e7f58e55c4bc4..1239c9e8e18824 100644 --- a/test/parallel/test-buffer-concat.js +++ b/test/parallel/test-buffer-concat.js @@ -35,7 +35,8 @@ function assertWrongList(value) { Buffer.concat(value); }, function(err) { return err instanceof TypeError && - err.message === '"list" argument must be an Array of Buffers'; + err.message === '"list" argument must be an Array of Buffer ' + + 'or Uint8Array instances'; }); } diff --git a/test/parallel/test-icu-transcode.js b/test/parallel/test-icu-transcode.js index 82877da71cf1d8..fc588b220f5366 100644 --- a/test/parallel/test-icu-transcode.js +++ b/test/parallel/test-icu-transcode.js @@ -40,7 +40,7 @@ for (const test in tests) { assert.throws( () => buffer.transcode(null, 'utf8', 'ascii'), - /^TypeError: "source" argument must be a Buffer$/ + /^TypeError: "source" argument must be a Buffer or Uint8Array$/ ); assert.throws(