Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src, buffer: do not segfault on out-of-range index #11927

Closed
wants to merge 2 commits into from
Closed

src, buffer: do not segfault on out-of-range index #11927

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c1bf555
src, buffer: do not segfault on out-of-range index
TimothyGu Mar 19, 2017
a2c7876
Fix up needed variable usage
TimothyGu Mar 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 20 additions & 8 deletions src/node_buffer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this used anywhere?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, below.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I mean, inside of ParseArrayIndex? I feel like I am missing the point of passing this argument…

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait... oops, git commit -p fail. Updated patch incoming.

if (arg->IsUndefined()) {
*ret = def;
return true;
Expand All @@ -183,7 +184,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 @@ -815,17 +816,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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,17 @@ 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);

if (!/Int/.test(funx)) {
assert.throws(
Expand All @@ -33,6 +29,15 @@ function write(funx, args, result, res) {
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 @@ -53,3 +58,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]));