fs: harden fs.readSync(buffer, options) typecheck

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: #42772 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs · Sep 11, 2022 · 41a6d82 · 41a6d82
1 parent 2275faa
commit 41a6d82
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 16 deletions.
diff --git a/lib/fs.js b/lib/fs.js
@@ -701,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);

diff --git a/test/parallel/test-fs-readSync-optional-params.js b/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');
 }