From c1bf5553f9cb1cefe57ee221706d7747a612e9f4 Mon Sep 17 00:00:00 2001 From: Timothy Gu Date: Sun, 19 Mar 2017 13:35:47 -0700 Subject: [PATCH 1/2] src, buffer: do not segfault on out-of-range index Also add test cases for partial writes and invalid indices. Fixes: https://github.com/nodejs/node/issues/8724 --- src/node_buffer.cc | 26 ++++++--- test/parallel/test-buffer-write-noassert.js | 63 ++++++++++++++++++--- 2 files changed, 74 insertions(+), 15 deletions(-) diff --git a/src/node_buffer.cc b/src/node_buffer.cc index dc8afd578373db..f486645ecfdcb8 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -169,7 +169,8 @@ void CallbackInfo::WeakCallback(Isolate* isolate) { // Parse index for external array data. inline MUST_USE_RESULT bool ParseArrayIndex(Local arg, size_t def, - size_t* ret) { + size_t* ret, + size_t needed = 0) { if (arg->IsUndefined()) { *ret = def; return true; @@ -815,17 +816,28 @@ void WriteFloatGeneric(const FunctionCallbackInfo& 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; diff --git a/test/parallel/test-buffer-write-noassert.js b/test/parallel/test-buffer-write-noassert.js index 7423e462ca1af2..10e9fd8b766837 100644 --- a/test/parallel/test-buffer-write-noassert.js +++ b/test/parallel/test-buffer-write-noassert.js @@ -4,6 +4,8 @@ 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); @@ -11,14 +13,8 @@ function write(funx, args, result, res) { 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( @@ -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])); @@ -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])); From a2c787605f9a7d9c8067d3ae0451a6a1a952ab19 Mon Sep 17 00:00:00 2001 From: Timothy Gu Date: Sun, 19 Mar 2017 13:59:45 -0700 Subject: [PATCH 2/2] Fix up needed variable usage --- src/node_buffer.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_buffer.cc b/src/node_buffer.cc index f486645ecfdcb8..04c0161eddd2ae 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -184,7 +184,7 @@ inline MUST_USE_RESULT bool ParseArrayIndex(Local arg, // Check that the result fits in a size_t. const uint64_t kSizeMax = static_cast(static_cast(-1)); // coverity[pointless_expression] - if (static_cast(tmp_i) > kSizeMax) + if (static_cast(tmp_i) > kSizeMax - needed) return false; *ret = static_cast(tmp_i);