From b3ec13d4497398a8a85738a64849570462660a4f Mon Sep 17 00:00:00 2001 From: Livia Medeiros Date: Fri, 29 Sep 2023 19:56:07 +0900 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/42835 Reviewed-By: Antoine du Hamel Reviewed-By: James M Snell --- doc/api/fs.md | 42 ++++++---- lib/fs.js | 14 ++-- lib/internal/fs/promises.js | 7 +- lib/internal/fs/utils.js | 9 +- .../test-fs-read-position-validation.mjs | 3 +- ...t-fs-read-promises-position-validation.mjs | 84 +++++++++++++++++++ .../test-fs-readSync-position-validation.mjs | 5 +- 7 files changed, 133 insertions(+), 31 deletions(-) create mode 100644 test/parallel/test-fs-read-promises-position-validation.mjs diff --git a/doc/api/fs.md b/doc/api/fs.md index 66613dde55c19c..34d10418b2acc8 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -369,16 +369,20 @@ added: v10.0.0 * `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` @@ -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} @@ -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` @@ -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 @@ -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` @@ -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} diff --git a/lib/fs.js b/lib/fs.js index 29f356a57cd22e..3931216e9522f1 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -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. @@ -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, diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index bc506b9be82fbc..a656ca411584b2 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -6,7 +6,6 @@ const { Error, MathMax, MathMin, - NumberIsSafeInteger, Promise, PromisePrototypeThen, PromiseResolve, @@ -67,6 +66,7 @@ const { validateCpOptions, validateOffsetLengthRead, validateOffsetLengthWrite, + validatePosition, validateRmOptions, validateRmdirOptions, validateStringAfterArrayBufferView, @@ -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; diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index 2fc7bf61e9c488..f84133296e86fb 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -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 { diff --git a/test/parallel/test-fs-read-position-validation.mjs b/test/parallel/test-fs-read-position-validation.mjs index 5b90a3e2c0e845..504f02c3374cd6 100644 --- a/test/parallel/test-fs-read-position-validation.mjs +++ b/test/parallel/test-fs-read-position-validation.mjs @@ -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); diff --git a/test/parallel/test-fs-read-promises-position-validation.mjs b/test/parallel/test-fs-read-promises-position-validation.mjs new file mode 100644 index 00000000000000..1531a002b01fb0 --- /dev/null +++ b/test/parallel/test-fs-read-promises-position-validation.mjs @@ -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); + } +} diff --git a/test/parallel/test-fs-readSync-position-validation.mjs b/test/parallel/test-fs-readSync-position-validation.mjs index 93fe4be1f0b65d..305e37778d9aac 100644 --- a/test/parallel/test-fs-readSync-position-validation.mjs +++ b/test/parallel/test-fs-readSync-position-validation.mjs @@ -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'); @@ -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);