From 2ba60100820afe9b01f0d3dcee46453ce26db7e0 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 21 Oct 2018 06:44:48 +0200 Subject: [PATCH] buffer: fix crash for invalid index types 2555cb4a4049dc4c41d8a2f4ce50909cc0a12a4a introduced a crash when a non-number value was passed to `ParseArrayIndex()`. We do not always have JS typechecking for that in place, though. This returns back to the previous behavior of coercing values to integers, which is certainly questionable. Refs: https://github.com/nodejs/node/pull/22129 Fixes: https://github.com/nodejs/node/issues/23668 --- src/node_buffer.cc | 62 ++++++++++++++++++------------- test/parallel/test-buffer-copy.js | 26 +++++++++++++ 2 files changed, 62 insertions(+), 26 deletions(-) diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 8642477db4d927..bf79e66cb19b5d 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -40,16 +40,18 @@ #define THROW_AND_RETURN_IF_OOB(r) \ do { \ - if (!(r)) return node::THROW_ERR_INDEX_OUT_OF_RANGE(env); \ - } while (0) + if ((r).IsNothing()) return; \ + if (!(r).FromJust()) \ + return node::THROW_ERR_INDEX_OUT_OF_RANGE(env); \ + } while (0) \ -#define SLICE_START_END(start_arg, end_arg, end_max) \ +#define SLICE_START_END(env, start_arg, end_arg, end_max) \ size_t start; \ size_t end; \ - THROW_AND_RETURN_IF_OOB(ParseArrayIndex(start_arg, 0, &start)); \ - THROW_AND_RETURN_IF_OOB(ParseArrayIndex(end_arg, end_max, &end)); \ + THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, start_arg, 0, &start)); \ + THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, end_arg, end_max, &end)); \ if (end < start) end = start; \ - THROW_AND_RETURN_IF_OOB(end <= end_max); \ + THROW_AND_RETURN_IF_OOB(Just(end <= end_max)); \ size_t length = end - start; namespace node { @@ -76,9 +78,11 @@ using v8::EscapableHandleScope; using v8::FunctionCallbackInfo; using v8::Integer; using v8::Isolate; +using v8::Just; using v8::Local; using v8::Maybe; using v8::MaybeLocal; +using v8::Nothing; using v8::Object; using v8::String; using v8::Uint32; @@ -161,29 +165,32 @@ void CallbackInfo::WeakCallback(Isolate* isolate) { } -// Parse index for external array data. -inline MUST_USE_RESULT bool ParseArrayIndex(Local arg, - size_t def, - size_t* ret) { +// Parse index for external array data. An empty Maybe indicates +// a pending exception. `false` indicates that the index is out-of-bounds. +inline MUST_USE_RESULT Maybe ParseArrayIndex(Environment* env, + Local arg, + size_t def, + size_t* ret) { if (arg->IsUndefined()) { *ret = def; - return true; + return Just(true); } - CHECK(arg->IsNumber()); - int64_t tmp_i = arg.As()->Value(); + int64_t tmp_i; + if (!arg->IntegerValue(env->context()).To(&tmp_i)) + return Nothing(); if (tmp_i < 0) - return false; + return Just(false); // Check that the result fits in a size_t. const uint64_t kSizeMax = static_cast(static_cast(-1)); // coverity[pointless_expression] if (static_cast(tmp_i) > kSizeMax) - return false; + return Just(false); *ret = static_cast(tmp_i); - return true; + return Just(true); } } // anonymous namespace @@ -452,7 +459,7 @@ void StringSlice(const FunctionCallbackInfo& args) { if (ts_obj_length == 0) return args.GetReturnValue().SetEmptyString(); - SLICE_START_END(args[0], args[1], ts_obj_length) + SLICE_START_END(env, args[0], args[1], ts_obj_length) Local error; MaybeLocal ret = @@ -485,9 +492,10 @@ void Copy(const FunctionCallbackInfo &args) { size_t source_start; size_t 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)); + THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[2], 0, &target_start)); + THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[3], 0, &source_start)); + THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[4], ts_obj_length, + &source_end)); // Copy 0 bytes; we're done if (target_start >= target_length || source_start >= source_end) @@ -617,13 +625,13 @@ void StringWrite(const FunctionCallbackInfo& args) { size_t offset; size_t max_length; - THROW_AND_RETURN_IF_OOB(ParseArrayIndex(args[1], 0, &offset)); + THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[1], 0, &offset)); if (offset > ts_obj_length) { return node::THROW_ERR_BUFFER_OUT_OF_BOUNDS( env, "\"offset\" is outside of buffer bounds"); } - THROW_AND_RETURN_IF_OOB(ParseArrayIndex(args[2], ts_obj_length - offset, + THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[2], ts_obj_length - offset, &max_length)); max_length = MIN(ts_obj_length - offset, max_length); @@ -678,10 +686,12 @@ void CompareOffset(const FunctionCallbackInfo &args) { size_t source_end; size_t target_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], target_length, &target_end)); - THROW_AND_RETURN_IF_OOB(ParseArrayIndex(args[5], ts_obj_length, &source_end)); + THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[2], 0, &target_start)); + THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[3], 0, &source_start)); + THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[4], target_length, + &target_end)); + THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[5], ts_obj_length, + &source_end)); if (source_start > ts_obj_length) return node::THROW_ERR_INDEX_OUT_OF_RANGE(env); diff --git a/test/parallel/test-buffer-copy.js b/test/parallel/test-buffer-copy.js index 8ede5101463b05..f3c351f09aead3 100644 --- a/test/parallel/test-buffer-copy.js +++ b/test/parallel/test-buffer-copy.js @@ -18,6 +18,17 @@ let cntr = 0; } } +{ + // Current behavior is to coerce values to integers. + b.fill(++cntr); + c.fill(++cntr); + const copied = b.copy(c, '0', '0', '512'); + assert.strictEqual(copied, 512); + for (let i = 0; i < c.length; i++) { + assert.strictEqual(c[i], b[i]); + } +} + { // copy c into b, without specifying sourceEnd b.fill(++cntr); @@ -144,3 +155,18 @@ assert.strictEqual(b.copy(c, 512, 0, 10), 0); assert.strictEqual(c[i], e[i]); } } + +// https://github.com/nodejs/node/issues/23668: Do not crash for invalid input. +c.fill('c'); +b.copy(c, 'not a valid offset'); +// Make sure this acted like a regular copy with `0` offset. +assert.deepStrictEqual(c, b.slice(0, c.length)); + +{ + c.fill('C'); + assert.throws(() => { + b.copy(c, { [Symbol.toPrimitive]() { throw new Error('foo'); } }); + }, /foo/); + // No copying took place: + assert.deepStrictEqual(c.toString(), 'C'.repeat(c.length)); +}