fs: adjust default length for fs.readSync and fsPromises/read

Makes default length reasonable when nonzero offset is set. PR-URL: #42128 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
nodejs · Apr 24, 2022 · 6b55ba2 · 6b55ba2
1 parent baeb66a
commit 6b55ba2
Show file tree

Hide file tree

Showing 4 changed files with 62 additions and 30 deletions.
diff --git a/doc/api/fs.md b/doc/api/fs.md
@@ -5118,7 +5118,7 @@ changes:
 * `buffer` {Buffer|TypedArray|DataView}
 * `options` {Object}
   * `offset` {integer} **Default:** `0`
-  * `length` {integer} **Default:** `buffer.byteLength`
+  * `length` {integer} **Default:** `buffer.byteLength - offset`
   * `position` {integer|bigint} **Default:** `null`
 * Returns: {number}
 

diff --git a/lib/fs.js b/lib/fs.js
@@ -605,7 +605,7 @@ function read(fd, buffer, offset, length, position, callback) {
 
   if (arguments.length <= 3) {
     // Assume fs.read(fd, options, callback)
-    let options = {};
+    let options = ObjectCreate(null);
     if (arguments.length < 3) {
       // This is fs.read(fd, callback)
       // buffer will be the callback
@@ -622,7 +622,7 @@ function read(fd, buffer, offset, length, position, callback) {
       buffer = Buffer.alloc(16384),
       offset = 0,
       length = buffer.byteLength - offset,
-      position
+      position = null
     } = options);
   }
 
@@ -687,10 +687,14 @@ function readSync(fd, buffer, offset, length, position) {
   validateBuffer(buffer);
 
   if (arguments.length <= 3) {
-    // Assume fs.read(fd, buffer, options)
-    const options = offset || {};
+    // Assume fs.readSync(fd, buffer, options)
+    const options = offset || ObjectCreate(null);
 
-    ({ offset = 0, length = buffer.byteLength, position } = options);
+    ({
+      offset = 0,
+      length = buffer.byteLength - offset,
+      position = null
+    } = options);
   }
 
   if (offset == null) {

diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
@@ -5,6 +5,7 @@ const {
   Error,
   MathMax,
   MathMin,
+  ObjectCreate,
   NumberIsSafeInteger,
   Promise,
   PromisePrototypeThen,
@@ -458,18 +459,15 @@ async function open(path, flags, mode) {
 async function read(handle, bufferOrOptions, offset, length, position) {
   let buffer = bufferOrOptions;
   if (!isArrayBufferView(buffer)) {
-    if (bufferOrOptions === undefined) {
-      bufferOrOptions = {};
-    }
-    if (bufferOrOptions.buffer) {
-      buffer = bufferOrOptions.buffer;
-      validateBuffer(buffer);
-    } else {
-      buffer = Buffer.alloc(16384);
-    }
-    offset = bufferOrOptions.offset || 0;
-    length = bufferOrOptions.length ?? buffer.byteLength;
-    position = bufferOrOptions.position ?? null;
+    bufferOrOptions ??= ObjectCreate(null);
+    ({
+      buffer = Buffer.alloc(16384),
+      offset = 0,
+      length = buffer.byteLength - offset,
+      position = null
+    } = bufferOrOptions);
+
+    validateBuffer(buffer);
   }
 
   if (offset == null) {

diff --git a/test/parallel/test-fs-readSync-optional-params.js b/test/parallel/test-fs-readSync-optional-params.js
@@ -5,23 +5,53 @@ 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');
 
 function runTest(defaultBuffer, options) {
-  const result = fs.readSync(fd, defaultBuffer, options);
-  assert.strictEqual(result, expected.length);
-  assert.deepStrictEqual(defaultBuffer, expected);
+  let fd;
+  try {
+    fd = fs.openSync(filepath, 'r');
+    const result = fs.readSync(fd, defaultBuffer, options);
+    assert.strictEqual(result, expected.length);
+    assert.deepStrictEqual(defaultBuffer, expected);
+  } finally {
+    if (fd != null) fs.closeSync(fd);
+  }
 }
 
-// Test passing in an empty options object
-runTest(Buffer.allocUnsafe(expected.length), { position: 0 });
+for (const options of [
 
-// Test not passing in any options object
-runTest(Buffer.allocUnsafe(expected.length));
+  // Test options object
+  { offset: 0 },
+  { length: expected.length },
+  { position: 0 },
+  { offset: 0, length: expected.length },
+  { offset: 0, position: 0 },
+  { length: expected.length, position: 0 },
+  { offset: 0, length: expected.length, position: 0 },
 
-// Test passing in options
-runTest(Buffer.allocUnsafe(expected.length), { offset: 0,
-                                               length: expected.length,
-                                               position: 0 });
+  { offset: null },
+  { position: null },
+  { position: -1 },
+  { position: 0n },
+
+  // 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);
+}