Skip to content

Commit 1411b30

Browse files
BridgeARBridgeAR
committed
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: #12179 Fixes: #8724 PR-URL: #18395 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
1 parent 069dd10 commit 1411b30

File tree

3 files changed

+76
-136
lines changed

3 files changed

+76
-136
lines changed

‎lib/buffer.js‎

Lines changed: 74 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,6 @@ const {
3434
swap16: _swap16,
3535
swap32: _swap32,
3636
swap64: _swap64,
37-
writeDoubleBE: _writeDoubleBE,
38-
writeDoubleLE: _writeDoubleLE,
39-
writeFloatBE: _writeFloatBE,
40-
writeFloatLE: _writeFloatLE,
4137
kMaxLength,
4238
kStringMaxLength
4339
} = process.binding('buffer');
@@ -85,6 +81,12 @@ const constants = Object.defineProperties({}, {
8581
}
8682
});
8783

84+
// Temporary buffers to convert numbers.
85+
const float32Array = new Float32Array(1);
86+
const uInt8Float32Array = new Uint8Array(float32Array.buffer);
87+
const float64Array = new Float64Array(1);
88+
const uInt8Float64Array = new Uint8Array(float64Array.buffer);
89+
8890
Buffer.poolSize = 8 * 1024;
8991
var poolSize, poolOffset, allocPool;
9092

@@ -1297,12 +1299,16 @@ Buffer.prototype.readFloatLE = function(offset, noAssert) {
12971299
return toFloat(this.readUInt32LE(offset, noAssert));
12981300
};
12991301

1302+
function checkOOB(buffer, offset, ext) {
1303+
if (offset + ext > buffer.length)
1304+
throw new errors.RangeError('ERR_INDEX_OUT_OF_RANGE');
1305+
}
1306+
13001307

13011308
function checkInt(buffer, value, offset, ext, max, min) {
13021309
if (value > max || value < min)
13031310
throw new errors.RangeError('ERR_INVALID_OPT_VALUE', 'value', value);
1304-
if (offset + ext > buffer.length)
1305-
throw new errors.RangeError('ERR_INDEX_OUT_OF_RANGE');
1311+
checkOOB(buffer, offset, ext);
13061312
}
13071313

13081314

@@ -1520,49 +1526,83 @@ Buffer.prototype.writeInt32BE = function writeInt32BE(value, offset, noAssert) {
15201526
return offset + 4;
15211527
};
15221528

1523-
1524-
Buffer.prototype.writeFloatLE = function writeFloatLE(val, offset, noAssert) {
1529+
function writeDoubleForwards(val, offset, noAssert) {
15251530
val = +val;
15261531
offset = offset >>> 0;
15271532
if (!noAssert)
1528-
_writeFloatLE(this, val, offset);
1529-
else
1530-
_writeFloatLE(this, val, offset, true);
1531-
return offset + 4;
1532-
};
1533-
1533+
checkOOB(this, offset, 8);
1534+
1535+
float64Array[0] = val;
1536+
this[offset++] = uInt8Float64Array[0];
1537+
this[offset++] = uInt8Float64Array[1];
1538+
this[offset++] = uInt8Float64Array[2];
1539+
this[offset++] = uInt8Float64Array[3];
1540+
this[offset++] = uInt8Float64Array[4];
1541+
this[offset++] = uInt8Float64Array[5];
1542+
this[offset++] = uInt8Float64Array[6];
1543+
this[offset++] = uInt8Float64Array[7];
1544+
return offset;
1545+
}
15341546

1535-
Buffer.prototype.writeFloatBE = function writeFloatBE(val, offset, noAssert) {
1547+
function writeDoubleBackwards(val, offset, noAssert) {
15361548
val = +val;
15371549
offset = offset >>> 0;
15381550
if (!noAssert)
1539-
_writeFloatBE(this, val, offset);
1540-
else
1541-
_writeFloatBE(this, val, offset, true);
1542-
return offset + 4;
1543-
};
1544-
1551+
checkOOB(this, offset, 8);
1552+
1553+
float64Array[0] = val;
1554+
this[offset++] = uInt8Float64Array[7];
1555+
this[offset++] = uInt8Float64Array[6];
1556+
this[offset++] = uInt8Float64Array[5];
1557+
this[offset++] = uInt8Float64Array[4];
1558+
this[offset++] = uInt8Float64Array[3];
1559+
this[offset++] = uInt8Float64Array[2];
1560+
this[offset++] = uInt8Float64Array[1];
1561+
this[offset++] = uInt8Float64Array[0];
1562+
return offset;
1563+
}
15451564

1546-
Buffer.prototype.writeDoubleLE = function writeDoubleLE(val, offset, noAssert) {
1565+
function writeFloatForwards(val, offset, noAssert) {
15471566
val = +val;
15481567
offset = offset >>> 0;
15491568
if (!noAssert)
1550-
_writeDoubleLE(this, val, offset);
1551-
else
1552-
_writeDoubleLE(this, val, offset, true);
1553-
return offset + 8;
1554-
};
1555-
1569+
checkOOB(this, offset, 4);
1570+
1571+
float32Array[0] = val;
1572+
this[offset++] = uInt8Float32Array[0];
1573+
this[offset++] = uInt8Float32Array[1];
1574+
this[offset++] = uInt8Float32Array[2];
1575+
this[offset++] = uInt8Float32Array[3];
1576+
return offset;
1577+
}
15561578

1557-
Buffer.prototype.writeDoubleBE = function writeDoubleBE(val, offset, noAssert) {
1579+
function writeFloatBackwards(val, offset, noAssert) {
15581580
val = +val;
15591581
offset = offset >>> 0;
15601582
if (!noAssert)
1561-
_writeDoubleBE(this, val, offset);
1562-
else
1563-
_writeDoubleBE(this, val, offset, true);
1564-
return offset + 8;
1565-
};
1583+
checkOOB(this, offset, 4);
1584+
1585+
float32Array[0] = val;
1586+
this[offset++] = uInt8Float32Array[3];
1587+
this[offset++] = uInt8Float32Array[2];
1588+
this[offset++] = uInt8Float32Array[1];
1589+
this[offset++] = uInt8Float32Array[0];
1590+
return offset;
1591+
}
1592+
1593+
// Check endianness.
1594+
float32Array[0] = -1;
1595+
if (uInt8Float32Array[3] === 0) { // Big endian.
1596+
Buffer.prototype.writeFloatLE = writeFloatBackwards;
1597+
Buffer.prototype.writeFloatBE = writeFloatForwards;
1598+
Buffer.prototype.writeDoubleLE = writeDoubleBackwards;
1599+
Buffer.prototype.writeDoubleBE = writeDoubleForwards;
1600+
} else { // Small endian.
1601+
Buffer.prototype.writeFloatLE = writeFloatForwards;
1602+
Buffer.prototype.writeFloatBE = writeFloatBackwards;
1603+
Buffer.prototype.writeDoubleLE = writeDoubleForwards;
1604+
Buffer.prototype.writeDoubleBE = writeDoubleBackwards;
1605+
}
15661606

15671607
function swap(b, n, m) {
15681608
const i = b[n];

‎src/node_buffer.cc‎

Lines changed: 2 additions & 95 deletions
Original file line numberDiff line numberDiff line change
@@ -163,8 +163,7 @@ void CallbackInfo::WeakCallback(Isolate* isolate) {
163163
// Parse index for external array data.
164164
inline MUST_USE_RESULT bool ParseArrayIndex(Local<Value> arg,
165165
size_t def,
166-
size_t* ret,
167-
size_t needed = 0) {
166+
size_t* ret) {
168167
if (arg->IsUndefined()) {
169168
*ret = def;
170169
return true;
@@ -178,7 +177,7 @@ inline MUST_USE_RESULT bool ParseArrayIndex(Local<Value> arg,
178177
// Check that the result fits in a size_t.
179178
const uint64_t kSizeMax = static_cast<uint64_t>(static_cast<size_t>(-1));
180179
// coverity[pointless_expression]
181-
if (static_cast<uint64_t>(tmp_i) > kSizeMax - needed)
180+
if (static_cast<uint64_t>(tmp_i) > kSizeMax)
182181
return false;
183182

184183
*ret = static_cast<size_t>(tmp_i);
@@ -686,93 +685,6 @@ void StringWrite(const FunctionCallbackInfo<Value>& args) {
686685
args.GetReturnValue().Set(written);
687686
}
688687

689-
690-
static inline void Swizzle(char* start, unsigned int len) {
691-
char* end = start + len - 1;
692-
while (start < end) {
693-
char tmp = *start;
694-
*start++ = *end;
695-
*end-- = tmp;
696-
}
697-
}
698-
699-
700-
template <typename T, enum Endianness endianness>
701-
void WriteFloatGeneric(const FunctionCallbackInfo<Value>& args) {
702-
Environment* env = Environment::GetCurrent(args);
703-
704-
bool should_assert = args.Length() < 4;
705-
706-
if (should_assert) {
707-
THROW_AND_RETURN_UNLESS_BUFFER(env, args[0]);
708-
}
709-
710-
Local<ArrayBufferView> ts_obj = args[0].As<ArrayBufferView>();
711-
ArrayBuffer::Contents ts_obj_c = ts_obj->Buffer()->GetContents();
712-
const size_t ts_obj_offset = ts_obj->ByteOffset();
713-
const size_t ts_obj_length = ts_obj->ByteLength();
714-
char* const ts_obj_data =
715-
static_cast<char*>(ts_obj_c.Data()) + ts_obj_offset;
716-
if (ts_obj_length > 0)
717-
CHECK_NE(ts_obj_data, nullptr);
718-
719-
T val = args[1]->NumberValue(env->context()).FromMaybe(0);
720-
721-
size_t memcpy_num = sizeof(T);
722-
size_t offset;
723-
724-
// If the offset is negative or larger than the size of the ArrayBuffer,
725-
// throw an error (if needed) and return directly.
726-
if (!ParseArrayIndex(args[2], 0, &offset, memcpy_num) ||
727-
offset >= ts_obj_length) {
728-
if (should_assert)
729-
THROW_AND_RETURN_IF_OOB(false);
730-
return;
731-
}
732-
733-
// If the offset is too large for the entire value, but small enough to fit
734-
// part of the value, throw an error and return only if should_assert is
735-
// true. Otherwise, write the part of the value that fits.
736-
if (offset + memcpy_num > ts_obj_length) {
737-
if (should_assert)
738-
THROW_AND_RETURN_IF_OOB(false);
739-
else
740-
memcpy_num = ts_obj_length - offset;
741-
}
742-
743-
union NoAlias {
744-
T val;
745-
char bytes[sizeof(T)];
746-
};
747-
748-
union NoAlias na = { val };
749-
char* ptr = static_cast<char*>(ts_obj_data) + offset;
750-
if (endianness != GetEndianness())
751-
Swizzle(na.bytes, sizeof(na.bytes));
752-
memcpy(ptr, na.bytes, memcpy_num);
753-
}
754-
755-
756-
void WriteFloatLE(const FunctionCallbackInfo<Value>& args) {
757-
WriteFloatGeneric<float, kLittleEndian>(args);
758-
}
759-
760-
761-
void WriteFloatBE(const FunctionCallbackInfo<Value>& args) {
762-
WriteFloatGeneric<float, kBigEndian>(args);
763-
}
764-
765-
766-
void WriteDoubleLE(const FunctionCallbackInfo<Value>& args) {
767-
WriteFloatGeneric<double, kLittleEndian>(args);
768-
}
769-
770-
771-
void WriteDoubleBE(const FunctionCallbackInfo<Value>& args) {
772-
WriteFloatGeneric<double, kBigEndian>(args);
773-
}
774-
775-
776688
void ByteLengthUtf8(const FunctionCallbackInfo<Value> &args) {
777689
CHECK(args[0]->IsString());
778690

@@ -1211,11 +1123,6 @@ void Initialize(Local<Object> target,
12111123
env->SetMethod(target, "indexOfNumber", IndexOfNumber);
12121124
env->SetMethod(target, "indexOfString", IndexOfString);
12131125

1214-
env->SetMethod(target, "writeDoubleBE", WriteDoubleBE);
1215-
env->SetMethod(target, "writeDoubleLE", WriteDoubleLE);
1216-
env->SetMethod(target, "writeFloatBE", WriteFloatBE);
1217-
env->SetMethod(target, "writeFloatLE", WriteFloatLE);
1218-
12191126
env->SetMethod(target, "swap16", Swap16);
12201127
env->SetMethod(target, "swap32", Swap32);
12211128
env->SetMethod(target, "swap64", Swap64);

‎test/parallel/test-buffer-write-noassert.js‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,6 @@ function write(funx, args, result, res) {
1616
writeInvalidOffset(-1);
1717
writeInvalidOffset(9);
1818

19-
if (!/Int/.test(funx)) {
20-
assert.throws(
21-
() => Buffer.alloc(9)[funx].apply(new Map(), args),
22-
/^TypeError: argument should be a Buffer$/
23-
);
24-
}
25-
2619
{
2720
const buf2 = Buffer.alloc(9);
2821
assert.strictEqual(buf2[funx](...args, true), result);

0 commit comments

Comments
 (0)