Skip to content

Commit

Permalink
fs: harden fs.readSync(buffer, options) typecheck
Browse files Browse the repository at this point in the history
Co-authored-by: Antoine du Hamel <[email protected]>
PR-URL: #42772
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
  • Loading branch information
2 people authored and nodejs-github-bot committed Sep 11, 2022
1 parent 2275faa commit 41a6d82
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 16 deletions.
16 changes: 9 additions & 7 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
34 changes: 25 additions & 9 deletions test/parallel/test-fs-readSync-optional-params.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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 },
Expand All @@ -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');
}

0 comments on commit 41a6d82

Please sign in to comment.