From 6753ad28e913fd7bf8e58a09a87d7406779fa9f1 Mon Sep 17 00:00:00 2001 From: LiviaMedeiros Date: Fri, 25 Feb 2022 19:34:50 +0800 Subject: [PATCH 1/6] fs: adjust default `length` for `fs.readSync` and fsPromises/`read` Makes default length reasonable when nonzero offset is set. --- lib/fs.js | 8 ++++++-- lib/internal/fs/promises.js | 21 +++++++++------------ 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index b8a5cefa1b0aa8..79525fb3632557 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -686,10 +686,14 @@ function readSync(fd, buffer, offset, length, position) { validateBuffer(buffer); if (arguments.length <= 3) { - // Assume fs.read(fd, buffer, options) + // Assume fs.readSync(fd, buffer, options) const options = offset || {}; - ({ 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 index fbc4de640ade3c..84e8e7a7ef5078 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -510,18 +510,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 ??= {}; + ({ + buffer = Buffer.alloc(16384), + offset = 0, + length = buffer.byteLength - offset, + position = null + } = bufferOrOptions); + + validateBuffer(buffer); } if (offset == null) { From a836687156a06432a435b3dcfc34380a760aaea8 Mon Sep 17 00:00:00 2001 From: LiviaMedeiros Date: Fri, 25 Feb 2022 19:36:21 +0800 Subject: [PATCH 2/6] doc: adjust default `length` for `fs.readSync` Makes default length reasonable when nonzero offset is set. --- 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 f2e4e2757cb4eb..2f9705752556d9 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -5396,7 +5396,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} From c6f38977758151f6c5bf842430722140c4300193 Mon Sep 17 00:00:00 2001 From: LiviaMedeiros Date: Sat, 26 Feb 2022 04:16:18 +0800 Subject: [PATCH 3/6] fs: replace default object with null-prototyped To prevent glitches if Object.prototype has been mutated. --- lib/fs.js | 6 +++--- lib/internal/fs/promises.js | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 79525fb3632557..611eaaa90c6c9c 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -604,7 +604,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 @@ -621,7 +621,7 @@ function read(fd, buffer, offset, length, position, callback) { buffer = Buffer.alloc(16384), offset = 0, length = buffer.byteLength - offset, - position + position = null } = options); } @@ -687,7 +687,7 @@ function readSync(fd, buffer, offset, length, position) { if (arguments.length <= 3) { // Assume fs.readSync(fd, buffer, options) - const options = offset || {}; + const options = offset || ObjectCreate(null); ({ offset = 0, diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 84e8e7a7ef5078..868c7df2f1ed79 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -5,6 +5,7 @@ const { Error, MathMax, MathMin, + ObjectCreate, NumberIsSafeInteger, Promise, PromisePrototypeThen, @@ -510,7 +511,7 @@ async function open(path, flags, mode) { async function read(handle, bufferOrOptions, offset, length, position) { let buffer = bufferOrOptions; if (!isArrayBufferView(buffer)) { - bufferOrOptions ??= {}; + bufferOrOptions ??= ObjectCreate(null); ({ buffer = Buffer.alloc(16384), offset = 0, From d933f01b7f926894e8e73cf42bb75f20e8fa64d6 Mon Sep 17 00:00:00 2001 From: LiviaMedeiros Date: Sat, 26 Feb 2022 18:04:45 +0800 Subject: [PATCH 4/6] test: expand fs.readSync tests --- .../test-fs-readSync-optional-params.js | 54 ++++++++++++++----- 1 file changed, 42 insertions(+), 12 deletions(-) diff --git a/test/parallel/test-fs-readSync-optional-params.js b/test/parallel/test-fs-readSync-optional-params.js index 37d3d24911db51..dc43a3dd32886d 100644 --- 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 sane 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 fda600ea9d4b84e304d6a1a1c1c7ef7e825eb771 Mon Sep 17 00:00:00 2001 From: LiviaMedeiros Date: Sat, 26 Feb 2022 18:12:15 +0800 Subject: [PATCH 5/6] squash: lint --- test/parallel/test-fs-readSync-optional-params.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-fs-readSync-optional-params.js b/test/parallel/test-fs-readSync-optional-params.js index dc43a3dd32886d..a2cdc1e5126a72 100644 --- a/test/parallel/test-fs-readSync-optional-params.js +++ b/test/parallel/test-fs-readSync-optional-params.js @@ -54,4 +54,4 @@ for (const options of [ [4, 4, 4, 4], ]) { runTest(Buffer.allocUnsafe(expected.length), options); -}; +} From c2fdd450fc8942339a38948da798544e147213ab Mon Sep 17 00:00:00 2001 From: Livia Medeiros <74449973+LiviaMedeiros@users.noreply.github.com> Date: Sat, 26 Feb 2022 19:22:17 +0800 Subject: [PATCH 6/6] Update test/parallel/test-fs-readSync-optional-params.js Co-authored-by: Antoine du Hamel --- test/parallel/test-fs-readSync-optional-params.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-fs-readSync-optional-params.js b/test/parallel/test-fs-readSync-optional-params.js index a2cdc1e5126a72..00f1a5531cf6ea 100644 --- a/test/parallel/test-fs-readSync-optional-params.js +++ b/test/parallel/test-fs-readSync-optional-params.js @@ -36,7 +36,7 @@ for (const options of [ { position: -1 }, { position: 0n }, - // Test sane default params + // Test default params {}, null, undefined,