From 0b4ee396f83a0d738a7e3589647eb4b58c1a8eb4 Mon Sep 17 00:00:00 2001
From: LiviaMedeiros <livia@cirno.name>
Date: Sat, 23 Apr 2022 11:49:36 +0800
Subject: [PATCH 1/8] 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`
---
 doc/api/fs.md               | 30 ++++++++++++++++--------------
 lib/fs.js                   | 14 ++++++++------
 lib/internal/fs/promises.js |  7 +++++--
 lib/internal/fs/utils.js    |  6 +++---
 4 files changed, 32 insertions(+), 25 deletions(-)

diff --git a/doc/api/fs.md b/doc/api/fs.md
index 5cf89e0eb0f1d8..6bf74fc46120d6 100644
--- a/doc/api/fs.md
+++ b/doc/api/fs.md
@@ -375,10 +375,10 @@ added: v10.0.0
   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`
@@ -404,10 +404,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`
@@ -433,10 +434,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`
@@ -3508,8 +3510,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 b17cf4f10cd3c1..65c6ecda90ec6e 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -693,10 +693,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');
+  }
 
   function wrapper(err, bytesRead) {
     // Retain a reference to buffer so that it can't be GC'ed too soon.
@@ -761,10 +762,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');
+  }
 
   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..1ab16dc2017238 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');
+  }
 
   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..b277cbfd9e6998 100644
--- a/lib/internal/fs/utils.js
+++ b/lib/internal/fs/utils.js
@@ -918,11 +918,11 @@ const validateStringAfterArrayBufferView = hideStackFrames((buffer, name) => {
 
 const validatePosition = hideStackFrames((position, name) => {
   if (typeof position === 'number') {
-    validateInteger(position, name);
+    validateInteger(position, name, -1);
   } else if (typeof position === 'bigint') {
-    if (!(position >= -(2n ** 63n) && position <= 2n ** 63n - 1n)) {
+    if (!(position >= -1n && position <= 2n ** 63n - 1n)) {
       throw new ERR_OUT_OF_RANGE(name,
-                                 `>= ${-(2n ** 63n)} && <= ${2n ** 63n - 1n}`,
+                                 `>= -1 && <= ${2n ** 63n - 1n}`,
                                  position);
     }
   } else {

From 0c2bea29e69a2682de3cff4c4e0373b9e2d98918 Mon Sep 17 00:00:00 2001
From: LiviaMedeiros <livia@cirno.name>
Date: Sat, 23 Apr 2022 14:40:08 +0800
Subject: [PATCH 2/8] squash: fix validation when `position + length` exceeds
 int64 limit

---
 lib/fs.js                   | 4 ++--
 lib/internal/fs/promises.js | 2 +-
 lib/internal/fs/utils.js    | 7 ++++---
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/lib/fs.js b/lib/fs.js
index 65c6ecda90ec6e..b51cade08d605b 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -696,7 +696,7 @@ function read(fd, buffer, offsetOrOptions, length, position, callback) {
   if (position == null) {
     position = -1;
   } else {
-    validatePosition(position, 'position');
+    validatePosition(position, 'position', length);
   }
 
   function wrapper(err, bytesRead) {
@@ -765,7 +765,7 @@ function readSync(fd, buffer, offsetOrOptions, length, position) {
   if (position == null) {
     position = -1;
   } else {
-    validatePosition(position, 'position');
+    validatePosition(position, 'position', length);
   }
 
   const ctx = {};
diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
index 1ab16dc2017238..a656ca411584b2 100644
--- a/lib/internal/fs/promises.js
+++ b/lib/internal/fs/promises.js
@@ -639,7 +639,7 @@ async function read(handle, bufferOrParams, offset, length, position) {
   if (position == null) {
     position = -1;
   } else {
-    validatePosition(position, 'position');
+    validatePosition(position, 'position', length);
   }
 
   const bytesRead = (await binding.read(handle.fd, buffer, offset, length,
diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js
index b277cbfd9e6998..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, -1);
   } else if (typeof position === 'bigint') {
-    if (!(position >= -1n && position <= 2n ** 63n - 1n)) {
+    const maxPosition = 2n ** 63n - 1n - BigInt(length);
+    if (!(position >= -1n && position <= maxPosition)) {
       throw new ERR_OUT_OF_RANGE(name,
-                                 `>= -1 && <= ${2n ** 63n - 1n}`,
+                                 `>= -1 && <= ${maxPosition}`,
                                  position);
     }
   } else {

From c911c77e66b87dc5b4e37c57bfc58a2e374f77cb Mon Sep 17 00:00:00 2001
From: LiviaMedeiros <livia@cirno.name>
Date: Sat, 23 Apr 2022 14:41:33 +0800
Subject: [PATCH 3/8] squash: add tests


From 022d17c4584d4940a52c9fd71b774cdae0453f11 Mon Sep 17 00:00:00 2001
From: LiviaMedeiros <livia@cirno.name>
Date: Mon, 16 May 2022 18:45:49 +0800
Subject: [PATCH 4/8] squash: resolve todo

---
 test/parallel/test-fs-read-position-validation.mjs     | 3 +--
 test/parallel/test-fs-readSync-position-validation.mjs | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

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-readSync-position-validation.mjs b/test/parallel/test-fs-readSync-position-validation.mjs
index 93fe4be1f0b65d..3444b40f760452 100644
--- a/test/parallel/test-fs-readSync-position-validation.mjs
+++ b/test/parallel/test-fs-readSync-position-validation.mjs
@@ -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);

From 39007fbdef97e821773db6f7f4b9808213e210cf Mon Sep 17 00:00:00 2001
From: LiviaMedeiros <livia@cirno.name>
Date: Mon, 16 May 2022 19:23:36 +0800
Subject: [PATCH 5/8] squash: add test for Promises API

---
 ...t-fs-read-promises-position-validation.mjs | 84 +++++++++++++++++++
 .../test-fs-readSync-position-validation.mjs  |  2 +-
 2 files changed, 85 insertions(+), 1 deletion(-)
 create mode 100644 test/parallel/test-fs-read-promises-position-validation.mjs

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 3444b40f760452..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');

From 204fb4ff1acaca14461ea8efe3ff015d80614270 Mon Sep 17 00:00:00 2001
From: LiviaMedeiros <livia@cirno.name>
Date: Tue, 11 Oct 2022 16:39:47 +0800
Subject: [PATCH 6/8] squash: add `changes` to docs

---
 doc/api/fs.md | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/doc/api/fs.md b/doc/api/fs.md
index 6bf74fc46120d6..4f71a42e756dac 100644
--- a/doc/api/fs.md
+++ b/doc/api/fs.md
@@ -369,6 +369,10 @@ added: v10.0.0
 
 <!-- YAML
 added: v10.0.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/42835
+    description: `position` accepts bigint values.
 -->
 
 * `buffer` {Buffer|TypedArray|DataView} A buffer that will be filled with the
@@ -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: `position` accepts bigint values.
 -->
 
 * `options` {Object}
@@ -425,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: `position` accepts bigint values.
 -->
 
 * `buffer` {Buffer|TypedArray|DataView} A buffer that will be filled with the

From 00f91899690d1a8dce4898419ffdf2ee3ee7514c Mon Sep 17 00:00:00 2001
From: LiviaMedeiros <livia@cirno.name>
Date: Tue, 11 Oct 2022 16:43:27 +0800
Subject: [PATCH 7/8] squash: make linter happy

---
 doc/api/fs.md | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/doc/api/fs.md b/doc/api/fs.md
index 4f71a42e756dac..f05a444438301a 100644
--- a/doc/api/fs.md
+++ b/doc/api/fs.md
@@ -372,7 +372,7 @@ added: v10.0.0
 changes:
   - version: REPLACEME
     pr-url: https://github.com/nodejs/node/pull/42835
-    description: `position` accepts bigint values.
+    description: Accepts bigint values as `position`.
 -->
 
 * `buffer` {Buffer|TypedArray|DataView} A buffer that will be filled with the
@@ -402,7 +402,7 @@ added:
 changes:
   - version: REPLACEME
     pr-url: https://github.com/nodejs/node/pull/42835
-    description: `position` accepts bigint values.
+    description: Accepts bigint values as `position`.
 -->
 
 * `options` {Object}
@@ -436,7 +436,7 @@ added:
 changes:
   - version: REPLACEME
     pr-url: https://github.com/nodejs/node/pull/42835
-    description: `position` accepts bigint values.
+    description: Accepts bigint values as `position`.
 -->
 
 * `buffer` {Buffer|TypedArray|DataView} A buffer that will be filled with the

From dc7a21e459c665c46167c38e4588521cf484c970 Mon Sep 17 00:00:00 2001
From: LiviaMedeiros <livia@cirno.name>
Date: Tue, 11 Oct 2022 16:46:47 +0800
Subject: [PATCH 8/8] squash: fix typo

---
 doc/api/fs.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/api/fs.md b/doc/api/fs.md
index f05a444438301a..10e988d390e95f 100644
--- a/doc/api/fs.md
+++ b/doc/api/fs.md
@@ -415,7 +415,7 @@ changes:
   * `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.
+    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