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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Author: LiviaMedeiros

doc/api/fs.md

@@ -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`
@@ -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}

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,

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;

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 {

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);

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);
+  }
+}

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);