Skip to content

Commit

Permalink
buffer: move c++ float functions to js
Browse files Browse the repository at this point in the history 
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: nodejs#12179
Fixes: nodejs#8724
  • Loading branch information
@BridgeAR
BridgeAR committed Mar 2, 2018
1 parent a93830a commit 48b4f00
Show file tree
Hide file tree
Showing 3 changed files with 76 additions and 136 deletions.
108 changes: 74 additions & 34 deletions lib/buffer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,6 @@ const {
swap16: _swap16,
swap32: _swap32,
swap64: _swap64,
writeDoubleBE: _writeDoubleBE,
writeDoubleLE: _writeDoubleLE,
writeFloatBE: _writeFloatBE,
writeFloatLE: _writeFloatLE,
kMaxLength,
kStringMaxLength
} = process.binding('buffer');
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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);
}


Expand Down Expand Up @@ -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];
Expand Down
97 changes: 2 additions & 95 deletions src/node_buffer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,8 +163,7 @@ 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 needed = 0) {
size_t* ret) {
if (arg->IsUndefined()) {
*ret = def;
return true;
Expand All @@ -178,7 +177,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 - needed)
if (static_cast<uint64_t>(tmp_i) > kSizeMax)
return false;

*ret = static_cast<size_t>(tmp_i);
Expand Down Expand Up @@ -686,93 +685,6 @@ void StringWrite(const FunctionCallbackInfo<Value>& 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 <typename T, enum Endianness endianness>
void WriteFloatGeneric(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

bool should_assert = args.Length() < 4;

if (should_assert) {
THROW_AND_RETURN_UNLESS_BUFFER(env, args[0]);
}

Local<ArrayBufferView> ts_obj = args[0].As<ArrayBufferView>();
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<char*>(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<char*>(ts_obj_data) + offset;
if (endianness != GetEndianness())
Swizzle(na.bytes, sizeof(na.bytes));
memcpy(ptr, na.bytes, memcpy_num);
}


void WriteFloatLE(const FunctionCallbackInfo<Value>& args) {
WriteFloatGeneric<float, kLittleEndian>(args);
}


void WriteFloatBE(const FunctionCallbackInfo<Value>& args) {
WriteFloatGeneric<float, kBigEndian>(args);
}


void WriteDoubleLE(const FunctionCallbackInfo<Value>& args) {
WriteFloatGeneric<double, kLittleEndian>(args);
}


void WriteDoubleBE(const FunctionCallbackInfo<Value>& args) {
WriteFloatGeneric<double, kBigEndian>(args);
}


void ByteLengthUtf8(const FunctionCallbackInfo<Value> &args) {
CHECK(args[0]->IsString());

Expand Down Expand Up @@ -1211,11 +1123,6 @@ void Initialize(Local<Object> 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);
Expand Down
7 changes: 0 additions & 7 deletions test/parallel/test-buffer-write-noassert.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit 48b4f00

Please sign in to comment.