Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit b3ec13d

Browse filesBrowse files
LiviaMedeirosLiviaMedeiros
authored
fs: adjust position validation in reading methods
This prohibits invalid values (< -1 and non-integers) and allows `filehandle.read()` to handle position up to `2n ** 63n - 1n` PR-URL: #42835 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 7f0e36a commit b3ec13d
Copy full SHA for b3ec13d

File tree

Expand file treeCollapse file tree

7 files changed

+133
-31
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

7 files changed

+133
-31
lines changed
Open diff view settings
Collapse file

‎doc/api/fs.md‎

Copy file name to clipboardExpand all lines: doc/api/fs.md
+28-14Lines changed: 28 additions & 14 deletions
  • Display the source diff
  • Display the rich diff
Original file line numberDiff line numberDiff line change
@@ -369,16 +369,20 @@ added: v10.0.0
369369
370370
<!-- YAML
371371
added: v10.0.0
372+
changes:
373+
- version: REPLACEME
374+
pr-url: https://github.com/nodejs/node/pull/42835
375+
description: Accepts bigint values as `position`.
372376
-->
373377
374378
* `buffer` {Buffer|TypedArray|DataView} A buffer that will be filled with the
375379
file data read.
376380
* `offset` {integer} The location in the buffer at which to start filling.
377381
* `length` {integer} The number of bytes to read.
378-
* `position` {integer|null} The location where to begin reading data from the
379-
file. If `null`, data will be read from the current file position, and
380-
the position will be updated. If `position` is an integer, the current
381-
file position will remain unchanged.
382+
* `position` {integer|bigint|null} The location where to begin reading data
383+
from the file. If `null` or `-1`, data will be read from the current file
384+
position, and the position will be updated. If `position` is a non-negative
385+
integer, the current file position will remain unchanged.
382386
* Returns: {Promise} Fulfills upon success with an object with two properties:
383387
* `bytesRead` {integer} The number of bytes read
384388
* `buffer` {Buffer|TypedArray|DataView} A reference to the passed in `buffer`
@@ -395,6 +399,10 @@ number of bytes read is zero.
395399
added:
396400
- v13.11.0
397401
- v12.17.0
402+
changes:
403+
- version: REPLACEME
404+
pr-url: https://github.com/nodejs/node/pull/42835
405+
description: Accepts bigint values as `position`.
398406
-->
399407
400408
* `options` {Object}
@@ -404,10 +412,11 @@ added:
404412
**Default:** `0`
405413
* `length` {integer} The number of bytes to read. **Default:**
406414
`buffer.byteLength - offset`
407-
* `position` {integer|null} The location where to begin reading data from the
408-
file. If `null`, data will be read from the current file position, and
409-
the position will be updated. If `position` is an integer, the current
410-
file position will remain unchanged. **Default:**: `null`
415+
* `position` {integer|bigint|null} The location where to begin reading data
416+
from the file. If `null` or `-1`, data will be read from the current file
417+
position, and the position will be updated. If `position` is a non-negative
418+
integer, the current file position will remain unchanged.
419+
**Default:**: `null`
411420
* Returns: {Promise} Fulfills upon success with an object with two properties:
412421
* `bytesRead` {integer} The number of bytes read
413422
* `buffer` {Buffer|TypedArray|DataView} A reference to the passed in `buffer`
@@ -424,6 +433,10 @@ number of bytes read is zero.
424433
added:
425434
- v18.2.0
426435
- v16.17.0
436+
changes:
437+
- version: REPLACEME
438+
pr-url: https://github.com/nodejs/node/pull/42835
439+
description: Accepts bigint values as `position`.
427440
-->
428441
429442
* `buffer` {Buffer|TypedArray|DataView} A buffer that will be filled with the
@@ -433,10 +446,11 @@ added:
433446
**Default:** `0`
434447
* `length` {integer} The number of bytes to read. **Default:**
435448
`buffer.byteLength - offset`
436-
* `position` {integer} The location where to begin reading data from the
437-
file. If `null`, data will be read from the current file position, and
438-
the position will be updated. If `position` is an integer, the current
439-
file position will remain unchanged. **Default:**: `null`
449+
* `position` {integer|bigint|null} The location where to begin reading data
450+
from the file. If `null` or `-1`, data will be read from the current file
451+
position, and the position will be updated. If `position` is a non-negative
452+
integer, the current file position will remain unchanged.
453+
**Default:**: `null`
440454
* Returns: {Promise} Fulfills upon success with an object with two properties:
441455
* `bytesRead` {integer} The number of bytes read
442456
* `buffer` {Buffer|TypedArray|DataView} A reference to the passed in `buffer`
@@ -3514,8 +3528,8 @@ changes:
35143528
* `length` {integer} The number of bytes to read.
35153529
* `position` {integer|bigint|null} Specifies where to begin reading from in the
35163530
file. If `position` is `null` or `-1 `, data will be read from the current
3517-
file position, and the file position will be updated. If `position` is an
3518-
integer, the file position will be unchanged.
3531+
file position, and the file position will be updated. If `position` is
3532+
a non-negative integer, the file position will be unchanged.
35193533
* `callback` {Function}
35203534
* `err` {Error}
35213535
* `bytesRead` {integer}
Collapse file

‎lib/fs.js‎

Copy file name to clipboardExpand all lines: lib/fs.js
+8-6Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -656,10 +656,11 @@ function read(fd, buffer, offsetOrOptions, length, position, callback) {
656656

657657
validateOffsetLengthRead(offset, length, buffer.byteLength);
658658

659-
if (position == null)
659+
if (position == null) {
660660
position = -1;
661-
662-
validatePosition(position, 'position');
661+
} else {
662+
validatePosition(position, 'position', length);
663+
}
663664

664665
function wrapper(err, bytesRead) {
665666
// Retain a reference to buffer so that it can't be GC'ed too soon.
@@ -724,10 +725,11 @@ function readSync(fd, buffer, offsetOrOptions, length, position) {
724725

725726
validateOffsetLengthRead(offset, length, buffer.byteLength);
726727

727-
if (position == null)
728+
if (position == null) {
728729
position = -1;
729-
730-
validatePosition(position, 'position');
730+
} else {
731+
validatePosition(position, 'position', length);
732+
}
731733

732734
const ctx = {};
733735
const result = binding.read(fd, buffer, offset, length, position,
Collapse file

‎lib/internal/fs/promises.js‎

Copy file name to clipboardExpand all lines: lib/internal/fs/promises.js
+5-2Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ const {
66
Error,
77
MathMax,
88
MathMin,
9-
NumberIsSafeInteger,
109
Promise,
1110
PromisePrototypeThen,
1211
PromiseResolve,
@@ -67,6 +66,7 @@ const {
6766
validateCpOptions,
6867
validateOffsetLengthRead,
6968
validateOffsetLengthWrite,
69+
validatePosition,
7070
validateRmOptions,
7171
validateRmdirOptions,
7272
validateStringAfterArrayBufferView,
@@ -636,8 +636,11 @@ async function read(handle, bufferOrParams, offset, length, position) {
636636

637637
validateOffsetLengthRead(offset, length, buffer.byteLength);
638638

639-
if (!NumberIsSafeInteger(position))
639+
if (position == null) {
640640
position = -1;
641+
} else {
642+
validatePosition(position, 'position', length);
643+
}
641644

642645
const bytesRead = (await binding.read(handle.fd, buffer, offset, length,
643646
position, kUsePromises)) || 0;
Collapse file

‎lib/internal/fs/utils.js‎

Copy file name to clipboardExpand all lines: lib/internal/fs/utils.js
+5-4Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -916,13 +916,14 @@ const validateStringAfterArrayBufferView = hideStackFrames((buffer, name) => {
916916
}
917917
});
918918

919-
const validatePosition = hideStackFrames((position, name) => {
919+
const validatePosition = hideStackFrames((position, name, length) => {
920920
if (typeof position === 'number') {
921-
validateInteger(position, name);
921+
validateInteger(position, name, -1);
922922
} else if (typeof position === 'bigint') {
923-
if (!(position >= -(2n ** 63n) && position <= 2n ** 63n - 1n)) {
923+
const maxPosition = 2n ** 63n - 1n - BigInt(length);
924+
if (!(position >= -1n && position <= maxPosition)) {
924925
throw new ERR_OUT_OF_RANGE(name,
925-
`>= ${-(2n ** 63n)} && <= ${2n ** 63n - 1n}`,
926+
`>= -1 && <= ${maxPosition}`,
926927
position);
927928
}
928929
} else {
Collapse file

‎test/parallel/test-fs-read-position-validation.mjs‎

Copy file name to clipboardExpand all lines: test/parallel/test-fs-read-position-validation.mjs
+1-2Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,8 +75,7 @@ async function testInvalid(code, position) {
7575

7676
await testValid(2n ** 63n - 1n - BigInt(length), [ 'EFBIG', 'EOVERFLOW' ]);
7777
await testInvalid('ERR_OUT_OF_RANGE', 2n ** 63n);
78-
79-
// TODO(LiviaMedeiros): test `2n ** 63n - BigInt(length)`
78+
await testInvalid('ERR_OUT_OF_RANGE', 2n ** 63n - BigInt(length));
8079

8180
await testInvalid('ERR_OUT_OF_RANGE', NaN);
8281
await testInvalid('ERR_OUT_OF_RANGE', -Infinity);
Collapse file

‎test/parallel/test-fs-read-promises-position-validation.mjs‎

Copy file name to clipboard
+84Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
import '../common/index.mjs';
2+
import * as fixtures from '../common/fixtures.mjs';
3+
import fs from 'fs';
4+
import assert from 'assert';
5+
6+
// This test ensures that "position" argument is correctly validated
7+
8+
const filepath = fixtures.path('x.txt');
9+
10+
const buffer = Buffer.from('xyz\n');
11+
const offset = 0;
12+
const length = buffer.byteLength;
13+
14+
// allowedErrors is an array of acceptable internal errors
15+
// For example, on some platforms read syscall might return -EFBIG
16+
async function testValid(position, allowedErrors = []) {
17+
let fh;
18+
try {
19+
fh = await fs.promises.open(filepath, 'r');
20+
await fh.read(buffer, offset, length, position);
21+
await fh.read({ buffer, offset, length, position });
22+
await fh.read(buffer, { offset, length, position });
23+
} catch (err) {
24+
if (!allowedErrors.includes(err.code)) {
25+
assert.fail(err);
26+
}
27+
} finally {
28+
await fh?.close();
29+
}
30+
}
31+
32+
async function testInvalid(code, position) {
33+
let fh;
34+
try {
35+
fh = await fs.promises.open(filepath, 'r');
36+
await assert.rejects(
37+
fh.read(buffer, offset, length, position),
38+
{ code }
39+
);
40+
await assert.rejects(
41+
fh.read({ buffer, offset, length, position }),
42+
{ code }
43+
);
44+
await assert.rejects(
45+
fh.read(buffer, { offset, length, position }),
46+
{ code }
47+
);
48+
} finally {
49+
await fh?.close();
50+
}
51+
}
52+
53+
{
54+
await testValid(undefined);
55+
await testValid(null);
56+
await testValid(-1);
57+
await testValid(-1n);
58+
59+
await testValid(0);
60+
await testValid(0n);
61+
await testValid(1);
62+
await testValid(1n);
63+
await testValid(9);
64+
await testValid(9n);
65+
await testValid(Number.MAX_SAFE_INTEGER, [ 'EFBIG' ]);
66+
67+
await testValid(2n ** 63n - 1n - BigInt(length), [ 'EFBIG' ]);
68+
await testInvalid('ERR_OUT_OF_RANGE', 2n ** 63n);
69+
await testInvalid('ERR_OUT_OF_RANGE', 2n ** 63n - BigInt(length));
70+
71+
await testInvalid('ERR_OUT_OF_RANGE', NaN);
72+
await testInvalid('ERR_OUT_OF_RANGE', -Infinity);
73+
await testInvalid('ERR_OUT_OF_RANGE', Infinity);
74+
await testInvalid('ERR_OUT_OF_RANGE', -0.999);
75+
await testInvalid('ERR_OUT_OF_RANGE', -(2n ** 64n));
76+
await testInvalid('ERR_OUT_OF_RANGE', Number.MAX_SAFE_INTEGER + 1);
77+
await testInvalid('ERR_OUT_OF_RANGE', Number.MAX_VALUE);
78+
79+
for (const badTypeValue of [
80+
false, true, '1', Symbol(1), {}, [], () => {}, Promise.resolve(1),
81+
]) {
82+
testInvalid('ERR_INVALID_ARG_TYPE', badTypeValue);
83+
}
84+
}
Collapse file

‎test/parallel/test-fs-readSync-position-validation.mjs‎

Copy file name to clipboardExpand all lines: test/parallel/test-fs-readSync-position-validation.mjs
+2-3Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ function testValid(position, allowedErrors = []) {
2828
}
2929
}
3030

31-
function testInvalid(code, position, internalCatch = false) {
31+
function testInvalid(code, position) {
3232
let fdSync;
3333
try {
3434
fdSync = fs.openSync(filepath, 'r');
@@ -61,8 +61,7 @@ function testInvalid(code, position, internalCatch = false) {
6161

6262
testValid(2n ** 63n - 1n - BigInt(length), [ 'EFBIG', 'EOVERFLOW' ]);
6363
testInvalid('ERR_OUT_OF_RANGE', 2n ** 63n);
64-
65-
// TODO(LiviaMedeiros): test `2n ** 63n - BigInt(length)`
64+
testInvalid('ERR_OUT_OF_RANGE', 2n ** 63n - BigInt(length));
6665

6766
testInvalid('ERR_OUT_OF_RANGE', NaN);
6867
testInvalid('ERR_OUT_OF_RANGE', -Infinity);

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.