From 1411b30f4646878fc3ff452c3d97a240c0d8b807 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 30 Jan 2018 17:21:28 +0100 Subject: [PATCH] buffer: move c++ float functions to js This ports the Buffer#write(Double|Float)(B|L)E functions to JS. This fixes a security issue concerning type confusion and fixes another possible crash in combination with `noAssert`. In addition to that it will also significantly improve the write performance. Fixes: https://github.com/nodejs/node/issues/12179 Fixes: https://github.com/nodejs/node/issues/8724 PR-URL: https://github.com/nodejs/node/pull/18395 Reviewed-By: James M Snell Reviewed-By: Matteo Collina Reviewed-By: Anatoli Papirovski Reviewed-By: Ben Noordhuis --- lib/buffer.js | 108 ++++++++++++++------ src/node_buffer.cc | 97 +----------------- test/parallel/test-buffer-write-noassert.js | 7 -- 3 files changed, 76 insertions(+), 136 deletions(-) diff --git a/lib/buffer.js b/lib/buffer.js index 0aa9e2001bd9d7..f36bd76acdcc2e 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -34,10 +34,6 @@ const { swap16: _swap16, swap32: _swap32, swap64: _swap64, - writeDoubleBE: _writeDoubleBE, - writeDoubleLE: _writeDoubleLE, - writeFloatBE: _writeFloatBE, - writeFloatLE: _writeFloatLE, kMaxLength, kStringMaxLength } = process.binding('buffer'); @@ -85,6 +81,12 @@ const constants = Object.defineProperties({}, { } }); +// Temporary buffers to convert numbers. +const float32Array = new Float32Array(1); +const uInt8Float32Array = new Uint8Array(float32Array.buffer); +const float64Array = new Float64Array(1); +const uInt8Float64Array = new Uint8Array(float64Array.buffer); + Buffer.poolSize = 8 * 1024; var poolSize, poolOffset, allocPool; @@ -1297,12 +1299,16 @@ Buffer.prototype.readFloatLE = function(offset, noAssert) { return toFloat(this.readUInt32LE(offset, noAssert)); }; +function checkOOB(buffer, offset, ext) { + if (offset + ext > buffer.length) + throw new errors.RangeError('ERR_INDEX_OUT_OF_RANGE'); +} + function checkInt(buffer, value, offset, ext, max, min) { if (value > max || value < min) throw new errors.RangeError('ERR_INVALID_OPT_VALUE', 'value', value); - if (offset + ext > buffer.length) - throw new errors.RangeError('ERR_INDEX_OUT_OF_RANGE'); + checkOOB(buffer, offset, ext); } @@ -1520,49 +1526,83 @@ Buffer.prototype.writeInt32BE = function writeInt32BE(value, offset, noAssert) { return offset + 4; }; - -Buffer.prototype.writeFloatLE = function writeFloatLE(val, offset, noAssert) { +function writeDoubleForwards(val, offset, noAssert) { val = +val; offset = offset >>> 0; if (!noAssert) - _writeFloatLE(this, val, offset); - else - _writeFloatLE(this, val, offset, true); - return offset + 4; -}; - + checkOOB(this, offset, 8); + + float64Array[0] = val; + this[offset++] = uInt8Float64Array[0]; + this[offset++] = uInt8Float64Array[1]; + this[offset++] = uInt8Float64Array[2]; + this[offset++] = uInt8Float64Array[3]; + this[offset++] = uInt8Float64Array[4]; + this[offset++] = uInt8Float64Array[5]; + this[offset++] = uInt8Float64Array[6]; + this[offset++] = uInt8Float64Array[7]; + return offset; +} -Buffer.prototype.writeFloatBE = function writeFloatBE(val, offset, noAssert) { +function writeDoubleBackwards(val, offset, noAssert) { val = +val; offset = offset >>> 0; if (!noAssert) - _writeFloatBE(this, val, offset); - else - _writeFloatBE(this, val, offset, true); - return offset + 4; -}; - + checkOOB(this, offset, 8); + + float64Array[0] = val; + this[offset++] = uInt8Float64Array[7]; + this[offset++] = uInt8Float64Array[6]; + this[offset++] = uInt8Float64Array[5]; + this[offset++] = uInt8Float64Array[4]; + this[offset++] = uInt8Float64Array[3]; + this[offset++] = uInt8Float64Array[2]; + this[offset++] = uInt8Float64Array[1]; + this[offset++] = uInt8Float64Array[0]; + return offset; +} -Buffer.prototype.writeDoubleLE = function writeDoubleLE(val, offset, noAssert) { +function writeFloatForwards(val, offset, noAssert) { val = +val; offset = offset >>> 0; if (!noAssert) - _writeDoubleLE(this, val, offset); - else - _writeDoubleLE(this, val, offset, true); - return offset + 8; -}; - + checkOOB(this, offset, 4); + + float32Array[0] = val; + this[offset++] = uInt8Float32Array[0]; + this[offset++] = uInt8Float32Array[1]; + this[offset++] = uInt8Float32Array[2]; + this[offset++] = uInt8Float32Array[3]; + return offset; +} -Buffer.prototype.writeDoubleBE = function writeDoubleBE(val, offset, noAssert) { +function writeFloatBackwards(val, offset, noAssert) { val = +val; offset = offset >>> 0; if (!noAssert) - _writeDoubleBE(this, val, offset); - else - _writeDoubleBE(this, val, offset, true); - return offset + 8; -}; + checkOOB(this, offset, 4); + + float32Array[0] = val; + this[offset++] = uInt8Float32Array[3]; + this[offset++] = uInt8Float32Array[2]; + this[offset++] = uInt8Float32Array[1]; + this[offset++] = uInt8Float32Array[0]; + return offset; +} + +// Check endianness. +float32Array[0] = -1; +if (uInt8Float32Array[3] === 0) { // Big endian. + Buffer.prototype.writeFloatLE = writeFloatBackwards; + Buffer.prototype.writeFloatBE = writeFloatForwards; + Buffer.prototype.writeDoubleLE = writeDoubleBackwards; + Buffer.prototype.writeDoubleBE = writeDoubleForwards; +} else { // Small endian. + Buffer.prototype.writeFloatLE = writeFloatForwards; + Buffer.prototype.writeFloatBE = writeFloatBackwards; + Buffer.prototype.writeDoubleLE = writeDoubleForwards; + Buffer.prototype.writeDoubleBE = writeDoubleBackwards; +} function swap(b, n, m) { const i = b[n]; diff --git a/src/node_buffer.cc b/src/node_buffer.cc index c44aa692cdacfe..c25fe11b5a4c6a 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -163,8 +163,7 @@ 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 needed = 0) { + size_t* ret) { if (arg->IsUndefined()) { *ret = def; return true; @@ -178,7 +177,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 - needed) + if (static_cast(tmp_i) > kSizeMax) return false; *ret = static_cast(tmp_i); @@ -686,93 +685,6 @@ void StringWrite(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(written); } - -static inline void Swizzle(char* start, unsigned int len) { - char* end = start + len - 1; - while (start < end) { - char tmp = *start; - *start++ = *end; - *end-- = tmp; - } -} - - -template -void WriteFloatGeneric(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - - bool should_assert = args.Length() < 4; - - if (should_assert) { - THROW_AND_RETURN_UNLESS_BUFFER(env, args[0]); - } - - Local ts_obj = args[0].As(); - ArrayBuffer::Contents ts_obj_c = ts_obj->Buffer()->GetContents(); - const size_t ts_obj_offset = ts_obj->ByteOffset(); - const size_t ts_obj_length = ts_obj->ByteLength(); - char* const ts_obj_data = - static_cast(ts_obj_c.Data()) + ts_obj_offset; - if (ts_obj_length > 0) - CHECK_NE(ts_obj_data, nullptr); - - T val = args[1]->NumberValue(env->context()).FromMaybe(0); - - size_t memcpy_num = sizeof(T); - size_t offset; - - // 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 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; - char bytes[sizeof(T)]; - }; - - union NoAlias na = { val }; - char* ptr = static_cast(ts_obj_data) + offset; - if (endianness != GetEndianness()) - Swizzle(na.bytes, sizeof(na.bytes)); - memcpy(ptr, na.bytes, memcpy_num); -} - - -void WriteFloatLE(const FunctionCallbackInfo& args) { - WriteFloatGeneric(args); -} - - -void WriteFloatBE(const FunctionCallbackInfo& args) { - WriteFloatGeneric(args); -} - - -void WriteDoubleLE(const FunctionCallbackInfo& args) { - WriteFloatGeneric(args); -} - - -void WriteDoubleBE(const FunctionCallbackInfo& args) { - WriteFloatGeneric(args); -} - - void ByteLengthUtf8(const FunctionCallbackInfo &args) { CHECK(args[0]->IsString()); @@ -1211,11 +1123,6 @@ void Initialize(Local target, env->SetMethod(target, "indexOfNumber", IndexOfNumber); env->SetMethod(target, "indexOfString", IndexOfString); - env->SetMethod(target, "writeDoubleBE", WriteDoubleBE); - env->SetMethod(target, "writeDoubleLE", WriteDoubleLE); - env->SetMethod(target, "writeFloatBE", WriteFloatBE); - env->SetMethod(target, "writeFloatLE", WriteFloatLE); - env->SetMethod(target, "swap16", Swap16); env->SetMethod(target, "swap32", Swap32); env->SetMethod(target, "swap64", Swap64); diff --git a/test/parallel/test-buffer-write-noassert.js b/test/parallel/test-buffer-write-noassert.js index 0730aaa3544983..a521e01129a094 100644 --- a/test/parallel/test-buffer-write-noassert.js +++ b/test/parallel/test-buffer-write-noassert.js @@ -16,13 +16,6 @@ function write(funx, args, result, res) { writeInvalidOffset(-1); writeInvalidOffset(9); - if (!/Int/.test(funx)) { - assert.throws( - () => Buffer.alloc(9)[funx].apply(new Map(), args), - /^TypeError: argument should be a Buffer$/ - ); - } - { const buf2 = Buffer.alloc(9); assert.strictEqual(buf2[funx](...args, true), result);