FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

buffer: fix crash for invalid index types · nodejs/node@7dc4c3b · GitHub

/ node Public

Commit 7dc4c3b

Browse files
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

2 files changed

Lines changed: 60 additions & 25 deletions

File tree

‎src/node_buffer.cc‎

Lines 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(

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

Lines 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)
Back | FazBrowse Home | New Git URL