fs: adjust position validation in reading methods

LiviaMedeiros · web-flow · commit b3ec13d44973 · 2023-09-29T10:56:07.000Z
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>
diff --git a/doc/api/fs.md b/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}
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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);
