Skip to content

Commit

Permalink
src, buffer: do not segfault on out-of-range index
Browse files Browse the repository at this point in the history 
Also add test cases for partial writes and invalid indices.

PR-URL: #11927
Fixes: #8724
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
  • Loading branch information
@TimothyGu @MylesBorins
TimothyGu authored and MylesBorins committed Apr 19, 2017
1 parent 3801790 commit c33933e
Show file tree
Hide file tree
Showing 2 changed files with 75 additions and 16 deletions.
28 changes: 20 additions & 8 deletions src/node_buffer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,8 @@ 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) {
size_t* ret,
size_t needed = 0) {
if (arg->IsUndefined()) {
*ret = def;
return true;
Expand All @@ -173,7 +174,7 @@ inline MUST_USE_RESULT bool ParseArrayIndex(Local<Value> arg,
// 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)
if (static_cast<uint64_t>(tmp_i) > kSizeMax - needed)
return false;

*ret = static_cast<size_t>(tmp_i);
Expand Down Expand Up @@ -805,17 +806,28 @@ void WriteFloatGeneric(const FunctionCallbackInfo<Value>& args) {
CHECK_NE(ts_obj_data, nullptr);

T val = args[1]->NumberValue(env->context()).FromMaybe(0);
size_t offset = args[2]->IntegerValue(env->context()).FromMaybe(0);

size_t memcpy_num = sizeof(T);
size_t offset;

if (should_assert) {
THROW_AND_RETURN_IF_OOB(offset + memcpy_num >= memcpy_num);
THROW_AND_RETURN_IF_OOB(offset + memcpy_num <= ts_obj_length);
// If the offset is negative or larger than the size of the ArrayBuffer,
// throw an error (if needed) and return directly.
if (!ParseArrayIndex(args[2], 0, &offset, memcpy_num) ||
offset >= ts_obj_length) {
if (should_assert)
THROW_AND_RETURN_IF_OOB(false);
return;
}

if (offset + memcpy_num > ts_obj_length)
memcpy_num = ts_obj_length - offset;
// If the offset is too large for the entire value, but small enough to fit
// part of the value, throw an error and return only if should_assert is
// true. Otherwise, write the part of the value that fits.
if (offset + memcpy_num > ts_obj_length) {
if (should_assert)
THROW_AND_RETURN_IF_OOB(false);
else
memcpy_num = ts_obj_length - offset;
}

union NoAlias {
T val;
Expand Down
63 changes: 55 additions & 8 deletions test/parallel/test-buffer-write-noassert.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,28 +4,33 @@ const assert = require('assert');

// testing buffer write functions

const outOfRange = /^RangeError: (?:Index )?out of range(?: index)?$/;

function write(funx, args, result, res) {
{
const buf = Buffer.alloc(9);
assert.strictEqual(buf[funx](...args), result);
assert.deepStrictEqual(buf, res);
}

{
const invalidArgs = Array.from(args);
invalidArgs[1] = -1;
assert.throws(
() => Buffer.alloc(9)[funx](...invalidArgs),
/^RangeError: (?:Index )?out of range(?: index)?$/
);
}
writeInvalidOffset(-1);
writeInvalidOffset(9);

{
const buf2 = Buffer.alloc(9);
assert.strictEqual(buf2[funx](...args, true), result);
assert.deepStrictEqual(buf2, res);
}

function writeInvalidOffset(offset) {
const newArgs = Array.from(args);
newArgs[1] = offset;
assert.throws(() => Buffer.alloc(9)[funx](...newArgs), outOfRange);

const buf = Buffer.alloc(9);
buf[funx](...newArgs, true);
assert.deepStrictEqual(buf, Buffer.alloc(9));
}
}

write('writeInt8', [1, 0], 1, Buffer.from([1, 0, 0, 0, 0, 0, 0, 0, 0]));
Expand All @@ -46,3 +51,45 @@ write('writeDoubleBE', [1, 1], 9, Buffer.from([0, 63, 240, 0, 0, 0, 0, 0, 0]));
write('writeDoubleLE', [1, 1], 9, Buffer.from([0, 0, 0, 0, 0, 0, 0, 240, 63]));
write('writeFloatBE', [1, 1], 5, Buffer.from([0, 63, 128, 0, 0, 0, 0, 0, 0]));
write('writeFloatLE', [1, 1], 5, Buffer.from([0, 0, 0, 128, 63, 0, 0, 0, 0]));

function writePartial(funx, args, result, res) {
assert.throws(() => Buffer.alloc(9)[funx](...args), outOfRange);
const buf = Buffer.alloc(9);
assert.strictEqual(buf[funx](...args, true), result);
assert.deepStrictEqual(buf, res);
}

// Test partial writes (cases where the buffer isn't large enough to hold the
// entire value, but is large enough to hold parts of it).
writePartial('writeIntBE', [0x0eadbeef, 6, 4], 10,
Buffer.from([0, 0, 0, 0, 0, 0, 0x0e, 0xad, 0xbe]));
writePartial('writeIntLE', [0x0eadbeef, 6, 4], 10,
Buffer.from([0, 0, 0, 0, 0, 0, 0xef, 0xbe, 0xad]));
writePartial('writeInt16BE', [0x1234, 8], 10,
Buffer.from([0, 0, 0, 0, 0, 0, 0, 0, 0x12]));
writePartial('writeInt16LE', [0x1234, 8], 10,
Buffer.from([0, 0, 0, 0, 0, 0, 0, 0, 0x34]));
writePartial('writeInt32BE', [0x0eadbeef, 6], 10,
Buffer.from([0, 0, 0, 0, 0, 0, 0x0e, 0xad, 0xbe]));
writePartial('writeInt32LE', [0x0eadbeef, 6], 10,
Buffer.from([0, 0, 0, 0, 0, 0, 0xef, 0xbe, 0xad]));
writePartial('writeUIntBE', [0xdeadbeef, 6, 4], 10,
Buffer.from([0, 0, 0, 0, 0, 0, 0xde, 0xad, 0xbe]));
writePartial('writeUIntLE', [0xdeadbeef, 6, 4], 10,
Buffer.from([0, 0, 0, 0, 0, 0, 0xef, 0xbe, 0xad]));
writePartial('writeUInt16BE', [0x1234, 8], 10,
Buffer.from([0, 0, 0, 0, 0, 0, 0, 0, 0x12]));
writePartial('writeUInt16LE', [0x1234, 8], 10,
Buffer.from([0, 0, 0, 0, 0, 0, 0, 0, 0x34]));
writePartial('writeUInt32BE', [0xdeadbeef, 6], 10,
Buffer.from([0, 0, 0, 0, 0, 0, 0xde, 0xad, 0xbe]));
writePartial('writeUInt32LE', [0xdeadbeef, 6], 10,
Buffer.from([0, 0, 0, 0, 0, 0, 0xef, 0xbe, 0xad]));
writePartial('writeDoubleBE', [1, 2], 10,
Buffer.from([0, 0, 63, 240, 0, 0, 0, 0, 0]));
writePartial('writeDoubleLE', [1, 2], 10,
Buffer.from([0, 0, 0, 0, 0, 0, 0, 0, 240]));
writePartial('writeFloatBE', [1, 6], 10,
Buffer.from([0, 0, 0, 0, 0, 0, 63, 128, 0]));
writePartial('writeFloatLE', [1, 6], 10,
Buffer.from([0, 0, 0, 0, 0, 0, 0, 0, 128]));

0 comments on commit c33933e

Please sign in to comment.