From cd174df353e78cde9181299adbf501a4a694dee8 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Sat, 2 Dec 2017 10:04:50 -0500 Subject: [PATCH] buffer: throw on failed fill attempts If fill() attempts to write a string to a buffer, but fails silently, then uninitialized memory could be leaked. This commit causes fill() to throw if the string write operation fails. Refs: https://github.com/nodejs/node/issues/17423 PR-URL: https://github.com/nodejs/node/pull/17427 Reviewed-By: James M Snell Reviewed-By: Ruben Bridgewater --- doc/api/buffer.md | 12 +++++++++-- lib/buffer.js | 6 +++++- src/node_buffer.cc | 9 ++++---- test/parallel/test-buffer-alloc.js | 21 ++++++++++-------- test/parallel/test-buffer-fill.js | 34 ++++++++++++++++++------------ 5 files changed, 52 insertions(+), 30 deletions(-) diff --git a/doc/api/buffer.md b/doc/api/buffer.md index 98b5130f3b64a0..aab6840f7643f2 100644 --- a/doc/api/buffer.md +++ b/doc/api/buffer.md @@ -515,6 +515,10 @@ changes: pr-url: https://github.com/nodejs/node/pull/17428 description: Specifying an invalid string for `fill` now results in a zero-filled buffer. + - version: REPLACEME + pr-url: https://github.com/nodejs/node/pull/17427 + description: Specifying an invalid string for `fill` triggers a thrown + exception. --> * `size` {integer} The desired length of the new `Buffer`. @@ -1225,6 +1229,10 @@ changes: - version: v5.7.0 pr-url: https://github.com/nodejs/node/pull/4935 description: The `encoding` parameter is supported now. + - version: REPLACEME + pr-url: https://github.com/nodejs/node/pull/17427 + description: Specifying an invalid string for `value` triggers a thrown + exception. --> * `value` {string|Buffer|integer} The value to fill `buf` with. @@ -1260,7 +1268,7 @@ console.log(Buffer.allocUnsafe(3).fill('\u0222')); ``` If `value` contains invalid characters, it is truncated; if no valid -fill data remains, no filling is performed: +fill data remains, an exception is thrown: ```js const buf = Buffer.allocUnsafe(5); @@ -1268,7 +1276,7 @@ const buf = Buffer.allocUnsafe(5); console.log(buf.fill('a')); // Prints: console.log(buf.fill('aazz', 'hex')); -// Prints: +// Throws a exception. console.log(buf.fill('zz', 'hex')); ``` diff --git a/lib/buffer.js b/lib/buffer.js index 8899d260ad29e0..40aaf37e09d600 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -892,8 +892,12 @@ function fill_(buf, val, start, end, encoding) { start = start >>> 0; end = end === undefined ? buf.length : end >>> 0; + const fillLength = bindingFill(buf, val, start, end, encoding); - return bindingFill(buf, val, start, end, encoding); + if (fillLength === -1) + throw new errors.TypeError('ERR_INVALID_ARG_VALUE', 'value', val); + + return fillLength; } diff --git a/src/node_buffer.cc b/src/node_buffer.cc index ca2c6a89cb012a..3a72b4f6c54553 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -643,11 +643,12 @@ void Fill(const FunctionCallbackInfo& args) { enc, nullptr); // This check is also needed in case Write() returns that no bytes could - // be written. - // TODO(trevnorris): Should this throw? Because of the string length was - // greater than 0 but couldn't be written then the string was invalid. + // be written. If no bytes could be written, then return -1 because the + // string is invalid. This will trigger a throw in JavaScript. Silently + // failing should be avoided because it can lead to buffers with unexpected + // contents. if (str_length == 0) - return args.GetReturnValue().Set(0); + return args.GetReturnValue().Set(-1); } start_fill: diff --git a/test/parallel/test-buffer-alloc.js b/test/parallel/test-buffer-alloc.js index 73dfce3624d9f8..f3de253c14e9da 100644 --- a/test/parallel/test-buffer-alloc.js +++ b/test/parallel/test-buffer-alloc.js @@ -1006,13 +1006,16 @@ assert.strictEqual(Buffer.prototype.toLocaleString, Buffer.prototype.toString); 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}`); -} +common.expectsError(() => { + Buffer.alloc(0x1000, 'This is not correctly encoded', 'hex'); +}, { + code: 'ERR_INVALID_ARG_VALUE', + type: TypeError +}); -{ - const buf = Buffer.alloc(0x1000, 'c', 'hex'); - assert(buf.every((byte) => byte === 0), `Buffer was not zeroed out: ${buf}`); -} +common.expectsError(() => { + Buffer.alloc(0x1000, 'c', 'hex'); +}, { + code: 'ERR_INVALID_ARG_VALUE', + type: TypeError +}); diff --git a/test/parallel/test-buffer-fill.js b/test/parallel/test-buffer-fill.js index 681207e95ef382..ad724f0b2b1903 100644 --- a/test/parallel/test-buffer-fill.js +++ b/test/parallel/test-buffer-fill.js @@ -134,20 +134,23 @@ testBufs('61c8b462c8b563c8b6', 4, -1, 'hex'); testBufs('61c8b462c8b563c8b6', 4, 1, 'hex'); testBufs('61c8b462c8b563c8b6', 12, 1, 'hex'); -{ +common.expectsError(() => { const buf = Buffer.allocUnsafe(SIZE); - assert.doesNotThrow(() => { - // Make sure this operation doesn't go on forever. - buf.fill('yKJh', 'hex'); - }); -} -{ + buf.fill('yKJh', 'hex'); +}, { + code: 'ERR_INVALID_ARG_VALUE', + type: TypeError +}); + +common.expectsError(() => { const buf = Buffer.allocUnsafe(SIZE); - assert.doesNotThrow(() => { - buf.fill('\u0222', 'hex'); - }); -} + + buf.fill('\u0222', 'hex'); +}, { + code: 'ERR_INVALID_ARG_VALUE', + type: TypeError +}); // BASE64 testBufs('YWJj', 'ucs2'); @@ -469,8 +472,11 @@ assert.strictEqual( Buffer.allocUnsafeSlow(16).fill('Љ', 'utf8').toString('utf8'), 'Љ'.repeat(8)); -{ +common.expectsError(() => { const buf = Buffer.from('a'.repeat(1000)); + buf.fill('This is not correctly encoded', 'hex'); - assert.strictEqual(buf.toString(), 'a'.repeat(1000)); -} +}, { + code: 'ERR_INVALID_ARG_VALUE', + type: TypeError +});