fs: harden options typecheck in reading methods

lib/fs.js

@@ -639,6 +639,9 @@ function read(fd, buffer, offsetOrOptions, length, position, callback) {
       buffer = Buffer.alloc(16384);
     }
 
+    if (params !== undefined) {
+      validateObject(params, 'options', { nullable: true });
+    }
     ({
       offset = 0,
       length = buffer.byteLength - offset,
@@ -698,26 +701,28 @@ ObjectDefineProperty(read, kCustomPromisifyArgsSymbol,
  *   offset?: number;
  *   length?: number;
  *   position?: number | bigint | null;
- *   }} [offset]
+ *   }} [offsetOrOptions]
  * @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 || kEmptyObject;
+  let offset = offsetOrOptions;
+  if (arguments.length <= 3 || typeof offsetOrOptions === 'object') {
+    if (offsetOrOptions !== undefined) {
+      validateObject(offsetOrOptions, 'options', { nullable: true });
+    }
 
     ({
       offset = 0,
       length = buffer.byteLength - offset,
       position = null,
-    } = options);
+    } = offsetOrOptions ?? kEmptyObject);
   }
 
-  if (offset == null) {
+  if (offset === undefined) {
     offset = 0;
   } else {
     validateInteger(offset, 'offset', 0);

lib/internal/fs/promises.js

@@ -77,6 +77,7 @@ const {
   validateBuffer,
   validateEncoding,
   validateInteger,
+  validateObject,
   validateString,
 } = require('internal/validators');
 const pathModule = require('path');
@@ -517,6 +518,9 @@ async function read(handle, bufferOrParams, offset, length, position) {
   let buffer = bufferOrParams;
   if (!isArrayBufferView(buffer)) {
     // This is fh.read(params)
+    if (bufferOrParams !== undefined) {
+      validateObject(bufferOrParams, 'options', { nullable: true });
+    }
     ({
       buffer = Buffer.alloc(16384),
       offset = 0,

test/parallel/test-fs-promises.js

@@ -153,14 +153,24 @@ async function executeOnHandle(dest, func) {
       });
     }
 
-    // Use fallback buffer allocation when input not buffer
+    // Use fallback buffer allocation when first argument is null
     {
       await executeOnHandle(dest, async (handle) => {
-        const ret = await handle.read(0, 0, 0, 0);
+        const ret = await handle.read(null, 0, 0, 0);
         assert.strictEqual(ret.buffer.length, 16384);
       });
     }
 
+    // TypeError if buffer is not ArrayBufferView or nullable object
+    {
+      await executeOnHandle(dest, async (handle) => {
+        await assert.rejects(
+          async () => handle.read(0, 0, 0, 0),
+          { code: 'ERR_INVALID_ARG_TYPE' }
+        );
+      });
+    }
+
     // Bytes written to file match buffer
     {
       await executeOnHandle(dest, async (handle) => {

test/parallel/test-fs-readSync-optional-params.js

@@ -8,13 +8,20 @@ const filepath = fixtures.path('x.txt');
 
 const expected = Buffer.from('xyz\n');
 
-function runTest(defaultBuffer, options) {
+function runTest(defaultBuffer, options, errorCode = false) {
   let fd;
   try {
     fd = fs.openSync(filepath, 'r');
-    const result = fs.readSync(fd, defaultBuffer, options);
-    assert.strictEqual(result, expected.length);
-    assert.deepStrictEqual(defaultBuffer, expected);
+    if (errorCode) {
+      assert.throws(
+        () => fs.readSync(fd, defaultBuffer, options),
+        { code: errorCode }
+      );
+    } else {
+      const result = fs.readSync(fd, defaultBuffer, options);
+      assert.strictEqual(result, expected.length);
+      assert.deepStrictEqual(defaultBuffer, expected);
+    }
   } finally {
     if (fd != null) fs.closeSync(fd);
   }
@@ -31,7 +38,6 @@ for (const options of [
   { length: expected.length, position: 0 },
   { offset: 0, length: expected.length, position: 0 },
 
-  { offset: null },
   { position: null },
   { position: -1 },
   { position: 0n },
@@ -41,17 +47,27 @@ for (const options of [
   null,
   undefined,
 
-  // Test if bad params are interpreted as default (not mandatory)
+  // Test malicious corner case: it works as {length: 4} but not intentionally
+  new String('4444'),
+]) {
+  runTest(Buffer.allocUnsafe(expected.length), options);
+}
+
+for (const options of [
+
+  // Test various invalid options
   false,
   true,
   Infinity,
   42n,
   Symbol(),
+  'amString',
+  [],
+  () => {},
 
-  // Test even more malicious corner cases
+  // Test if arbitrary entity with expected .length is not mistaken for options
   '4'.repeat(expected.length),
-  new String('4444'),
   [4, 4, 4, 4],
 ]) {
-  runTest(Buffer.allocUnsafe(expected.length), mustNotMutateObjectDeep(options));
+  runTest(Buffer.allocUnsafe(expected.length), mustNotMutateObjectDeep(options), 'ERR_INVALID_ARG_TYPE');
 }