From b05ef978d32ef981aaceb717dc06777eb1e77365 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 2 Dec 2017 16:28:35 +0100 Subject: [PATCH] buffer: zero-fill buffer allocated with invalid content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Zero-fill when `Buffer.alloc()` receives invalid fill data. A solution like https://github.com/nodejs/node/pull/17427 which switches to throwing makes sense, but is likely a breaking change. This suggestion leaves the behaviour of `buffer.fill()` untouched, since any change to it would be a breaking change, and lets `Buffer.alloc()` check whether any filling took place or not. PR-URL: https://github.com/nodejs/node/pull/17428 Backport-PR-URL: https://github.com/nodejs/node/pull/17467 Refs: https://github.com/nodejs/node/pull/17427 Refs: https://github.com/nodejs/node/issues/17423 Reviewed-By: James M Snell Reviewed-By: Colin Ihrig Reviewed-By: Anatoli Papirovski Reviewed-By: Ali Ijaz Sheikh Reviewed-By: Tobias Nießen Reviewed-By: Luigi Pinca Reviewed-By: Michaël Zasso --- doc/api/buffer.md | 7 ++++++- lib/buffer.js | 25 +++++++++++++++---------- src/node_buffer.cc | 8 ++++++-- test/parallel/test-buffer-alloc.js | 11 +++++++++++ test/parallel/test-buffer-fill.js | 6 ++++++ 5 files changed, 44 insertions(+), 13 deletions(-) diff --git a/doc/api/buffer.md b/doc/api/buffer.md index 86236891ccc970..c9f975770bc6ae 100644 --- a/doc/api/buffer.md +++ b/doc/api/buffer.md @@ -510,6 +510,11 @@ console.log(buf2.toString()); ### Class Method: Buffer.alloc(size[, fill[, encoding]]) * `size` {integer} The desired length of the new `Buffer`. @@ -1253,7 +1258,7 @@ Example: Fill a `Buffer` with a two-byte character console.log(Buffer.allocUnsafe(3).fill('\u0222')); ``` -If `value` is contains invalid characters, it is truncated; if no valid +If `value` contains invalid characters, it is truncated; if no valid fill data remains, no filling is performed: ```js diff --git a/lib/buffer.js b/lib/buffer.js index 251b24cceede42..6d28e84bc7ff3d 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -238,7 +238,9 @@ Buffer.alloc = function(size, fill, encoding) { // be interpreted as a start offset. if (typeof encoding !== 'string') encoding = undefined; - return createUnsafeBuffer(size).fill(fill, encoding); + const ret = createUnsafeBuffer(size); + if (fill_(ret, fill, encoding) > 0) + return ret; } return new FastBuffer(size); }; @@ -796,15 +798,20 @@ Buffer.prototype.includes = function includes(val, byteOffset, encoding) { // buffer.fill(buffer[, offset[, end]]) // buffer.fill(string[, offset[, end]][, encoding]) Buffer.prototype.fill = function fill(val, start, end, encoding) { + fill_(this, val, start, end, encoding); + return this; +}; + +function fill_(buf, val, start, end, encoding) { // Handle string cases: if (typeof val === 'string') { if (typeof start === 'string') { encoding = start; start = 0; - end = this.length; + end = buf.length; } else if (typeof end === 'string') { encoding = end; - end = this.length; + end = buf.length; } if (encoding !== undefined && typeof encoding !== 'string') { @@ -832,19 +839,17 @@ Buffer.prototype.fill = function fill(val, start, end, encoding) { } // Invalid ranges are not set to a default, so can range check early. - if (start < 0 || end > this.length) + if (start < 0 || end > buf.length) throw new RangeError('Out of range index'); if (end <= start) - return this; + return 0; start = start >>> 0; - end = end === undefined ? this.length : end >>> 0; + end = end === undefined ? buf.length : end >>> 0; - binding.fill(this, val, start, end, encoding); - - return this; -}; + return binding.fill(buf, val, start, end, encoding); +} Buffer.prototype.write = function(string, offset, length, encoding) { diff --git a/src/node_buffer.cc b/src/node_buffer.cc index b39bbbb5d28265..b93ec413d90b77 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -591,6 +591,8 @@ void Fill(const FunctionCallbackInfo& args) { THROW_AND_RETURN_IF_OOB(start <= end); THROW_AND_RETURN_IF_OOB(fill_length + start <= ts_obj_length); + args.GetReturnValue().Set(static_cast(fill_length)); + // First check if Buffer has been passed. if (Buffer::HasInstance(args[1])) { SPREAD_BUFFER_ARG(args[1], fill_obj); @@ -612,8 +614,10 @@ void Fill(const FunctionCallbackInfo& args) { enc == UTF8 ? str_obj->Utf8Length() : enc == UCS2 ? str_obj->Length() * sizeof(uint16_t) : str_obj->Length(); - if (str_length == 0) + if (str_length == 0) { + args.GetReturnValue().Set(0); return; + } // Can't use StringBytes::Write() in all cases. For example if attempting // to write a two byte character into a one byte Buffer. @@ -643,7 +647,7 @@ void Fill(const FunctionCallbackInfo& args) { // TODO(trevnorris): Should this throw? Because of the string length was // greater than 0 but couldn't be written then the string was invalid. if (str_length == 0) - return; + return args.GetReturnValue().Set(0); } start_fill: diff --git a/test/parallel/test-buffer-alloc.js b/test/parallel/test-buffer-alloc.js index faa6b2cd1e8bbd..6d7e5581ad665d 100644 --- a/test/parallel/test-buffer-alloc.js +++ b/test/parallel/test-buffer-alloc.js @@ -993,3 +993,14 @@ assert.strictEqual(Buffer.prototype.toLocaleString, Buffer.prototype.toString); const buf = Buffer.from('test'); assert.strictEqual(buf.toLocaleString(), buf.toString()); } + +{ + // Ref: https://github.com/nodejs/node/issues/17423 + const buf = Buffer.alloc(0x1000, 'This is not correctly encoded', 'hex'); + assert(buf.every((byte) => byte === 0), `Buffer was not zeroed out: ${buf}`); +} + +{ + const buf = Buffer.alloc(0x1000, 'c', 'hex'); + assert(buf.every((byte) => byte === 0), `Buffer was not zeroed out: ${buf}`); +} diff --git a/test/parallel/test-buffer-fill.js b/test/parallel/test-buffer-fill.js index 5278cb9b61b03f..2ca79170538a87 100644 --- a/test/parallel/test-buffer-fill.js +++ b/test/parallel/test-buffer-fill.js @@ -439,3 +439,9 @@ assert.strictEqual( assert.strictEqual( Buffer.allocUnsafeSlow(16).fill('Љ', 'utf8').toString('utf8'), 'Љ'.repeat(8)); + +{ + const buf = Buffer.from('a'.repeat(1000)); + buf.fill('This is not correctly encoded', 'hex'); + assert.strictEqual(buf.toString(), 'a'.repeat(1000)); +}