Skip to content

Commit 60b5b38

Browse files
BridgeARBridgeAR
committed
buffer: do not always use defaults
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: #20054 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
1 parent 198eb9c commit 60b5b38

7 files changed

+233
-238
lines changed

‎lib/internal/buffer.js

+20-12
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,8 @@ function boundsError(value, length, type) {
5959

6060
// Read integers.
6161
function readUIntLE(offset, byteLength) {
62+
if (offset === undefined)
63+
throw new ERR_INVALID_ARG_TYPE('offset', 'number', offset);
6264
if (byteLength === 6)
6365
return readUInt48LE(this, offset);
6466
if (byteLength === 5)
@@ -69,7 +71,7 @@ function readUIntLE(offset, byteLength) {
6971
return this.readUInt32LE(offset);
7072
if (byteLength === 2)
7173
return this.readUInt16LE(offset);
72-
if (byteLength === 1 || byteLength === undefined)
74+
if (byteLength === 1)
7375
return this.readUInt8(offset);
7476

7577
boundsError(byteLength, 6, 'byteLength');
@@ -146,6 +148,8 @@ function readUInt8(offset = 0) {
146148
}
147149

148150
function readUIntBE(offset, byteLength) {
151+
if (offset === undefined)
152+
throw new ERR_INVALID_ARG_TYPE('offset', 'number', offset);
149153
if (byteLength === 6)
150154
return readUInt48BE(this, offset);
151155
if (byteLength === 5)
@@ -156,7 +160,7 @@ function readUIntBE(offset, byteLength) {
156160
return this.readUInt32BE(offset);
157161
if (byteLength === 2)
158162
return this.readUInt16BE(offset);
159-
if (byteLength === 1 || byteLength === undefined)
163+
if (byteLength === 1)
160164
return this.readUInt8(offset);
161165

162166
boundsError(byteLength, 6, 'byteLength');
@@ -224,6 +228,8 @@ function readUInt16BE(offset = 0) {
224228
}
225229

226230
function readIntLE(offset, byteLength) {
231+
if (offset === undefined)
232+
throw new ERR_INVALID_ARG_TYPE('offset', 'number', offset);
227233
if (byteLength === 6)
228234
return readInt48LE(this, offset);
229235
if (byteLength === 5)
@@ -234,7 +240,7 @@ function readIntLE(offset, byteLength) {
234240
return this.readInt32LE(offset);
235241
if (byteLength === 2)
236242
return this.readInt16LE(offset);
237-
if (byteLength === 1 || byteLength === undefined)
243+
if (byteLength === 1)
238244
return this.readInt8(offset);
239245

240246
boundsError(byteLength, 6, 'byteLength');
@@ -314,6 +320,8 @@ function readInt8(offset = 0) {
314320
}
315321

316322
function readIntBE(offset, byteLength) {
323+
if (offset === undefined)
324+
throw new ERR_INVALID_ARG_TYPE('offset', 'number', offset);
317325
if (byteLength === 6)
318326
return readInt48BE(this, offset);
319327
if (byteLength === 5)
@@ -324,7 +332,7 @@ function readIntBE(offset, byteLength) {
324332
return this.readInt32BE(offset);
325333
if (byteLength === 2)
326334
return this.readInt16BE(offset);
327-
if (byteLength === 1 || byteLength === undefined)
335+
if (byteLength === 1)
328336
return this.readInt8(offset);
329337

330338
boundsError(byteLength, 6, 'byteLength');
@@ -460,7 +468,7 @@ function readDoubleForwards(offset = 0) {
460468
}
461469

462470
// Write integers.
463-
function writeUIntLE(value, offset = 0, byteLength) {
471+
function writeUIntLE(value, offset, byteLength) {
464472
if (byteLength === 6)
465473
return writeU_Int48LE(this, value, offset, 0, 0xffffffffffff);
466474
if (byteLength === 5)
@@ -471,7 +479,7 @@ function writeUIntLE(value, offset = 0, byteLength) {
471479
return writeU_Int32LE(this, value, offset, 0, 0xffffffff);
472480
if (byteLength === 2)
473481
return writeU_Int16LE(this, value, offset, 0, 0xffff);
474-
if (byteLength === 1 || byteLength === undefined)
482+
if (byteLength === 1)
475483
return writeU_Int8(this, value, offset, 0, 0xff);
476484

477485
boundsError(byteLength, 6, 'byteLength');
@@ -571,7 +579,7 @@ function writeUInt8(value, offset = 0) {
571579
return writeU_Int8(this, value, offset, 0, 0xff);
572580
}
573581

574-
function writeUIntBE(value, offset = 0, byteLength) {
582+
function writeUIntBE(value, offset, byteLength) {
575583
if (byteLength === 6)
576584
return writeU_Int48BE(this, value, offset, 0, 0xffffffffffffff);
577585
if (byteLength === 5)
@@ -582,7 +590,7 @@ function writeUIntBE(value, offset = 0, byteLength) {
582590
return writeU_Int32BE(this, value, offset, 0, 0xffffffff);
583591
if (byteLength === 2)
584592
return writeU_Int16BE(this, value, offset, 0, 0xffff);
585-
if (byteLength === 1 || byteLength === undefined)
593+
if (byteLength === 1)
586594
return writeU_Int8(this, value, offset, 0, 0xff);
587595

588596
boundsError(byteLength, 6, 'byteLength');
@@ -663,7 +671,7 @@ function writeUInt16BE(value, offset = 0) {
663671
return writeU_Int16BE(this, value, offset, 0, 0xffffffff);
664672
}
665673

666-
function writeIntLE(value, offset = 0, byteLength) {
674+
function writeIntLE(value, offset, byteLength) {
667675
if (byteLength === 6)
668676
return writeU_Int48LE(this, value, offset, -0x800000000000, 0x7fffffffffff);
669677
if (byteLength === 5)
@@ -674,7 +682,7 @@ function writeIntLE(value, offset = 0, byteLength) {
674682
return writeU_Int32LE(this, value, offset, -0x80000000, 0x7fffffff);
675683
if (byteLength === 2)
676684
return writeU_Int16LE(this, value, offset, -0x8000, 0x7fff);
677-
if (byteLength === 1 || byteLength === undefined)
685+
if (byteLength === 1)
678686
return writeU_Int8(this, value, offset, -0x80, 0x7f);
679687

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

695-
function writeIntBE(value, offset = 0, byteLength) {
703+
function writeIntBE(value, offset, byteLength) {
696704
if (byteLength === 6)
697705
return writeU_Int48BE(this, value, offset, -0x800000000000, 0x7fffffffffff);
698706
if (byteLength === 5)
@@ -703,7 +711,7 @@ function writeIntBE(value, offset = 0, byteLength) {
703711
return writeU_Int32BE(this, value, offset, -0x80000000, 0x7fffffff);
704712
if (byteLength === 2)
705713
return writeU_Int16BE(this, value, offset, -0x8000, 0x7fff);
706-
if (byteLength === 1 || byteLength === undefined)
714+
if (byteLength === 1)
707715
return writeU_Int8(this, value, offset, -0x80, 0x7f);
708716

709717
boundsError(byteLength, 6, 'byteLength');

‎test/parallel/test-buffer-read.js

+20-67
Original file line numberDiff line numberDiff line change
@@ -51,91 +51,44 @@ read(buf, 'readUInt32LE', [1], 0xcfea48fd);
5151
read(buf, 'readUIntBE', [2, 2], 0x48ea);
5252
read(buf, 'readUIntLE', [2, 2], 0xea48);
5353

54-
// invalid byteLength parameter for readUIntBE() and readUIntLE()
55-
common.expectsError(() => { buf.readUIntBE(2, 0); },
56-
{ code: 'ERR_OUT_OF_RANGE' });
57-
common.expectsError(() => { buf.readUIntLE(2, 7); },
58-
{ code: 'ERR_OUT_OF_RANGE' });
59-
60-
// attempt to overflow buffers, similar to previous bug in array buffers
54+
// Attempt to overflow buffers, similar to previous bug in array buffers
6155
assert.throws(() => Buffer.allocUnsafe(8).readFloatBE(0xffffffff),
6256
RangeError);
6357
assert.throws(() => Buffer.allocUnsafe(8).readFloatLE(0xffffffff),
6458
RangeError);
6559

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

70-
// offset checks
64+
// Offset checks
7165
{
7266
const buf = Buffer.allocUnsafe(0);
7367

7468
assert.throws(() => buf.readUInt8(0), RangeError);
7569
assert.throws(() => buf.readInt8(0), RangeError);
7670
}
7771

78-
{
79-
const buf = Buffer.from([0xFF]);
80-
81-
assert.strictEqual(buf.readUInt8(0), 255);
82-
assert.strictEqual(buf.readInt8(0), -1);
83-
}
84-
85-
[16, 32].forEach((bits) => {
86-
const buf = Buffer.allocUnsafe(bits / 8 - 1);
87-
88-
assert.throws(() => buf[`readUInt${bits}BE`](0),
89-
RangeError,
90-
`readUInt${bits}BE()`);
91-
92-
assert.throws(() => buf[`readUInt${bits}LE`](0),
93-
RangeError,
94-
`readUInt${bits}LE()`);
95-
96-
assert.throws(() => buf[`readInt${bits}BE`](0),
97-
RangeError,
98-
`readInt${bits}BE()`);
99-
100-
assert.throws(() => buf[`readInt${bits}LE`](0),
101-
RangeError,
102-
`readInt${bits}LE()`);
72+
[16, 32].forEach((bit) => {
73+
const buf = Buffer.allocUnsafe(bit / 8 - 1);
74+
[`Int${bit}B`, `Int${bit}L`, `UInt${bit}B`, `UInt${bit}L`].forEach((fn) => {
75+
assert.throws(
76+
() => buf[`read${fn}E`](0),
77+
{
78+
name: 'RangeError [ERR_BUFFER_OUT_OF_BOUNDS]',
79+
message: 'Attempt to write outside buffer bounds'
80+
}
81+
);
82+
});
10383
});
10484

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

108-
assert.strictEqual(buf[`readUInt${bits}BE`](0),
109-
(0xFFFFFFFF >>> (32 - bits)));
110-
111-
assert.strictEqual(buf[`readUInt${bits}LE`](0),
112-
(0xFFFFFFFF >>> (32 - bits)));
113-
114-
assert.strictEqual(buf[`readInt${bits}BE`](0),
115-
(0xFFFFFFFF >> (32 - bits)));
116-
117-
assert.strictEqual(buf[`readInt${bits}LE`](0),
118-
(0xFFFFFFFF >> (32 - bits)));
91+
assert.strictEqual(buf[`readInt${bits}${endian}`](0),
92+
(0xFFFFFFFF >> (32 - bits)));
93+
});
11994
});
120-
121-
// Test for common read(U)IntLE/BE
122-
{
123-
const buf = Buffer.from([0x01, 0x02, 0x03, 0x04, 0x05, 0x06]);
124-
125-
assert.strictEqual(buf.readUIntLE(0, 1), 0x01);
126-
assert.strictEqual(buf.readUIntBE(0, 1), 0x01);
127-
assert.strictEqual(buf.readUIntLE(0, 3), 0x030201);
128-
assert.strictEqual(buf.readUIntBE(0, 3), 0x010203);
129-
assert.strictEqual(buf.readUIntLE(0, 5), 0x0504030201);
130-
assert.strictEqual(buf.readUIntBE(0, 5), 0x0102030405);
131-
assert.strictEqual(buf.readUIntLE(0, 6), 0x060504030201);
132-
assert.strictEqual(buf.readUIntBE(0, 6), 0x010203040506);
133-
assert.strictEqual(buf.readIntLE(0, 1), 0x01);
134-
assert.strictEqual(buf.readIntBE(0, 1), 0x01);
135-
assert.strictEqual(buf.readIntLE(0, 3), 0x030201);
136-
assert.strictEqual(buf.readIntBE(0, 3), 0x010203);
137-
assert.strictEqual(buf.readIntLE(0, 5), 0x0504030201);
138-
assert.strictEqual(buf.readIntBE(0, 5), 0x0102030405);
139-
assert.strictEqual(buf.readIntLE(0, 6), 0x060504030201);
140-
assert.strictEqual(buf.readIntBE(0, 6), 0x010203040506);
141-
}

‎test/parallel/test-buffer-readint.js

+19-28
Original file line numberDiff line numberDiff line change
@@ -46,18 +46,14 @@ const assert = require('assert');
4646

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

51-
data[0] = 0x23;
5251
assert.strictEqual(data.readInt8(0), 0x23);
5352

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

5756
data[0] = 0x87;
58-
data[1] = 0xab;
59-
data[2] = 0x7c;
60-
data[3] = 0xef;
6157
assert.strictEqual(data.readInt8(0), -121);
6258
assert.strictEqual(data.readInt8(1), -85);
6359
assert.strictEqual(data.readInt8(2), 124);
@@ -66,10 +62,8 @@ const assert = require('assert');
6662

6763
// Test 16 bit integers
6864
{
69-
const buffer = Buffer.alloc(6);
65+
const buffer = Buffer.from([0x16, 0x79, 0x65, 0x6e, 0x69, 0x78]);
7066

71-
buffer[0] = 0x16;
72-
buffer[1] = 0x79;
7367
assert.strictEqual(buffer.readInt16BE(0), 0x1679);
7468
assert.strictEqual(buffer.readInt16LE(0), 0x7916);
7569

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

8175
buffer[0] = 0x77;
8276
buffer[1] = 0x65;
83-
buffer[2] = 0x65;
84-
buffer[3] = 0x6e;
85-
buffer[4] = 0x69;
86-
buffer[5] = 0x78;
8777
assert.strictEqual(buffer.readInt16BE(0), 0x7765);
8878
assert.strictEqual(buffer.readInt16BE(1), 0x6565);
8979
assert.strictEqual(buffer.readInt16BE(2), 0x656e);
@@ -98,12 +88,8 @@ const assert = require('assert');
9888

9989
// Test 32 bit integers
10090
{
101-
const buffer = Buffer.alloc(6);
91+
const buffer = Buffer.from([0x43, 0x53, 0x16, 0x79, 0x36, 0x17]);
10292

103-
buffer[0] = 0x43;
104-
buffer[1] = 0x53;
105-
buffer[2] = 0x16;
106-
buffer[3] = 0x79;
10793
assert.strictEqual(buffer.readInt32BE(0), 0x43531679);
10894
assert.strictEqual(buffer.readInt32LE(0), 0x79165343);
10995

@@ -118,8 +104,6 @@ const assert = require('assert');
118104
buffer[1] = 0xc3;
119105
buffer[2] = 0x95;
120106
buffer[3] = 0xa9;
121-
buffer[4] = 0x36;
122-
buffer[5] = 0x17;
123107
assert.strictEqual(buffer.readInt32BE(0), 0x42c395a9);
124108
assert.strictEqual(buffer.readInt32BE(1), -1013601994);
125109
assert.strictEqual(buffer.readInt32BE(2), -1784072681);
@@ -130,17 +114,24 @@ const assert = require('assert');
130114

131115
// Test Int
132116
{
133-
const buffer = Buffer.alloc(8);
117+
const buffer = Buffer.from([0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08]);
118+
119+
assert.strictEqual(buffer.readIntLE(0, 1), 0x01);
120+
assert.strictEqual(buffer.readIntBE(0, 1), 0x01);
121+
assert.strictEqual(buffer.readIntLE(0, 3), 0x030201);
122+
assert.strictEqual(buffer.readIntBE(0, 3), 0x010203);
123+
assert.strictEqual(buffer.readIntLE(0, 5), 0x0504030201);
124+
assert.strictEqual(buffer.readIntBE(0, 5), 0x0102030405);
125+
assert.strictEqual(buffer.readIntLE(0, 6), 0x060504030201);
126+
assert.strictEqual(buffer.readIntBE(0, 6), 0x010203040506);
127+
assert.strictEqual(buffer.readIntLE(1, 6), 0x070605040302);
128+
assert.strictEqual(buffer.readIntBE(1, 6), 0x020304050607);
129+
assert.strictEqual(buffer.readIntLE(2, 6), 0x080706050403);
130+
assert.strictEqual(buffer.readIntBE(2, 6), 0x030405060708);
134131

135132
// Check byteLength.
136133
['readIntBE', 'readIntLE'].forEach((fn) => {
137-
138-
// Verify that default offset & byteLength works fine.
139-
buffer[fn](undefined, undefined);
140-
buffer[fn](undefined);
141-
buffer[fn]();
142-
143-
['', '0', null, {}, [], () => {}, true, false].forEach((len) => {
134+
['', '0', null, {}, [], () => {}, true, false, undefined].forEach((len) => {
144135
assert.throws(
145136
() => buffer[fn](0, len),
146137
{ code: 'ERR_INVALID_ARG_TYPE' });
@@ -171,7 +162,7 @@ const assert = require('assert');
171162
// Test 1 to 6 bytes.
172163
for (let i = 1; i < 6; i++) {
173164
['readIntBE', 'readIntLE'].forEach((fn) => {
174-
['', '0', null, {}, [], () => {}, true, false].forEach((o) => {
165+
['', '0', null, {}, [], () => {}, true, false, undefined].forEach((o) => {
175166
assert.throws(
176167
() => buffer[fn](o, i),
177168
{

0 commit comments

Comments
 (0)