Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 7dc4c3b

Browse filesBrowse files
addaleaxtargos
addaleax
authored and
targos
committed
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 PR-URL: #25154 Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent e00b326 commit 7dc4c3b
Copy full SHA for 7dc4c3b

File tree

Expand file treeCollapse file tree

2 files changed

+60
-25
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

2 files changed

+60
-25
lines changed
Open diff view settings
Collapse file

‎src/node_buffer.cc‎

Copy file name to clipboardExpand all lines: src/node_buffer.cc
+34-25Lines changed: 34 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -40,17 +40,18 @@
4040

4141
#define THROW_AND_RETURN_IF_OOB(r) \
4242
do { \
43-
if (!(r)) \
43+
if ((r).IsNothing()) return; \
44+
if (!(r).FromJust()) \
4445
return node::THROW_ERR_OUT_OF_RANGE(env, "Index out of range"); \
4546
} while (0) \
4647

47-
#define SLICE_START_END(start_arg, end_arg, end_max) \
48+
#define SLICE_START_END(env, start_arg, end_arg, end_max) \
4849
size_t start; \
4950
size_t end; \
50-
THROW_AND_RETURN_IF_OOB(ParseArrayIndex(start_arg, 0, &start)); \
51-
THROW_AND_RETURN_IF_OOB(ParseArrayIndex(end_arg, end_max, &end)); \
51+
THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, start_arg, 0, &start)); \
52+
THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, end_arg, end_max, &end)); \
5253
if (end < start) end = start; \
53-
THROW_AND_RETURN_IF_OOB(end <= end_max); \
54+
THROW_AND_RETURN_IF_OOB(Just(end <= end_max)); \
5455
size_t length = end - start;
5556

5657
namespace node {
@@ -75,9 +76,11 @@ using v8::EscapableHandleScope;
7576
using v8::FunctionCallbackInfo;
7677
using v8::Integer;
7778
using v8::Isolate;
79+
using v8::Just;
7880
using v8::Local;
7981
using v8::Maybe;
8082
using v8::MaybeLocal;
83+
using v8::Nothing;
8184
using v8::Object;
8285
using v8::String;
8386
using v8::Uint32;
@@ -160,29 +163,32 @@ void CallbackInfo::WeakCallback(Isolate* isolate) {
160163
}
161164

162165

163-
// Parse index for external array data.
164-
inline MUST_USE_RESULT bool ParseArrayIndex(Local<Value> arg,
165-
size_t def,
166-
size_t* ret) {
166+
// Parse index for external array data. An empty Maybe indicates
167+
// a pending exception. `false` indicates that the index is out-of-bounds.
168+
inline MUST_USE_RESULT Maybe<bool> ParseArrayIndex(Environment* env,
169+
Local<Value> arg,
170+
size_t def,
171+
size_t* ret) {
167172
if (arg->IsUndefined()) {
168173
*ret = def;
169-
return true;
174+
return Just(true);
170175
}
171176

172-
CHECK(arg->IsNumber());
173-
int64_t tmp_i = arg.As<Integer>()->Value();
177+
int64_t tmp_i;
178+
if (!arg->IntegerValue(env->context()).To(&tmp_i))
179+
return Nothing<bool>();
174180

175181
if (tmp_i < 0)
176-
return false;
182+
return Just(false);
177183

178184
// Check that the result fits in a size_t.
179185
const uint64_t kSizeMax = static_cast<uint64_t>(static_cast<size_t>(-1));
180186
// coverity[pointless_expression]
181187
if (static_cast<uint64_t>(tmp_i) > kSizeMax)
182-
return false;
188+
return Just(false);
183189

184190
*ret = static_cast<size_t>(tmp_i);
185-
return true;
191+
return Just(true);
186192
}
187193

188194
} // anonymous namespace
@@ -465,7 +471,7 @@ void StringSlice(const FunctionCallbackInfo<Value>& args) {
465471
if (ts_obj_length == 0)
466472
return args.GetReturnValue().SetEmptyString();
467473

468-
SLICE_START_END(args[0], args[1], ts_obj_length)
474+
SLICE_START_END(env, args[0], args[1], ts_obj_length)
469475

470476
Local<Value> error;
471477
MaybeLocal<Value> ret =
@@ -498,9 +504,10 @@ void Copy(const FunctionCallbackInfo<Value> &args) {
498504
size_t source_start;
499505
size_t source_end;
500506

501-
THROW_AND_RETURN_IF_OOB(ParseArrayIndex(args[2], 0, &target_start));
502-
THROW_AND_RETURN_IF_OOB(ParseArrayIndex(args[3], 0, &source_start));
503-
THROW_AND_RETURN_IF_OOB(ParseArrayIndex(args[4], ts_obj_length, &source_end));
507+
THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[2], 0, &target_start));
508+
THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[3], 0, &source_start));
509+
THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[4], ts_obj_length,
510+
&source_end));
504511

505512
// Copy 0 bytes; we're done
506513
if (target_start >= target_length || source_start >= source_end)
@@ -631,13 +638,13 @@ void StringWrite(const FunctionCallbackInfo<Value>& args) {
631638
size_t offset;
632639
size_t max_length;
633640

634-
THROW_AND_RETURN_IF_OOB(ParseArrayIndex(args[1], 0, &offset));
641+
THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[1], 0, &offset));
635642
if (offset > ts_obj_length) {
636643
return node::THROW_ERR_BUFFER_OUT_OF_BOUNDS(
637644
env, "\"offset\" is outside of buffer bounds");
638645
}
639646

640-
THROW_AND_RETURN_IF_OOB(ParseArrayIndex(args[2], ts_obj_length - offset,
647+
THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[2], ts_obj_length - offset,
641648
&max_length));
642649

643650
max_length = MIN(ts_obj_length - offset, max_length);
@@ -692,10 +699,12 @@ void CompareOffset(const FunctionCallbackInfo<Value> &args) {
692699
size_t source_end;
693700
size_t target_end;
694701

695-
THROW_AND_RETURN_IF_OOB(ParseArrayIndex(args[2], 0, &target_start));
696-
THROW_AND_RETURN_IF_OOB(ParseArrayIndex(args[3], 0, &source_start));
697-
THROW_AND_RETURN_IF_OOB(ParseArrayIndex(args[4], target_length, &target_end));
698-
THROW_AND_RETURN_IF_OOB(ParseArrayIndex(args[5], ts_obj_length, &source_end));
702+
THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[2], 0, &target_start));
703+
THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[3], 0, &source_start));
704+
THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[4], target_length,
705+
&target_end));
706+
THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[5], ts_obj_length,
707+
&source_end));
699708

700709
if (source_start > ts_obj_length)
701710
return THROW_ERR_OUT_OF_RANGE(
Collapse file

‎test/parallel/test-buffer-copy.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-buffer-copy.js
+26Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,17 @@ let cntr = 0;
2525
}
2626
}
2727

28+
{
29+
// Current behavior is to coerce values to integers.
30+
b.fill(++cntr);
31+
c.fill(++cntr);
32+
const copied = b.copy(c, '0', '0', '512');
33+
assert.strictEqual(copied, 512);
34+
for (let i = 0; i < c.length; i++) {
35+
assert.strictEqual(c[i], b[i]);
36+
}
37+
}
38+
2839
{
2940
// copy c into b, without specifying sourceEnd
3041
b.fill(++cntr);
@@ -152,3 +163,18 @@ assert.strictEqual(b.copy(c, 512, 0, 10), 0);
152163
assert.strictEqual(c[i], e[i]);
153164
}
154165
}
166+
167+
// https://github.com/nodejs/node/issues/23668: Do not crash for invalid input.
168+
c.fill('c');
169+
b.copy(c, 'not a valid offset');
170+
// Make sure this acted like a regular copy with `0` offset.
171+
assert.deepStrictEqual(c, b.slice(0, c.length));
172+
173+
{
174+
c.fill('C');
175+
assert.throws(() => {
176+
b.copy(c, { [Symbol.toPrimitive]() { throw new Error('foo'); } });
177+
}, /foo/);
178+
// No copying took place:
179+
assert.deepStrictEqual(c.toString(), 'C'.repeat(c.length));
180+
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.