buffer: fix crash for invalid index types

2555cb4 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: #22129 Fixes: #23668
nodejs · MylesBorins · Oct 30, 2018 · Oct 21, 2018 · Oct 18, 2018 · Oct 21, 2018
commit 2ba60100820afe9b01f0d3dcee46453ce26db7e0
diff --git 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<Value> 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<bool> ParseArrayIndex(Environment* env,
+                                                   Local<Value> 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<Integer>()->Value();
+  int64_t tmp_i;
+  if (!arg->IntegerValue(env->context()).To(&tmp_i))
+    return Nothing<bool>();
 
   if (tmp_i < 0)
-    return false;
+    return Just(false);
 
   // Check that the result fits in a size_t.
   const uint64_t kSizeMax = static_cast<uint64_t>(static_cast<size_t>(-1));
   // coverity[pointless_expression]
   if (static_cast<uint64_t>(tmp_i) > kSizeMax)
-    return false;
+    return Just(false);
 
   *ret = static_cast<size_t>(tmp_i);
-  return true;
+  return Just(true);
 }
 
 }  // anonymous namespace
@@ -452,7 +459,7 @@ void StringSlice(const FunctionCallbackInfo<Value>& 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<Value> error;
   MaybeLocal<Value> ret =
@@ -485,9 +492,10 @@ void Copy(const FunctionCallbackInfo<Value> &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<Value>& 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<Value> &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
@@ -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));
+}