Skip to content

Commit

Permalink
buffer: do not always use defaults
Browse files Browse the repository at this point in the history 
The Buffer#(read|write)U?Int(B|L)E functions should not use a default
value. This is very likely a bug and it was never documented that
way.

Besides that this also improves the tests by adding more tests and by
refactoring them to less code lines.

PR-URL: nodejs#20054
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
  • Loading branch information
@BridgeAR
BridgeAR committed Apr 29, 2018
1 parent 198eb9c commit 60b5b38
Show file tree
Hide file tree
Showing 7 changed files with 233 additions and 238 deletions.
32 changes: 20 additions & 12 deletions lib/internal/buffer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ function boundsError(value, length, type) {

// Read integers.
function readUIntLE(offset, byteLength) {
if (offset === undefined)
throw new ERR_INVALID_ARG_TYPE('offset', 'number', offset);
if (byteLength === 6)
return readUInt48LE(this, offset);
if (byteLength === 5)
Expand All @@ -69,7 +71,7 @@ function readUIntLE(offset, byteLength) {
return this.readUInt32LE(offset);
if (byteLength === 2)
return this.readUInt16LE(offset);
if (byteLength === 1 || byteLength === undefined)
if (byteLength === 1)
return this.readUInt8(offset);

boundsError(byteLength, 6, 'byteLength');
Expand Down Expand Up @@ -146,6 +148,8 @@ function readUInt8(offset = 0) {
}

function readUIntBE(offset, byteLength) {
if (offset === undefined)
throw new ERR_INVALID_ARG_TYPE('offset', 'number', offset);
if (byteLength === 6)
return readUInt48BE(this, offset);
if (byteLength === 5)
Expand All @@ -156,7 +160,7 @@ function readUIntBE(offset, byteLength) {
return this.readUInt32BE(offset);
if (byteLength === 2)
return this.readUInt16BE(offset);
if (byteLength === 1 || byteLength === undefined)
if (byteLength === 1)
return this.readUInt8(offset);

boundsError(byteLength, 6, 'byteLength');
Expand Down Expand Up @@ -224,6 +228,8 @@ function readUInt16BE(offset = 0) {
}

function readIntLE(offset, byteLength) {
if (offset === undefined)
throw new ERR_INVALID_ARG_TYPE('offset', 'number', offset);
if (byteLength === 6)
return readInt48LE(this, offset);
if (byteLength === 5)
Expand All @@ -234,7 +240,7 @@ function readIntLE(offset, byteLength) {
return this.readInt32LE(offset);
if (byteLength === 2)
return this.readInt16LE(offset);
if (byteLength === 1 || byteLength === undefined)
if (byteLength === 1)
return this.readInt8(offset);

boundsError(byteLength, 6, 'byteLength');
Expand Down Expand Up @@ -314,6 +320,8 @@ function readInt8(offset = 0) {
}

function readIntBE(offset, byteLength) {
if (offset === undefined)
throw new ERR_INVALID_ARG_TYPE('offset', 'number', offset);
if (byteLength === 6)
return readInt48BE(this, offset);
if (byteLength === 5)
Expand All @@ -324,7 +332,7 @@ function readIntBE(offset, byteLength) {
return this.readInt32BE(offset);
if (byteLength === 2)
return this.readInt16BE(offset);
if (byteLength === 1 || byteLength === undefined)
if (byteLength === 1)
return this.readInt8(offset);

boundsError(byteLength, 6, 'byteLength');
Expand Down Expand Up @@ -460,7 +468,7 @@ function readDoubleForwards(offset = 0) {
}

// Write integers.
function writeUIntLE(value, offset = 0, byteLength) {
function writeUIntLE(value, offset, byteLength) {
if (byteLength === 6)
return writeU_Int48LE(this, value, offset, 0, 0xffffffffffff);
if (byteLength === 5)
Expand All @@ -471,7 +479,7 @@ function writeUIntLE(value, offset = 0, byteLength) {
return writeU_Int32LE(this, value, offset, 0, 0xffffffff);
if (byteLength === 2)
return writeU_Int16LE(this, value, offset, 0, 0xffff);
if (byteLength === 1 || byteLength === undefined)
if (byteLength === 1)
return writeU_Int8(this, value, offset, 0, 0xff);

boundsError(byteLength, 6, 'byteLength');
Expand Down Expand Up @@ -571,7 +579,7 @@ function writeUInt8(value, offset = 0) {
return writeU_Int8(this, value, offset, 0, 0xff);
}

function writeUIntBE(value, offset = 0, byteLength) {
function writeUIntBE(value, offset, byteLength) {
if (byteLength === 6)
return writeU_Int48BE(this, value, offset, 0, 0xffffffffffffff);
if (byteLength === 5)
Expand All @@ -582,7 +590,7 @@ function writeUIntBE(value, offset = 0, byteLength) {
return writeU_Int32BE(this, value, offset, 0, 0xffffffff);
if (byteLength === 2)
return writeU_Int16BE(this, value, offset, 0, 0xffff);
if (byteLength === 1 || byteLength === undefined)
if (byteLength === 1)
return writeU_Int8(this, value, offset, 0, 0xff);

boundsError(byteLength, 6, 'byteLength');
Expand Down Expand Up @@ -663,7 +671,7 @@ function writeUInt16BE(value, offset = 0) {
return writeU_Int16BE(this, value, offset, 0, 0xffffffff);
}

function writeIntLE(value, offset = 0, byteLength) {
function writeIntLE(value, offset, byteLength) {
if (byteLength === 6)
return writeU_Int48LE(this, value, offset, -0x800000000000, 0x7fffffffffff);
if (byteLength === 5)
Expand All @@ -674,7 +682,7 @@ function writeIntLE(value, offset = 0, byteLength) {
return writeU_Int32LE(this, value, offset, -0x80000000, 0x7fffffff);
if (byteLength === 2)
return writeU_Int16LE(this, value, offset, -0x8000, 0x7fff);
if (byteLength === 1 || byteLength === undefined)
if (byteLength === 1)
return writeU_Int8(this, value, offset, -0x80, 0x7f);

boundsError(byteLength, 6, 'byteLength');
Expand All @@ -692,7 +700,7 @@ function writeInt8(value, offset = 0) {
return writeU_Int8(this, value, offset, -0x80, 0x7f);
}

function writeIntBE(value, offset = 0, byteLength) {
function writeIntBE(value, offset, byteLength) {
if (byteLength === 6)
return writeU_Int48BE(this, value, offset, -0x800000000000, 0x7fffffffffff);
if (byteLength === 5)
Expand All @@ -703,7 +711,7 @@ function writeIntBE(value, offset = 0, byteLength) {
return writeU_Int32BE(this, value, offset, -0x80000000, 0x7fffffff);
if (byteLength === 2)
return writeU_Int16BE(this, value, offset, -0x8000, 0x7fff);
if (byteLength === 1 || byteLength === undefined)
if (byteLength === 1)
return writeU_Int8(this, value, offset, -0x80, 0x7f);

boundsError(byteLength, 6, 'byteLength');
Expand Down
87 changes: 20 additions & 67 deletions test/parallel/test-buffer-read.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,91 +51,44 @@ read(buf, 'readUInt32LE', [1], 0xcfea48fd);
read(buf, 'readUIntBE', [2, 2], 0x48ea);
read(buf, 'readUIntLE', [2, 2], 0xea48);

// invalid byteLength parameter for readUIntBE() and readUIntLE()
common.expectsError(() => { buf.readUIntBE(2, 0); },
{ code: 'ERR_OUT_OF_RANGE' });
common.expectsError(() => { buf.readUIntLE(2, 7); },
{ code: 'ERR_OUT_OF_RANGE' });

// attempt to overflow buffers, similar to previous bug in array buffers
// Attempt to overflow buffers, similar to previous bug in array buffers
assert.throws(() => Buffer.allocUnsafe(8).readFloatBE(0xffffffff),
RangeError);
assert.throws(() => Buffer.allocUnsafe(8).readFloatLE(0xffffffff),
RangeError);

// ensure negative values can't get past offset
// Ensure negative values can't get past offset
assert.throws(() => Buffer.allocUnsafe(8).readFloatBE(-1), RangeError);
assert.throws(() => Buffer.allocUnsafe(8).readFloatLE(-1), RangeError);

// offset checks
// Offset checks
{
const buf = Buffer.allocUnsafe(0);

assert.throws(() => buf.readUInt8(0), RangeError);
assert.throws(() => buf.readInt8(0), RangeError);
}

{
const buf = Buffer.from([0xFF]);

assert.strictEqual(buf.readUInt8(0), 255);
assert.strictEqual(buf.readInt8(0), -1);
}

[16, 32].forEach((bits) => {
const buf = Buffer.allocUnsafe(bits / 8 - 1);

assert.throws(() => buf[`readUInt${bits}BE`](0),
RangeError,
`readUInt${bits}BE()`);

assert.throws(() => buf[`readUInt${bits}LE`](0),
RangeError,
`readUInt${bits}LE()`);

assert.throws(() => buf[`readInt${bits}BE`](0),
RangeError,
`readInt${bits}BE()`);

assert.throws(() => buf[`readInt${bits}LE`](0),
RangeError,
`readInt${bits}LE()`);
[16, 32].forEach((bit) => {
const buf = Buffer.allocUnsafe(bit / 8 - 1);
[`Int${bit}B`, `Int${bit}L`, `UInt${bit}B`, `UInt${bit}L`].forEach((fn) => {
assert.throws(
() => buf[`read${fn}E`](0),
{
name: 'RangeError [ERR_BUFFER_OUT_OF_BOUNDS]',
message: 'Attempt to write outside buffer bounds'
}
);
});
});

[16, 32].forEach((bits) => {
const buf = Buffer.from([0xFF, 0xFF, 0xFF, 0xFF]);
['LE', 'BE'].forEach((endian) => {
assert.strictEqual(buf[`readUInt${bits}${endian}`](0),
(0xFFFFFFFF >>> (32 - bits)));

assert.strictEqual(buf[`readUInt${bits}BE`](0),
(0xFFFFFFFF >>> (32 - bits)));

assert.strictEqual(buf[`readUInt${bits}LE`](0),
(0xFFFFFFFF >>> (32 - bits)));

assert.strictEqual(buf[`readInt${bits}BE`](0),
(0xFFFFFFFF >> (32 - bits)));

assert.strictEqual(buf[`readInt${bits}LE`](0),
(0xFFFFFFFF >> (32 - bits)));
assert.strictEqual(buf[`readInt${bits}${endian}`](0),
(0xFFFFFFFF >> (32 - bits)));
});
});

// Test for common read(U)IntLE/BE
{
const buf = Buffer.from([0x01, 0x02, 0x03, 0x04, 0x05, 0x06]);

assert.strictEqual(buf.readUIntLE(0, 1), 0x01);
assert.strictEqual(buf.readUIntBE(0, 1), 0x01);
assert.strictEqual(buf.readUIntLE(0, 3), 0x030201);
assert.strictEqual(buf.readUIntBE(0, 3), 0x010203);
assert.strictEqual(buf.readUIntLE(0, 5), 0x0504030201);
assert.strictEqual(buf.readUIntBE(0, 5), 0x0102030405);
assert.strictEqual(buf.readUIntLE(0, 6), 0x060504030201);
assert.strictEqual(buf.readUIntBE(0, 6), 0x010203040506);
assert.strictEqual(buf.readIntLE(0, 1), 0x01);
assert.strictEqual(buf.readIntBE(0, 1), 0x01);
assert.strictEqual(buf.readIntLE(0, 3), 0x030201);
assert.strictEqual(buf.readIntBE(0, 3), 0x010203);
assert.strictEqual(buf.readIntLE(0, 5), 0x0504030201);
assert.strictEqual(buf.readIntBE(0, 5), 0x0102030405);
assert.strictEqual(buf.readIntLE(0, 6), 0x060504030201);
assert.strictEqual(buf.readIntBE(0, 6), 0x010203040506);
}
47 changes: 19 additions & 28 deletions test/parallel/test-buffer-readint.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,18 +46,14 @@ const assert = require('assert');

// Test 8 bit signed integers
{
const data = Buffer.alloc(4);
const data = Buffer.from([0x23, 0xab, 0x7c, 0xef]);

data[0] = 0x23;
assert.strictEqual(data.readInt8(0), 0x23);

data[0] = 0xff;
assert.strictEqual(data.readInt8(0), -1);

data[0] = 0x87;
data[1] = 0xab;
data[2] = 0x7c;
data[3] = 0xef;
assert.strictEqual(data.readInt8(0), -121);
assert.strictEqual(data.readInt8(1), -85);
assert.strictEqual(data.readInt8(2), 124);
Expand All @@ -66,10 +62,8 @@ const assert = require('assert');

// Test 16 bit integers
{
const buffer = Buffer.alloc(6);
const buffer = Buffer.from([0x16, 0x79, 0x65, 0x6e, 0x69, 0x78]);

buffer[0] = 0x16;
buffer[1] = 0x79;
assert.strictEqual(buffer.readInt16BE(0), 0x1679);
assert.strictEqual(buffer.readInt16LE(0), 0x7916);

Expand All @@ -80,10 +74,6 @@ const assert = require('assert');

buffer[0] = 0x77;
buffer[1] = 0x65;
buffer[2] = 0x65;
buffer[3] = 0x6e;
buffer[4] = 0x69;
buffer[5] = 0x78;
assert.strictEqual(buffer.readInt16BE(0), 0x7765);
assert.strictEqual(buffer.readInt16BE(1), 0x6565);
assert.strictEqual(buffer.readInt16BE(2), 0x656e);
Expand All @@ -98,12 +88,8 @@ const assert = require('assert');

// Test 32 bit integers
{
const buffer = Buffer.alloc(6);
const buffer = Buffer.from([0x43, 0x53, 0x16, 0x79, 0x36, 0x17]);

buffer[0] = 0x43;
buffer[1] = 0x53;
buffer[2] = 0x16;
buffer[3] = 0x79;
assert.strictEqual(buffer.readInt32BE(0), 0x43531679);
assert.strictEqual(buffer.readInt32LE(0), 0x79165343);

Expand All @@ -118,8 +104,6 @@ const assert = require('assert');
buffer[1] = 0xc3;
buffer[2] = 0x95;
buffer[3] = 0xa9;
buffer[4] = 0x36;
buffer[5] = 0x17;
assert.strictEqual(buffer.readInt32BE(0), 0x42c395a9);
assert.strictEqual(buffer.readInt32BE(1), -1013601994);
assert.strictEqual(buffer.readInt32BE(2), -1784072681);
Expand All @@ -130,17 +114,24 @@ const assert = require('assert');

// Test Int
{
const buffer = Buffer.alloc(8);
const buffer = Buffer.from([0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08]);

assert.strictEqual(buffer.readIntLE(0, 1), 0x01);
assert.strictEqual(buffer.readIntBE(0, 1), 0x01);
assert.strictEqual(buffer.readIntLE(0, 3), 0x030201);
assert.strictEqual(buffer.readIntBE(0, 3), 0x010203);
assert.strictEqual(buffer.readIntLE(0, 5), 0x0504030201);
assert.strictEqual(buffer.readIntBE(0, 5), 0x0102030405);
assert.strictEqual(buffer.readIntLE(0, 6), 0x060504030201);
assert.strictEqual(buffer.readIntBE(0, 6), 0x010203040506);
assert.strictEqual(buffer.readIntLE(1, 6), 0x070605040302);
assert.strictEqual(buffer.readIntBE(1, 6), 0x020304050607);
assert.strictEqual(buffer.readIntLE(2, 6), 0x080706050403);
assert.strictEqual(buffer.readIntBE(2, 6), 0x030405060708);

// Check byteLength.
['readIntBE', 'readIntLE'].forEach((fn) => {

// Verify that default offset & byteLength works fine.
buffer[fn](undefined, undefined);
buffer[fn](undefined);
buffer[fn]();

['', '0', null, {}, [], () => {}, true, false].forEach((len) => {
['', '0', null, {}, [], () => {}, true, false, undefined].forEach((len) => {
assert.throws(
() => buffer[fn](0, len),
{ code: 'ERR_INVALID_ARG_TYPE' });
Expand Down Expand Up @@ -171,7 +162,7 @@ const assert = require('assert');
// Test 1 to 6 bytes.
for (let i = 1; i < 6; i++) {
['readIntBE', 'readIntLE'].forEach((fn) => {
['', '0', null, {}, [], () => {}, true, false].forEach((o) => {
['', '0', null, {}, [], () => {}, true, false, undefined].forEach((o) => {
assert.throws(
() => buffer[fn](o, i),
{
Expand Down
Loading

0 comments on commit 60b5b38

Please sign in to comment.