From 23c6c661fab47c16601c9af4403588ba7cfd122f Mon Sep 17 00:00:00 2001 From: LiviaMedeiros Date: Tue, 19 Apr 2022 14:19:55 +0800 Subject: [PATCH] fs: harden fs.readSync(buffer, options) typecheck Co-authored-by: Antoine du Hamel PR-URL: https://github.com/nodejs/node/pull/42772 Reviewed-By: Antoine du Hamel Reviewed-By: James M Snell --- lib/fs.js | 16 +++++---- .../test-fs-readSync-optional-params.js | 34 ++++++++++++++----- 2 files changed, 34 insertions(+), 16 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index bcfa8aa6b2b611..06aa44b303e9ca 100644 --- 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 index 5388e8037765fd..7fc1abfd91a57c 100644 --- 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'); }