From c69abaaa5c3a7bde6804b2dba6f7aacd58bf0cf3 Mon Sep 17 00:00:00 2001
From: LiviaMedeiros <livia@cirno.name>
Date: Fri, 15 Apr 2022 22:01:15 +0800
Subject: [PATCH 01/16] fs: add read(buffer, options) signatures

This adds the following:
`fs.read(fd, buffer[, options], callback)`
`filehandle.read(buffer[, options])`
---
 lib/fs.js                                     | 35 +++++++++++--------
 lib/internal/fs/promises.js                   | 20 ++++++++---
 .../test-fs-readSync-optional-params.js       | 12 +------
 3 files changed, 37 insertions(+), 30 deletions(-)

diff --git a/lib/fs.js b/lib/fs.js
index 405c206d56685c..a2de246b094433 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -605,21 +605,30 @@ function openSync(path, flags, mode) {
  *   ) => any} callback
  * @returns {void}
  */
-function read(fd, buffer, offset, length, position, callback) {
+function read(fd, buffer, offsetOrOptions, length, position, callback) {
   fd = getValidatedFd(fd);
 
-  if (arguments.length <= 3) {
-    // Assume fs.read(fd, options, callback)
-    let options = ObjectCreate(null);
+  let offset = offsetOrOptions;
+  if (arguments.length === 4 && typeof offsetOrOptions === 'object') {
+    // This is fs.read(fd, buffer, options, callback)
+    callback = length;
+    ({
+      offset = 0,
+      length = buffer.byteLength - offset,
+      position = null
+    } = offsetOrOptions ?? ObjectCreate(null));
+  } else if (arguments.length <= 3) {
+    // Assume fs.read(fd, params, callback)
+    let params = ObjectCreate(null);
     if (arguments.length < 3) {
       // This is fs.read(fd, callback)
       // buffer will be the callback
       callback = buffer;
     } else {
       // This is fs.read(fd, {}, callback)
-      // buffer will be the options object
+      // buffer will be the params object
       // offset is the callback
-      options = buffer;
+      params = buffer;
       callback = offset;
     }
 
@@ -628,7 +637,7 @@ function read(fd, buffer, offset, length, position, callback) {
       offset = 0,
       length = buffer.byteLength - offset,
       position = null
-    } = options);
+    } = params);
   }
 
   validateBuffer(buffer);
@@ -686,23 +695,21 @@ ObjectDefineProperty(read, internalUtil.customPromisifyArgs,
  *   }} [offset]
  * @returns {number}
  */
-function readSync(fd, buffer, offset, length, position) {
+function readSync(fd, buffer, offsetOrOptions, length, position) {
   fd = getValidatedFd(fd);
 
   validateBuffer(buffer);
 
-  if (arguments.length <= 3) {
-    // Assume fs.readSync(fd, buffer, options)
-    const options = offset || ObjectCreate(null);
-
+  let offset = offsetOrOptions;
+  if (typeof offset === 'object') {
     ({
       offset = 0,
       length = buffer.byteLength - offset,
       position = null
-    } = options);
+    } = offsetOrOptions ?? ObjectCreate(null));
   }
 
-  if (offset == null) {
+  if (offset === false || offset === undefined) {
     offset = 0;
   } else {
     validateInteger(offset, 'offset', 0);
diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
index 73c159550740b0..20651c75d2c331 100644
--- a/lib/internal/fs/promises.js
+++ b/lib/internal/fs/promises.js
@@ -508,21 +508,31 @@ async function open(path, flags, mode) {
                                  flagsNumber, mode, kUsePromises));
 }
 
-async function read(handle, bufferOrOptions, offset, length, position) {
-  let buffer = bufferOrOptions;
+async function read(handle, bufferOrParams, offset, length, position) {
+  let buffer = bufferOrParams;
   if (!isArrayBufferView(buffer)) {
-    bufferOrOptions ??= ObjectCreate(null);
+    // This is fh.read(params)
+    bufferOrParams ??= ObjectCreate(null);
     ({
       buffer = Buffer.alloc(16384),
       offset = 0,
       length = buffer.byteLength - offset,
       position = null
-    } = bufferOrOptions);
+    } = bufferOrParams);
 
     validateBuffer(buffer);
   }
 
-  if (offset == null) {
+  if (typeof offset === 'object') {
+    // This is fh.read(buffer, options)
+    ({
+      offset = 0,
+      length = buffer.byteLength - offset,
+      position = null
+    } = offset ?? ObjectCreate(null));
+  }
+
+  if (offset === false || offset === undefined) {
     offset = 0;
   } else {
     validateInteger(offset, 'offset', 0);
diff --git a/test/parallel/test-fs-readSync-optional-params.js b/test/parallel/test-fs-readSync-optional-params.js
index 00f1a5531cf6ea..4399a8c35b54cc 100644
--- a/test/parallel/test-fs-readSync-optional-params.js
+++ b/test/parallel/test-fs-readSync-optional-params.js
@@ -31,7 +31,7 @@ for (const options of [
   { length: expected.length, position: 0 },
   { offset: 0, length: expected.length, position: 0 },
 
-  { offset: null },
+  { offset: false },
   { position: null },
   { position: -1 },
   { position: 0n },
@@ -39,18 +39,8 @@ for (const options of [
   // Test default params
   {},
   null,
-  undefined,
-
-  // Test if bad params are interpreted as default (not mandatory)
-  false,
-  true,
-  Infinity,
-  42n,
-  Symbol(),
 
   // Test even more malicious corner cases
-  '4'.repeat(expected.length),
-  new String('4444'),
   [4, 4, 4, 4],
 ]) {
   runTest(Buffer.allocUnsafe(expected.length), options);

From aec64cae9d78dd5a13f1fb71dbcc478fb1bdb296 Mon Sep 17 00:00:00 2001
From: LiviaMedeiros <livia@cirno.name>
Date: Sun, 17 Apr 2022 19:12:29 +0800
Subject: [PATCH 02/16] squash: make offsetOrOptions more consistent

---
 lib/fs.js                                 | 18 ++++-----
 test/parallel/test-fs-read-offset-null.js | 49 +++++++++++++++++------
 2 files changed, 46 insertions(+), 21 deletions(-)

diff --git a/lib/fs.js b/lib/fs.js
index a2de246b094433..7eb83e5ef36487 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -595,7 +595,7 @@ function openSync(path, flags, mode) {
  * Reads file from the specified `fd` (file descriptor).
  * @param {number} fd
  * @param {Buffer | TypedArray | DataView} buffer
- * @param {number} offset
+ * @param {number} offsetOrOptions
  * @param {number} length
  * @param {number | bigint} position
  * @param {(
@@ -609,7 +609,10 @@ function read(fd, buffer, offsetOrOptions, length, position, callback) {
   fd = getValidatedFd(fd);
 
   let offset = offsetOrOptions;
-  if (arguments.length === 4 && typeof offsetOrOptions === 'object') {
+  if (
+    arguments.length === 4 &&
+    (offsetOrOptions === undefined || typeof offsetOrOptions === 'object')
+  ) {
     // This is fs.read(fd, buffer, options, callback)
     callback = length;
     ({
@@ -618,16 +621,13 @@ function read(fd, buffer, offsetOrOptions, length, position, callback) {
       position = null
     } = offsetOrOptions ?? ObjectCreate(null));
   } else if (arguments.length <= 3) {
-    // Assume fs.read(fd, params, callback)
+    // Assume fs.read(fd[, params], callback)
     let params = ObjectCreate(null);
     if (arguments.length < 3) {
       // This is fs.read(fd, callback)
-      // buffer will be the callback
       callback = buffer;
     } else {
-      // This is fs.read(fd, {}, callback)
-      // buffer will be the params object
-      // offset is the callback
+      // This is fs.read(fd, params, callback)
       params = buffer;
       callback = offset;
     }
@@ -643,7 +643,7 @@ function read(fd, buffer, offsetOrOptions, length, position, callback) {
   validateBuffer(buffer);
   callback = maybeCallback(callback);
 
-  if (offset == null) {
+  if (offset === false || offset === undefined) {
     offset = 0;
   } else {
     validateInteger(offset, 'offset', 0);
@@ -692,7 +692,7 @@ ObjectDefineProperty(read, internalUtil.customPromisifyArgs,
  *   offset?: number;
  *   length?: number;
  *   position?: number | bigint;
- *   }} [offset]
+ *   }} [offsetOrOptions]
  * @returns {number}
  */
 function readSync(fd, buffer, offsetOrOptions, length, position) {
diff --git a/test/parallel/test-fs-read-offset-null.js b/test/parallel/test-fs-read-offset-null.js
index 27e893f781b2ff..22ca5f818f5931 100644
--- a/test/parallel/test-fs-read-offset-null.js
+++ b/test/parallel/test-fs-read-offset-null.js
@@ -15,28 +15,53 @@ const buf = Buffer.alloc(1);
 // Reading only one character, hence buffer of one byte is enough.
 
 // Test for callback API.
-fs.open(filepath, 'r', common.mustSucceed((fd) => {
-  fs.read(fd, { offset: null, buffer: buf },
-          common.mustSucceed((bytesRead, buffer) => {
-            // Test is done by making sure the first letter in buffer is
-            // same as first letter in file.
-            // 120 is the hex for ascii code of letter x.
-            assert.strictEqual(buffer[0], 120);
-            fs.close(fd, common.mustSucceed(() => {}));
-          }));
-}));
+fs.open(filepath, 'r', (_, fd) => {
+  assert.throws(
+    () => fs.read(fd, { offset: null, buffer: buf }, () => {}),
+    {
+      code: 'ERR_INVALID_ARG_TYPE',
+      name: 'TypeError',
+    }
+  );
+  assert.throws(
+    () => fs.read(fd, buf, { offset: null }, () => {}),
+    {
+      code: 'ERR_INVALID_ARG_TYPE',
+      name: 'TypeError',
+    }
+  );
+  fs.close(fd, common.mustSucceed(() => {}));
+});
 
 let filehandle = null;
 
 // Test for promise api
 (async () => {
   filehandle = await fsPromises.open(filepath, 'r');
-  const readObject = await filehandle.read(buf, null, buf.length);
+  assert.rejects(
+    async () => await filehandle.read(buf, { offset: null }),
+    {
+      code: 'ERR_INVALID_ARG_TYPE',
+      name: 'TypeError',
+    }
+  );
+})()
+.then(common.mustCall())
+.finally(async () => {
+  if (filehandle)
+    await filehandle.close();
+});
+
+(async () => {
+  filehandle = await fsPromises.open(filepath, 'r');
+
+  // In this test, null is interpreted as default options, not null offset.
+  // 120 is the ascii code of letter x.
+  const readObject = await filehandle.read(buf, null);
   assert.strictEqual(readObject.buffer[0], 120);
 })()
 .then(common.mustCall())
 .finally(async () => {
-// Close the file handle if it is opened
   if (filehandle)
     await filehandle.close();
 });

From 42c9c40bfe5144202d2a706481ff0646dfe11444 Mon Sep 17 00:00:00 2001
From: LiviaMedeiros <livia@cirno.name>
Date: Sun, 17 Apr 2022 20:08:37 +0800
Subject: [PATCH 03/16] squash: add doc entries

---
 doc/api/fs.md                             | 49 +++++++++++++++++++++++
 test/parallel/test-fs-read-offset-null.js |  2 +-
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/doc/api/fs.md b/doc/api/fs.md
index c32c624f48b4a7..c46a78f78b4ddb 100644
--- a/doc/api/fs.md
+++ b/doc/api/fs.md
@@ -417,6 +417,33 @@ Reads data from the file and stores that in the given buffer.
 If the file is not modified concurrently, the end-of-file is reached when the
 number of bytes read is zero.
 
+#### `filehandle.read(buffer[, options])`
+
+<!-- YAML
+added: REPLACEME
+-->
+
+* `buffer` {Buffer|TypedArray|DataView} A buffer that will be filled with the
+  file data read.
+* `options` {Object}
+  * `offset` {integer} The location in the buffer at which to start filling.
+    **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`
+* 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`
+    argument.
+
+Reads data from the file and stores that in the given buffer.
+
+If the file is not modified concurrently, the end-of-file is reached when the
+number of bytes read is zero.
+
 #### `filehandle.readableWebStream()`
 
 <!-- YAML
@@ -3243,6 +3270,28 @@ Similar to the [`fs.read()`][] function, this version takes an optional
 `options` object. If no `options` object is specified, it will default with the
 above values.
 
+### `fs.read(fd, buffer[, options], callback)`
+
+<!-- YAML
+added: REPLACEME
+-->
+
+* `fd` {integer}
+* `buffer` {Buffer|TypedArray|DataView} The buffer that the data will be
+  written to.
+* `options` {Object}
+  * `offset` {integer} **Default:** `0`
+  * `length` {integer} **Default:** `buffer.byteLength - offset`
+  * `position` {integer|bigint} **Default:** `null`
+* `callback` {Function}
+  * `err` {Error}
+  * `bytesRead` {integer}
+  * `buffer` {Buffer}
+
+Similar to the [`fs.read()`][] function, this version takes an optional
+`options` object. If no `options` object is specified, it will default with the
+above values.
+
 ### `fs.readdir(path[, options], callback)`
 
 <!-- YAML
diff --git a/test/parallel/test-fs-read-offset-null.js b/test/parallel/test-fs-read-offset-null.js
index 22ca5f818f5931..95e9c67ba4275c 100644
--- a/test/parallel/test-fs-read-offset-null.js
+++ b/test/parallel/test-fs-read-offset-null.js
@@ -39,7 +39,7 @@ let filehandle = null;
 (async () => {
   filehandle = await fsPromises.open(filepath, 'r');
   assert.rejects(
-    async () => await filehandle.read(buf, { offset: null }),
+    async () => filehandle.read(buf, { offset: null }),
     {
       code: 'ERR_INVALID_ARG_TYPE',
       name: 'TypeError',

From 71f03ce69ebe2b4bc9e98d449db120e8153af504 Mon Sep 17 00:00:00 2001
From: LiviaMedeiros <livia@cirno.name>
Date: Sun, 17 Apr 2022 21:35:15 +0800
Subject: [PATCH 04/16] squash: fix fs.read()

---
 lib/fs.js | 40 +++++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/lib/fs.js b/lib/fs.js
index 7eb83e5ef36487..f10f7e255ef1f4 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -136,6 +136,7 @@ const {
   validateEncoding,
   validateFunction,
   validateInteger,
+  validateObject,
   validateString,
 } = require('internal/validators');
 
@@ -609,35 +610,32 @@ function read(fd, buffer, offsetOrOptions, length, position, callback) {
   fd = getValidatedFd(fd);
 
   let offset = offsetOrOptions;
-  if (
-    arguments.length === 4 &&
-    (offsetOrOptions === undefined || typeof offsetOrOptions === 'object')
-  ) {
-    // This is fs.read(fd, buffer, options, callback)
-    callback = length;
-    ({
-      offset = 0,
-      length = buffer.byteLength - offset,
-      position = null
-    } = offsetOrOptions ?? ObjectCreate(null));
-  } else if (arguments.length <= 3) {
-    // Assume fs.read(fd[, params], callback)
-    let params = ObjectCreate(null);
-    if (arguments.length < 3) {
+  let params = null;
+  if (arguments.length <= 4) {
+    if (arguments.length === 4) {
+      // This is fs.read(fd, buffer, options, callback)
+      validateObject(offsetOrOptions, 'options', { nullable: true });
+      callback = length;
+      params = offsetOrOptions;
+    } else if (arguments.length === 3) {
+      // This is fs.read(fd, bufferOrParams, callback)
+      if (!isArrayBufferView(buffer)) {
+        // This is fs.read(fd, params, callback)
+        params = buffer;
+        ({ buffer = Buffer.alloc(16384) } = params ?? ObjectCreate(null));
+      }
+      callback = offsetOrOptions;
+    } else {
       // This is fs.read(fd, callback)
       callback = buffer;
-    } else {
-      // This is fs.read(fd, params, callback)
-      params = buffer;
-      callback = offset;
+      buffer = Buffer.alloc(16384);
     }
 
     ({
-      buffer = Buffer.alloc(16384),
       offset = 0,
       length = buffer.byteLength - offset,
       position = null
-    } = params);
+    } = params ?? ObjectCreate(null));
   }
 
   validateBuffer(buffer);

From 0bd9de772ac40f25d8c80e7c3c0acc5e5771ff94 Mon Sep 17 00:00:00 2001
From: LiviaMedeiros <livia@cirno.name>
Date: Mon, 18 Apr 2022 00:11:04 +0800
Subject: [PATCH 05/16] squash: expand fs-read-optional test

---
 test/parallel/test-fs-read-optional-params.js | 33 ++++++++++++++-----
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/test/parallel/test-fs-read-optional-params.js b/test/parallel/test-fs-read-optional-params.js
index aac5f51d0bb2db..c7a291a798bad7 100644
--- a/test/parallel/test-fs-read-optional-params.js
+++ b/test/parallel/test-fs-read-optional-params.js
@@ -9,28 +9,45 @@ const fd = fs.openSync(filepath, 'r');
 
 const expected = Buffer.from('xyz\n');
 const defaultBufferAsync = Buffer.alloc(16384);
-const bufferAsOption = Buffer.allocUnsafe(expected.length);
+const bufferAsOption = Buffer.allocUnsafe(expected.byteLength);
 
 // Test not passing in any options object
 fs.read(fd, common.mustCall((err, bytesRead, buffer) => {
-  assert.strictEqual(bytesRead, expected.length);
-  assert.deepStrictEqual(defaultBufferAsync.length, buffer.length);
+  assert.strictEqual(bytesRead, expected.byteLength);
+  assert.deepStrictEqual(defaultBufferAsync.byteLength, buffer.byteLength);
+}));
+fs.read(fd, bufferAsOption, { position: 0 }, common.mustCall((err, bytesRead, buffer) => {
+  assert.strictEqual(bytesRead, expected.byteLength);
+  assert.deepStrictEqual(bufferAsOption.byteLength, buffer.byteLength);
 }));
 
 // Test passing in an empty options object
 fs.read(fd, { position: 0 }, common.mustCall((err, bytesRead, buffer) => {
-  assert.strictEqual(bytesRead, expected.length);
-  assert.deepStrictEqual(defaultBufferAsync.length, buffer.length);
+  assert.strictEqual(bytesRead, expected.byteLength);
+  assert.deepStrictEqual(defaultBufferAsync.byteLength, buffer.byteLength);
+}));
+fs.read(fd, bufferAsOption, { position: 0 }, common.mustCall((err, bytesRead, buffer) => {
+  assert.strictEqual(bytesRead, expected.byteLength);
+  assert.deepStrictEqual(bufferAsOption.byteLength, buffer.byteLength);
 }));
 
 // Test passing in options
 fs.read(fd, {
   buffer: bufferAsOption,
   offset: 0,
-  length: bufferAsOption.length,
+  length: bufferAsOption.byteLength,
+  position: 0
+},
+        common.mustCall((err, bytesRead, buffer) => {
+          assert.strictEqual(bytesRead, expected.byteLength);
+          assert.deepStrictEqual(bufferAsOption.byteLength, buffer.byteLength);
+        }));
+fs.read(fd, bufferAsOption, {
+  offset: 0,
+  length: bufferAsOption.byteLength,
   position: 0
 },
         common.mustCall((err, bytesRead, buffer) => {
-          assert.strictEqual(bytesRead, expected.length);
-          assert.deepStrictEqual(bufferAsOption.length, buffer.length);
+          assert.strictEqual(bytesRead, expected.byteLength);
+          assert.deepStrictEqual(bufferAsOption.byteLength, buffer.byteLength);
         }));

From abd6e4f947e516f5fde476acff643e7d36e4e5b5 Mon Sep 17 00:00:00 2001
From: LiviaMedeiros <livia@cirno.name>
Date: Mon, 18 Apr 2022 00:19:42 +0800
Subject: [PATCH 06/16] squash: expand fs-read-promises-optional test

---
 .../test-fs-read-promises-optional-params.js         | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/test/parallel/test-fs-read-promises-optional-params.js b/test/parallel/test-fs-read-promises-optional-params.js
index 9d19eaa41151a8..48311e9df45c91 100644
--- a/test/parallel/test-fs-read-promises-optional-params.js
+++ b/test/parallel/test-fs-read-promises-optional-params.js
@@ -10,10 +10,18 @@ const fd = fs.openSync(filepath, 'r');
 
 const expected = Buffer.from('xyz\n');
 const defaultBufferAsync = Buffer.alloc(16384);
+const bufferAsOption = Buffer.allocUnsafe(expected.byteLength);
 
 read(fd, {})
   .then(function({ bytesRead, buffer }) {
-    assert.strictEqual(bytesRead, expected.length);
-    assert.deepStrictEqual(defaultBufferAsync.length, buffer.length);
+    assert.strictEqual(bytesRead, expected.byteLength);
+    assert.deepStrictEqual(defaultBufferAsync.byteLength, buffer.byteLength);
+  })
+  .then(common.mustCall());
+
+read(fd, bufferAsOption, { position: 0 })
+  .then(function({ bytesRead, buffer }) {
+    assert.strictEqual(bytesRead, expected.byteLength);
+    assert.deepStrictEqual(bufferAsOption.byteLength, buffer.byteLength);
   })
   .then(common.mustCall());

From 982630982498f4fc14fc5fb07c2c884d491b98e4 Mon Sep 17 00:00:00 2001
From: LiviaMedeiros <livia@cirno.name>
Date: Mon, 18 Apr 2022 16:48:13 +0800
Subject: [PATCH 07/16] squash: improve tests

---
 test/parallel/test-fs-read-offset-null.js     | 10 +---
 test/parallel/test-fs-read-optional-params.js | 59 ++++++++-----------
 2 files changed, 25 insertions(+), 44 deletions(-)

diff --git a/test/parallel/test-fs-read-offset-null.js b/test/parallel/test-fs-read-offset-null.js
index 95e9c67ba4275c..6b4e8353d4e6e9 100644
--- a/test/parallel/test-fs-read-offset-null.js
+++ b/test/parallel/test-fs-read-offset-null.js
@@ -47,10 +47,7 @@ let filehandle = null;
   );
 })()
 .then(common.mustCall())
-.finally(async () => {
-  if (filehandle)
-    await filehandle.close();
-});
+.finally(() => filehandle?.close());
 
 (async () => {
   filehandle = await fsPromises.open(filepath, 'r');
@@ -61,7 +58,4 @@ let filehandle = null;
   assert.strictEqual(readObject.buffer[0], 120);
 })()
 .then(common.mustCall())
-.finally(async () => {
-  if (filehandle)
-    await filehandle.close();
-});
+.finally(() => filehandle?.close());
diff --git a/test/parallel/test-fs-read-optional-params.js b/test/parallel/test-fs-read-optional-params.js
index c7a291a798bad7..9f50c4aa3a861e 100644
--- a/test/parallel/test-fs-read-optional-params.js
+++ b/test/parallel/test-fs-read-optional-params.js
@@ -5,49 +5,36 @@ const fixtures = require('../common/fixtures');
 const fs = require('fs');
 const assert = require('assert');
 const filepath = fixtures.path('x.txt');
-const fd = fs.openSync(filepath, 'r');
 
 const expected = Buffer.from('xyz\n');
 const defaultBufferAsync = Buffer.alloc(16384);
 const bufferAsOption = Buffer.allocUnsafe(expected.byteLength);
 
-// Test not passing in any options object
-fs.read(fd, common.mustCall((err, bytesRead, buffer) => {
-  assert.strictEqual(bytesRead, expected.byteLength);
-  assert.deepStrictEqual(defaultBufferAsync.byteLength, buffer.byteLength);
-}));
-fs.read(fd, bufferAsOption, { position: 0 }, common.mustCall((err, bytesRead, buffer) => {
-  assert.strictEqual(bytesRead, expected.byteLength);
-  assert.deepStrictEqual(bufferAsOption.byteLength, buffer.byteLength);
-}));
+function testValid(message, ...options) {
+  const paramsMsg = `${message} (as params)`;
+  const paramsFilehandle = fs.openSync(filepath, 'r');
+  fs.read(paramsFilehandle, ...options, common.mustCall((err, bytesRead, buffer) => {
+    if (err) throw err;
+    assert.strictEqual(bytesRead, expected.byteLength, paramsMsg);
+    assert.deepStrictEqual(defaultBufferAsync.byteLength, buffer.byteLength, paramsMsg);
+    fs.closeSync(paramsFilehandle);
+  }));
 
-// Test passing in an empty options object
-fs.read(fd, { position: 0 }, common.mustCall((err, bytesRead, buffer) => {
-  assert.strictEqual(bytesRead, expected.byteLength);
-  assert.deepStrictEqual(defaultBufferAsync.byteLength, buffer.byteLength);
-}));
-fs.read(fd, bufferAsOption, { position: 0 }, common.mustCall((err, bytesRead, buffer) => {
-  assert.strictEqual(bytesRead, expected.byteLength);
-  assert.deepStrictEqual(bufferAsOption.byteLength, buffer.byteLength);
-}));
+  const optionsMsg = `${message} (as options)`;
+  const optionsFilehandle = fs.openSync(filepath, 'r');
+  fs.read(optionsFilehandle, bufferAsOption, ...options, common.mustCall((err, bytesRead, buffer) => {
+    if (err) throw err;
+    assert.strictEqual(bytesRead, expected.byteLength, optionsMsg);
+    assert.deepStrictEqual(bufferAsOption.byteLength, buffer.byteLength, optionsMsg);
+    fs.closeSync(optionsFilehandle);
+  }));
+}
 
-// Test passing in options
-fs.read(fd, {
-  buffer: bufferAsOption,
+testValid('Not passing in any object');
+testValid('Passing in a null', null);
+testValid('Passing in an empty object', {});
+testValid('Passing in an object', {
   offset: 0,
   length: bufferAsOption.byteLength,
   position: 0
-},
-        common.mustCall((err, bytesRead, buffer) => {
-          assert.strictEqual(bytesRead, expected.byteLength);
-          assert.deepStrictEqual(bufferAsOption.byteLength, buffer.byteLength);
-        }));
-fs.read(fd, bufferAsOption, {
-  offset: 0,
-  length: bufferAsOption.byteLength,
-  position: 0
-},
-        common.mustCall((err, bytesRead, buffer) => {
-          assert.strictEqual(bytesRead, expected.byteLength);
-          assert.deepStrictEqual(bufferAsOption.byteLength, buffer.byteLength);
-        }));
+});

From a32c914aec81d222307b2d304e518e033b9b43de Mon Sep 17 00:00:00 2001
From: LiviaMedeiros <livia@cirno.name>
Date: Mon, 18 Apr 2022 16:53:14 +0800
Subject: [PATCH 08/16] squash: fixup

---
 test/parallel/test-fs-read-optional-params.js | 2 --
 1 file changed, 2 deletions(-)

diff --git a/test/parallel/test-fs-read-optional-params.js b/test/parallel/test-fs-read-optional-params.js
index 9f50c4aa3a861e..e99fce31109de7 100644
--- a/test/parallel/test-fs-read-optional-params.js
+++ b/test/parallel/test-fs-read-optional-params.js
@@ -14,7 +14,6 @@ function testValid(message, ...options) {
   const paramsMsg = `${message} (as params)`;
   const paramsFilehandle = fs.openSync(filepath, 'r');
   fs.read(paramsFilehandle, ...options, common.mustCall((err, bytesRead, buffer) => {
-    if (err) throw err;
     assert.strictEqual(bytesRead, expected.byteLength, paramsMsg);
     assert.deepStrictEqual(defaultBufferAsync.byteLength, buffer.byteLength, paramsMsg);
     fs.closeSync(paramsFilehandle);
@@ -23,7 +22,6 @@ function testValid(message, ...options) {
   const optionsMsg = `${message} (as options)`;
   const optionsFilehandle = fs.openSync(filepath, 'r');
   fs.read(optionsFilehandle, bufferAsOption, ...options, common.mustCall((err, bytesRead, buffer) => {
-    if (err) throw err;
     assert.strictEqual(bytesRead, expected.byteLength, optionsMsg);
     assert.deepStrictEqual(bufferAsOption.byteLength, buffer.byteLength, optionsMsg);
     fs.closeSync(optionsFilehandle);

From 6a974c2d21764f95c4b88d0bc26f303f52cfe5ee Mon Sep 17 00:00:00 2001
From: LiviaMedeiros <livia@cirno.name>
Date: Mon, 18 Apr 2022 16:59:44 +0800
Subject: [PATCH 09/16] squash: fixup

---
 test/parallel/test-fs-read-optional-params.js | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/parallel/test-fs-read-optional-params.js b/test/parallel/test-fs-read-optional-params.js
index e99fce31109de7..aa665c16a6fa54 100644
--- a/test/parallel/test-fs-read-optional-params.js
+++ b/test/parallel/test-fs-read-optional-params.js
@@ -13,7 +13,7 @@ const bufferAsOption = Buffer.allocUnsafe(expected.byteLength);
 function testValid(message, ...options) {
   const paramsMsg = `${message} (as params)`;
   const paramsFilehandle = fs.openSync(filepath, 'r');
-  fs.read(paramsFilehandle, ...options, common.mustCall((err, bytesRead, buffer) => {
+  fs.read(paramsFilehandle, ...options, common.mustSucceed((bytesRead, buffer) => {
     assert.strictEqual(bytesRead, expected.byteLength, paramsMsg);
     assert.deepStrictEqual(defaultBufferAsync.byteLength, buffer.byteLength, paramsMsg);
     fs.closeSync(paramsFilehandle);
@@ -21,7 +21,7 @@ function testValid(message, ...options) {
 
   const optionsMsg = `${message} (as options)`;
   const optionsFilehandle = fs.openSync(filepath, 'r');
-  fs.read(optionsFilehandle, bufferAsOption, ...options, common.mustCall((err, bytesRead, buffer) => {
+  fs.read(optionsFilehandle, bufferAsOption, ...options, common.mustSucceed((bytesRead, buffer) => {
     assert.strictEqual(bytesRead, expected.byteLength, optionsMsg);
     assert.deepStrictEqual(bufferAsOption.byteLength, buffer.byteLength, optionsMsg);
     fs.closeSync(optionsFilehandle);

From 0f586d2585aa10803f9ecb6268635c7f059e408a Mon Sep 17 00:00:00 2001
From: LiviaMedeiros <livia@cirno.name>
Date: Mon, 18 Apr 2022 17:05:45 +0800
Subject: [PATCH 10/16] squash: inline null coalescing

---
 lib/internal/fs/promises.js | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
index 20651c75d2c331..83c341ac070d0f 100644
--- a/lib/internal/fs/promises.js
+++ b/lib/internal/fs/promises.js
@@ -512,13 +512,12 @@ async function read(handle, bufferOrParams, offset, length, position) {
   let buffer = bufferOrParams;
   if (!isArrayBufferView(buffer)) {
     // This is fh.read(params)
-    bufferOrParams ??= ObjectCreate(null);
     ({
       buffer = Buffer.alloc(16384),
       offset = 0,
       length = buffer.byteLength - offset,
       position = null
-    } = bufferOrParams);
+    } = bufferOrParams ?? ObjectCreate(null));
 
     validateBuffer(buffer);
   }

From 6410decb29b332b11c3608cbd4edfe4cff61d19c Mon Sep 17 00:00:00 2001
From: LiviaMedeiros <livia@cirno.name>
Date: Tue, 19 Apr 2022 15:05:40 +0800
Subject: [PATCH 11/16] squash: revert changes in fs.readSync()

---
 lib/fs.js                                         | 14 ++++++++------
 test/parallel/test-fs-read-offset-null.js         | 15 +++------------
 test/parallel/test-fs-readSync-optional-params.js | 12 +++++++++++-
 3 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/lib/fs.js b/lib/fs.js
index f10f7e255ef1f4..e1c30e8ba2ce3c 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -690,24 +690,26 @@ ObjectDefineProperty(read, internalUtil.customPromisifyArgs,
  *   offset?: number;
  *   length?: number;
  *   position?: number | bigint;
- *   }} [offsetOrOptions]
+ *   }} [offset]
  * @returns {number}
  */
-function readSync(fd, buffer, offsetOrOptions, length, position) {
+function readSync(fd, buffer, offset, length, position) {
   fd = getValidatedFd(fd);
 
   validateBuffer(buffer);
 
-  let offset = offsetOrOptions;
-  if (typeof offset === 'object') {
+  if (arguments.length <= 3) {
+    // Assume fs.readSync(fd, buffer, options)
+    const options = offset || ObjectCreate(null);
+
     ({
       offset = 0,
       length = buffer.byteLength - offset,
       position = null
-    } = offsetOrOptions ?? ObjectCreate(null));
+    } = options);
   }
 
-  if (offset === false || offset === undefined) {
+  if (offset == null) {
     offset = 0;
   } else {
     validateInteger(offset, 'offset', 0);
diff --git a/test/parallel/test-fs-read-offset-null.js b/test/parallel/test-fs-read-offset-null.js
index 6b4e8353d4e6e9..51fb718612655d 100644
--- a/test/parallel/test-fs-read-offset-null.js
+++ b/test/parallel/test-fs-read-offset-null.js
@@ -18,17 +18,11 @@ const buf = Buffer.alloc(1);
 fs.open(filepath, 'r', (_, fd) => {
   assert.throws(
     () => fs.read(fd, { offset: null, buffer: buf }, () => {}),
-    {
-      code: 'ERR_INVALID_ARG_TYPE',
-      name: 'TypeError',
-    }
+    { code: 'ERR_INVALID_ARG_TYPE' }
   );
   assert.throws(
     () => fs.read(fd, buf, { offset: null }, () => {}),
-    {
-      code: 'ERR_INVALID_ARG_TYPE',
-      name: 'TypeError',
-    }
+    { code: 'ERR_INVALID_ARG_TYPE' }
   );
   fs.close(fd, common.mustSucceed(() => {}));
 });
@@ -40,10 +34,7 @@ let filehandle = null;
   filehandle = await fsPromises.open(filepath, 'r');
   assert.rejects(
     async () => filehandle.read(buf, { offset: null }),
-    {
-      code: 'ERR_INVALID_ARG_TYPE',
-      name: 'TypeError',
-    }
+    { code: 'ERR_INVALID_ARG_TYPE' }
   );
 })()
 .then(common.mustCall())
diff --git a/test/parallel/test-fs-readSync-optional-params.js b/test/parallel/test-fs-readSync-optional-params.js
index 4399a8c35b54cc..00f1a5531cf6ea 100644
--- a/test/parallel/test-fs-readSync-optional-params.js
+++ b/test/parallel/test-fs-readSync-optional-params.js
@@ -31,7 +31,7 @@ for (const options of [
   { length: expected.length, position: 0 },
   { offset: 0, length: expected.length, position: 0 },
 
-  { offset: false },
+  { offset: null },
   { position: null },
   { position: -1 },
   { position: 0n },
@@ -39,8 +39,18 @@ for (const options of [
   // Test default params
   {},
   null,
+  undefined,
+
+  // Test if bad params are interpreted as default (not mandatory)
+  false,
+  true,
+  Infinity,
+  42n,
+  Symbol(),
 
   // Test even more malicious corner cases
+  '4'.repeat(expected.length),
+  new String('4444'),
   [4, 4, 4, 4],
 ]) {
   runTest(Buffer.allocUnsafe(expected.length), options);

From ccaa4cb122bfbf17550ebe789af079eafaf74ec8 Mon Sep 17 00:00:00 2001
From: LiviaMedeiros <livia@cirno.name>
Date: Tue, 19 Apr 2022 17:39:06 +0800
Subject: [PATCH 12/16] squash: disallow offset === false

---
 lib/fs.js                   | 2 +-
 lib/internal/fs/promises.js | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/fs.js b/lib/fs.js
index e1c30e8ba2ce3c..d0351a0c514530 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -641,7 +641,7 @@ function read(fd, buffer, offsetOrOptions, length, position, callback) {
   validateBuffer(buffer);
   callback = maybeCallback(callback);
 
-  if (offset === false || offset === undefined) {
+  if (offset === undefined) {
     offset = 0;
   } else {
     validateInteger(offset, 'offset', 0);
diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
index 83c341ac070d0f..d89cf48366a5f3 100644
--- a/lib/internal/fs/promises.js
+++ b/lib/internal/fs/promises.js
@@ -531,7 +531,7 @@ async function read(handle, bufferOrParams, offset, length, position) {
     } = offset ?? ObjectCreate(null));
   }
 
-  if (offset === false || offset === undefined) {
+  if (offset === undefined) {
     offset = 0;
   } else {
     validateInteger(offset, 'offset', 0);

From 0e6b06af2c33c5898eb836d534bab50a1f4e374f Mon Sep 17 00:00:00 2001
From: LiviaMedeiros <livia@cirno.name>
Date: Thu, 21 Apr 2022 15:46:18 +0800
Subject: [PATCH 13/16] squash: fixup

---
 test/parallel/test-fs-read-offset-null.js | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/parallel/test-fs-read-offset-null.js b/test/parallel/test-fs-read-offset-null.js
index 51fb718612655d..cc2a36049cfa2b 100644
--- a/test/parallel/test-fs-read-offset-null.js
+++ b/test/parallel/test-fs-read-offset-null.js
@@ -15,7 +15,7 @@ const buf = Buffer.alloc(1);
 // Reading only one character, hence buffer of one byte is enough.
 
 // Test for callback API.
-fs.open(filepath, 'r', (_, fd) => {
+fs.open(filepath, 'r', common.mustSucceed((fd) => {
   assert.throws(
     () => fs.read(fd, { offset: null, buffer: buf }, () => {}),
     { code: 'ERR_INVALID_ARG_TYPE' }
@@ -25,7 +25,7 @@ fs.open(filepath, 'r', (_, fd) => {
     { code: 'ERR_INVALID_ARG_TYPE' }
   );
   fs.close(fd, common.mustSucceed(() => {}));
-});
+}));
 
 let filehandle = null;
 

From 42b640c6ff185c7fb565eb613c31460c4e9661aa Mon Sep 17 00:00:00 2001
From: LiviaMedeiros <livia@cirno.name>
Date: Thu, 21 Apr 2022 16:42:59 +0800
Subject: [PATCH 14/16] squash: allow offset === null

---
 lib/fs.js                                 |  2 +-
 lib/internal/fs/promises.js               |  4 +--
 test/parallel/test-fs-read-offset-null.js | 35 ++++++++++++-----------
 3 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/lib/fs.js b/lib/fs.js
index d0351a0c514530..cdd941617df9f7 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -641,7 +641,7 @@ function read(fd, buffer, offsetOrOptions, length, position, callback) {
   validateBuffer(buffer);
   callback = maybeCallback(callback);
 
-  if (offset === undefined) {
+  if (offset == null) {
     offset = 0;
   } else {
     validateInteger(offset, 'offset', 0);
diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
index d89cf48366a5f3..ea9482e69dc904 100644
--- a/lib/internal/fs/promises.js
+++ b/lib/internal/fs/promises.js
@@ -522,7 +522,7 @@ async function read(handle, bufferOrParams, offset, length, position) {
     validateBuffer(buffer);
   }
 
-  if (typeof offset === 'object') {
+  if (offset !== null && typeof offset === 'object') {
     // This is fh.read(buffer, options)
     ({
       offset = 0,
@@ -531,7 +531,7 @@ async function read(handle, bufferOrParams, offset, length, position) {
     } = offset ?? ObjectCreate(null));
   }
 
-  if (offset === undefined) {
+  if (offset == null) {
     offset = 0;
   } else {
     validateInteger(offset, 'offset', 0);
diff --git a/test/parallel/test-fs-read-offset-null.js b/test/parallel/test-fs-read-offset-null.js
index cc2a36049cfa2b..11c4a4af77c0a5 100644
--- a/test/parallel/test-fs-read-offset-null.js
+++ b/test/parallel/test-fs-read-offset-null.js
@@ -14,17 +14,25 @@ const filepath = fixtures.path('x.txt');
 const buf = Buffer.alloc(1);
 // Reading only one character, hence buffer of one byte is enough.
 
+// Test is done by making sure the first letter in buffer is
+// same as first letter in file.
+// 120 is the ascii code of letter x.
+
 // Test for callback API.
 fs.open(filepath, 'r', common.mustSucceed((fd) => {
-  assert.throws(
-    () => fs.read(fd, { offset: null, buffer: buf }, () => {}),
-    { code: 'ERR_INVALID_ARG_TYPE' }
-  );
-  assert.throws(
-    () => fs.read(fd, buf, { offset: null }, () => {}),
-    { code: 'ERR_INVALID_ARG_TYPE' }
-  );
-  fs.close(fd, common.mustSucceed(() => {}));
+  fs.read(fd, { offset: null, buffer: buf },
+          common.mustSucceed((bytesRead, buffer) => {
+            assert.strictEqual(buffer[0], 120);
+            fs.close(fd, common.mustSucceed(() => {}));
+          }));
+}));
+
+fs.open(filepath, 'r', common.mustSucceed((fd) => {
+  fs.read(fd, buf, { offset: null },
+          common.mustSucceed((bytesRead, buffer) => {
+            assert.strictEqual(buffer[0], 120);
+            fs.close(fd, common.mustSucceed(() => {}));
+          }));
 }));
 
 let filehandle = null;
@@ -32,19 +40,14 @@ let filehandle = null;
 // Test for promise api
 (async () => {
   filehandle = await fsPromises.open(filepath, 'r');
-  assert.rejects(
-    async () => filehandle.read(buf, { offset: null }),
-    { code: 'ERR_INVALID_ARG_TYPE' }
-  );
+  const readObject = await filehandle.read(buf, { offset: null }, buf.length);
+  assert.strictEqual(readObject.buffer[0], 120);
 })()
 .then(common.mustCall())
 .finally(() => filehandle?.close());
 
 (async () => {
   filehandle = await fsPromises.open(filepath, 'r');
-
-  // In this test, null is interpreted as default options, not null offset.
-  // 120 is the ascii code of letter x.
   const readObject = await filehandle.read(buf, null);
   assert.strictEqual(readObject.buffer[0], 120);
 })()

From e1531437150e7e973dbab0e79895201a294dcde9 Mon Sep 17 00:00:00 2001
From: LiviaMedeiros <livia@cirno.name>
Date: Thu, 21 Apr 2022 19:17:53 +0800
Subject: [PATCH 15/16] squash: fix tests being other way round

---
 test/parallel/test-fs-read-offset-null.js | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/parallel/test-fs-read-offset-null.js b/test/parallel/test-fs-read-offset-null.js
index 11c4a4af77c0a5..9f3d95ebdca4b4 100644
--- a/test/parallel/test-fs-read-offset-null.js
+++ b/test/parallel/test-fs-read-offset-null.js
@@ -40,7 +40,7 @@ let filehandle = null;
 // Test for promise api
 (async () => {
   filehandle = await fsPromises.open(filepath, 'r');
-  const readObject = await filehandle.read(buf, { offset: null }, buf.length);
+  const readObject = await filehandle.read(buf, { offset: null });
   assert.strictEqual(readObject.buffer[0], 120);
 })()
 .then(common.mustCall())
@@ -48,7 +48,7 @@ let filehandle = null;
 
 (async () => {
   filehandle = await fsPromises.open(filepath, 'r');
-  const readObject = await filehandle.read(buf, null);
+  const readObject = await filehandle.read(buf, null, buf.length, 0);
   assert.strictEqual(readObject.buffer[0], 120);
 })()
 .then(common.mustCall())

From ab0b534874af180ba2cd60dcc646ebe795c88309 Mon Sep 17 00:00:00 2001
From: LiviaMedeiros <livia@cirno.name>
Date: Sat, 23 Apr 2022 00:47:38 +0800
Subject: [PATCH 16/16] squash: adjust tests

---
 test/parallel/test-fs-read-offset-null.js     | 23 +++++++++++++------
 test/parallel/test-fs-read-optional-params.js |  2 +-
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/test/parallel/test-fs-read-offset-null.js b/test/parallel/test-fs-read-offset-null.js
index 9f3d95ebdca4b4..012c94e41e92c7 100644
--- a/test/parallel/test-fs-read-offset-null.js
+++ b/test/parallel/test-fs-read-offset-null.js
@@ -14,11 +14,11 @@ const filepath = fixtures.path('x.txt');
 const buf = Buffer.alloc(1);
 // Reading only one character, hence buffer of one byte is enough.
 
-// Test is done by making sure the first letter in buffer is
+// Tests are done by making sure the first letter in buffer is
 // same as first letter in file.
 // 120 is the ascii code of letter x.
 
-// Test for callback API.
+// Tests for callback API.
 fs.open(filepath, 'r', common.mustSucceed((fd) => {
   fs.read(fd, { offset: null, buffer: buf },
           common.mustSucceed((bytesRead, buffer) => {
@@ -37,19 +37,28 @@ fs.open(filepath, 'r', common.mustSucceed((fd) => {
 
 let filehandle = null;
 
-// Test for promise api
+// Tests for promises api
 (async () => {
   filehandle = await fsPromises.open(filepath, 'r');
   const readObject = await filehandle.read(buf, { offset: null });
   assert.strictEqual(readObject.buffer[0], 120);
 })()
-.then(common.mustCall())
-.finally(() => filehandle?.close());
+.finally(() => filehandle?.close())
+.then(common.mustCall());
+
+// Undocumented: omitted position works the same as position === null
+(async () => {
+  filehandle = await fsPromises.open(filepath, 'r');
+  const readObject = await filehandle.read(buf, null, buf.length);
+  assert.strictEqual(readObject.buffer[0], 120);
+})()
+.finally(() => filehandle?.close())
+.then(common.mustCall());
 
 (async () => {
   filehandle = await fsPromises.open(filepath, 'r');
   const readObject = await filehandle.read(buf, null, buf.length, 0);
   assert.strictEqual(readObject.buffer[0], 120);
 })()
-.then(common.mustCall())
-.finally(() => filehandle?.close());
+.finally(() => filehandle?.close())
+.then(common.mustCall());
diff --git a/test/parallel/test-fs-read-optional-params.js b/test/parallel/test-fs-read-optional-params.js
index aa665c16a6fa54..2699850b0e9597 100644
--- a/test/parallel/test-fs-read-optional-params.js
+++ b/test/parallel/test-fs-read-optional-params.js
@@ -34,5 +34,5 @@ testValid('Passing in an empty object', {});
 testValid('Passing in an object', {
   offset: 0,
   length: bufferAsOption.byteLength,
-  position: 0
+  position: 0,
 });