From 1e802539b2811f5090281cfc8041f21120c2c3c6 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Sat, 13 Jan 2018 00:30:28 -0500 Subject: [PATCH] buffer: throw when filling with empty buffers Prior to this commit, Node would enter an infinite loop when attempting to fill a non-zero length buffer with a zero length buffer. This commit introduces a thrown exception in this scenario. PR-URL: https://github.com/nodejs/node/pull/18129 Fixes: https://github.com/nodejs/node/issues/18128 Reviewed-By: Richard Lau Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: James M Snell Reviewed-By: Luigi Pinca Reviewed-By: Franziska Hinkelmann --- doc/api/buffer.md | 8 ++++++++ src/node_buffer.cc | 14 +++++++------- test/parallel/test-buffer-alloc.js | 7 +++++++ 3 files changed, 22 insertions(+), 7 deletions(-) diff --git a/doc/api/buffer.md b/doc/api/buffer.md index f1d3cab6664886..767d7e2aa8a30c 100644 --- a/doc/api/buffer.md +++ b/doc/api/buffer.md @@ -519,6 +519,10 @@ changes: pr-url: https://github.com/nodejs/node/pull/17427 description: Specifying an invalid string for `fill` triggers a thrown exception. + - version: REPLACEME + pr-url: https://github.com/nodejs/node/pull/18129 + description: Attempting to fill a non-zero length buffer with a zero length + buffer triggers a thrown exception. --> * `size` {integer} The desired length of the new `Buffer`. @@ -1231,6 +1235,10 @@ changes: pr-url: https://github.com/nodejs/node/pull/17427 description: Specifying an invalid string for `value` triggers a thrown exception. + - version: REPLACEME + pr-url: https://github.com/nodejs/node/pull/18129 + description: Attempting to fill a non-zero length buffer with a zero length + buffer triggers a thrown exception. --> * `value` {string|Buffer|integer} The value to fill `buf` with. diff --git a/src/node_buffer.cc b/src/node_buffer.cc index f0be320d61f9b2..2705fc1c07881e 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -642,13 +642,6 @@ void Fill(const FunctionCallbackInfo& args) { str_obj, enc, nullptr); - // This check is also needed in case Write() returns that no bytes could - // 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(-1); } start_fill: @@ -656,6 +649,13 @@ void Fill(const FunctionCallbackInfo& args) { if (str_length >= fill_length) return; + // If str_length is zero, then either an empty buffer was provided, or Write() + // indicated that no bytes could be written. If no bytes could be written, + // then return -1 because the fill value 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(-1); size_t in_there = str_length; char* ptr = ts_obj_data + start + str_length; diff --git a/test/parallel/test-buffer-alloc.js b/test/parallel/test-buffer-alloc.js index 97392ed51cf843..58792788d64818 100644 --- a/test/parallel/test-buffer-alloc.js +++ b/test/parallel/test-buffer-alloc.js @@ -1024,3 +1024,10 @@ common.expectsError(() => { code: 'ERR_INVALID_ARG_VALUE', type: TypeError }); + +common.expectsError(() => { + Buffer.alloc(1, Buffer.alloc(0)); +}, { + code: 'ERR_INVALID_ARG_VALUE', + type: TypeError +});