Skip to content

Commit

Permalink
fs: adjust position validation in reading methods
Browse files Browse the repository at this point in the history
This prohibits invalid values (< -1 and non-integers) and
allows `filehandle.read()` to handle position up to `2n ** 63n - 1n`

PR-URL: nodejs#42835
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
LiviaMedeiros committed Sep 29, 2023
1 parent 7f0e36a commit b3ec13d
Show file tree
Hide file tree
Showing 7 changed files with 133 additions and 31 deletions.
42 changes: 28 additions & 14 deletions doc/api/fs.md
Original file line number Diff line number Diff line change
Expand Up @@ -369,16 +369,20 @@ added: v10.0.0
<!-- YAML
added: v10.0.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/42835
description: Accepts bigint values as `position`.
-->
* `buffer` {Buffer|TypedArray|DataView} A buffer that will be filled with the
file data read.
* `offset` {integer} The location in the buffer at which to start filling.
* `length` {integer} The number of bytes to read.
* `position` {integer|null} The location where to begin reading data from the
file. If `null`, data will be read from the current file position, and
the position will be updated. If `position` is an integer, the current
file position will remain unchanged.
* `position` {integer|bigint|null} The location where to begin reading data
from the file. If `null` or `-1`, data will be read from the current file
position, and the position will be updated. If `position` is a non-negative
integer, the current file position will remain unchanged.
* Returns: {Promise} Fulfills upon success with an object with two properties:
* `bytesRead` {integer} The number of bytes read
* `buffer` {Buffer|TypedArray|DataView} A reference to the passed in `buffer`
Expand All @@ -395,6 +399,10 @@ number of bytes read is zero.
added:
- v13.11.0
- v12.17.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/42835
description: Accepts bigint values as `position`.
-->
* `options` {Object}
Expand All @@ -404,10 +412,11 @@ added:
**Default:** `0`
* `length` {integer} The number of bytes to read. **Default:**
`buffer.byteLength - offset`
* `position` {integer|null} The location where to begin reading data from the
file. If `null`, data will be read from the current file position, and
the position will be updated. If `position` is an integer, the current
file position will remain unchanged. **Default:**: `null`
* `position` {integer|bigint|null} The location where to begin reading data
from the file. If `null` or `-1`, data will be read from the current file
position, and the position will be updated. If `position` is a non-negative
integer, the current file position will remain unchanged.
**Default:**: `null`
* Returns: {Promise} Fulfills upon success with an object with two properties:
* `bytesRead` {integer} The number of bytes read
* `buffer` {Buffer|TypedArray|DataView} A reference to the passed in `buffer`
Expand All @@ -424,6 +433,10 @@ number of bytes read is zero.
added:
- v18.2.0
- v16.17.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/42835
description: Accepts bigint values as `position`.
-->
* `buffer` {Buffer|TypedArray|DataView} A buffer that will be filled with the
Expand All @@ -433,10 +446,11 @@ added:
**Default:** `0`
* `length` {integer} The number of bytes to read. **Default:**
`buffer.byteLength - offset`
* `position` {integer} The location where to begin reading data from the
file. If `null`, data will be read from the current file position, and
the position will be updated. If `position` is an integer, the current
file position will remain unchanged. **Default:**: `null`
* `position` {integer|bigint|null} The location where to begin reading data
from the file. If `null` or `-1`, data will be read from the current file
position, and the position will be updated. If `position` is a non-negative
integer, the current file position will remain unchanged.
**Default:**: `null`
* Returns: {Promise} Fulfills upon success with an object with two properties:
* `bytesRead` {integer} The number of bytes read
* `buffer` {Buffer|TypedArray|DataView} A reference to the passed in `buffer`
Expand Down Expand Up @@ -3514,8 +3528,8 @@ changes:
* `length` {integer} The number of bytes to read.
* `position` {integer|bigint|null} Specifies where to begin reading from in the
file. If `position` is `null` or `-1 `, data will be read from the current
file position, and the file position will be updated. If `position` is an
integer, the file position will be unchanged.
file position, and the file position will be updated. If `position` is
a non-negative integer, the file position will be unchanged.
* `callback` {Function}
* `err` {Error}
* `bytesRead` {integer}
Expand Down
14 changes: 8 additions & 6 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -656,10 +656,11 @@ function read(fd, buffer, offsetOrOptions, length, position, callback) {

validateOffsetLengthRead(offset, length, buffer.byteLength);

if (position == null)
if (position == null) {
position = -1;

validatePosition(position, 'position');
} else {
validatePosition(position, 'position', length);
}

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

validateOffsetLengthRead(offset, length, buffer.byteLength);

if (position == null)
if (position == null) {
position = -1;

validatePosition(position, 'position');
} else {
validatePosition(position, 'position', length);
}

const ctx = {};
const result = binding.read(fd, buffer, offset, length, position,
Expand Down
7 changes: 5 additions & 2 deletions lib/internal/fs/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ const {
Error,
MathMax,
MathMin,
NumberIsSafeInteger,
Promise,
PromisePrototypeThen,
PromiseResolve,
Expand Down Expand Up @@ -67,6 +66,7 @@ const {
validateCpOptions,
validateOffsetLengthRead,
validateOffsetLengthWrite,
validatePosition,
validateRmOptions,
validateRmdirOptions,
validateStringAfterArrayBufferView,
Expand Down Expand Up @@ -636,8 +636,11 @@ async function read(handle, bufferOrParams, offset, length, position) {

validateOffsetLengthRead(offset, length, buffer.byteLength);

if (!NumberIsSafeInteger(position))
if (position == null) {
position = -1;
} else {
validatePosition(position, 'position', length);
}

const bytesRead = (await binding.read(handle.fd, buffer, offset, length,
position, kUsePromises)) || 0;
Expand Down
9 changes: 5 additions & 4 deletions lib/internal/fs/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -916,13 +916,14 @@ const validateStringAfterArrayBufferView = hideStackFrames((buffer, name) => {
}
});

const validatePosition = hideStackFrames((position, name) => {
const validatePosition = hideStackFrames((position, name, length) => {
if (typeof position === 'number') {
validateInteger(position, name);
validateInteger(position, name, -1);
} else if (typeof position === 'bigint') {
if (!(position >= -(2n ** 63n) && position <= 2n ** 63n - 1n)) {
const maxPosition = 2n ** 63n - 1n - BigInt(length);
if (!(position >= -1n && position <= maxPosition)) {
throw new ERR_OUT_OF_RANGE(name,
`>= ${-(2n ** 63n)} && <= ${2n ** 63n - 1n}`,
`>= -1 && <= ${maxPosition}`,
position);
}
} else {
Expand Down
3 changes: 1 addition & 2 deletions test/parallel/test-fs-read-position-validation.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,7 @@ async function testInvalid(code, position) {

await testValid(2n ** 63n - 1n - BigInt(length), [ 'EFBIG', 'EOVERFLOW' ]);
await testInvalid('ERR_OUT_OF_RANGE', 2n ** 63n);

// TODO(LiviaMedeiros): test `2n ** 63n - BigInt(length)`
await testInvalid('ERR_OUT_OF_RANGE', 2n ** 63n - BigInt(length));

await testInvalid('ERR_OUT_OF_RANGE', NaN);
await testInvalid('ERR_OUT_OF_RANGE', -Infinity);
Expand Down
84 changes: 84 additions & 0 deletions test/parallel/test-fs-read-promises-position-validation.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
import '../common/index.mjs';
import * as fixtures from '../common/fixtures.mjs';
import fs from 'fs';
import assert from 'assert';

// This test ensures that "position" argument is correctly validated

const filepath = fixtures.path('x.txt');

const buffer = Buffer.from('xyz\n');
const offset = 0;
const length = buffer.byteLength;

// allowedErrors is an array of acceptable internal errors
// For example, on some platforms read syscall might return -EFBIG
async function testValid(position, allowedErrors = []) {
let fh;
try {
fh = await fs.promises.open(filepath, 'r');
await fh.read(buffer, offset, length, position);
await fh.read({ buffer, offset, length, position });
await fh.read(buffer, { offset, length, position });
} catch (err) {
if (!allowedErrors.includes(err.code)) {
assert.fail(err);
}
} finally {
await fh?.close();
}
}

async function testInvalid(code, position) {
let fh;
try {
fh = await fs.promises.open(filepath, 'r');
await assert.rejects(
fh.read(buffer, offset, length, position),
{ code }
);
await assert.rejects(
fh.read({ buffer, offset, length, position }),
{ code }
);
await assert.rejects(
fh.read(buffer, { offset, length, position }),
{ code }
);
} finally {
await fh?.close();
}
}

{
await testValid(undefined);
await testValid(null);
await testValid(-1);
await testValid(-1n);

await testValid(0);
await testValid(0n);
await testValid(1);
await testValid(1n);
await testValid(9);
await testValid(9n);
await testValid(Number.MAX_SAFE_INTEGER, [ 'EFBIG' ]);

await testValid(2n ** 63n - 1n - BigInt(length), [ 'EFBIG' ]);
await testInvalid('ERR_OUT_OF_RANGE', 2n ** 63n);
await testInvalid('ERR_OUT_OF_RANGE', 2n ** 63n - BigInt(length));

await testInvalid('ERR_OUT_OF_RANGE', NaN);
await testInvalid('ERR_OUT_OF_RANGE', -Infinity);
await testInvalid('ERR_OUT_OF_RANGE', Infinity);
await testInvalid('ERR_OUT_OF_RANGE', -0.999);
await testInvalid('ERR_OUT_OF_RANGE', -(2n ** 64n));
await testInvalid('ERR_OUT_OF_RANGE', Number.MAX_SAFE_INTEGER + 1);
await testInvalid('ERR_OUT_OF_RANGE', Number.MAX_VALUE);

for (const badTypeValue of [
false, true, '1', Symbol(1), {}, [], () => {}, Promise.resolve(1),
]) {
testInvalid('ERR_INVALID_ARG_TYPE', badTypeValue);
}
}
5 changes: 2 additions & 3 deletions test/parallel/test-fs-readSync-position-validation.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ function testValid(position, allowedErrors = []) {
}
}

function testInvalid(code, position, internalCatch = false) {
function testInvalid(code, position) {
let fdSync;
try {
fdSync = fs.openSync(filepath, 'r');
Expand Down Expand Up @@ -61,8 +61,7 @@ function testInvalid(code, position, internalCatch = false) {

testValid(2n ** 63n - 1n - BigInt(length), [ 'EFBIG', 'EOVERFLOW' ]);
testInvalid('ERR_OUT_OF_RANGE', 2n ** 63n);

// TODO(LiviaMedeiros): test `2n ** 63n - BigInt(length)`
testInvalid('ERR_OUT_OF_RANGE', 2n ** 63n - BigInt(length));

testInvalid('ERR_OUT_OF_RANGE', NaN);
testInvalid('ERR_OUT_OF_RANGE', -Infinity);
Expand Down

0 comments on commit b3ec13d

Please sign in to comment.