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

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/42772
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This commit is contained in:
LiviaMedeiros 2022-04-19 14:19:55 +08:00 committed by Node.js GitHub Bot
parent 2275faac2b
commit 41a6d82968
2 changed files with 34 additions and 16 deletions

View File

@ -701,26 +701,28 @@ ObjectDefineProperty(read, kCustomPromisifyArgsSymbol,
* offset?: number; * offset?: number;
* length?: number; * length?: number;
* position?: number | bigint | null; * position?: number | bigint | null;
* }} [offset] * }} [offsetOrOptions]
* @returns {number} * @returns {number}
*/ */
function readSync(fd, buffer, offset, length, position) { function readSync(fd, buffer, offsetOrOptions, length, position) {
fd = getValidatedFd(fd); fd = getValidatedFd(fd);
validateBuffer(buffer); validateBuffer(buffer);
if (arguments.length <= 3) { let offset = offsetOrOptions;
// Assume fs.readSync(fd, buffer, options) if (arguments.length <= 3 || typeof offsetOrOptions === 'object') {
const options = offset || kEmptyObject; if (offsetOrOptions !== undefined) {
validateObject(offsetOrOptions, 'options', { nullable: true });
}
({ ({
offset = 0, offset = 0,
length = buffer.byteLength - offset, length = buffer.byteLength - offset,
position = null, position = null,
} = options); } = offsetOrOptions ?? kEmptyObject);
} }
if (offset == null) { if (offset === undefined) {
offset = 0; offset = 0;
} else { } else {
validateInteger(offset, 'offset', 0); validateInteger(offset, 'offset', 0);

View File

@ -8,13 +8,20 @@ const filepath = fixtures.path('x.txt');
const expected = Buffer.from('xyz\n'); const expected = Buffer.from('xyz\n');
function runTest(defaultBuffer, options) { function runTest(defaultBuffer, options, errorCode = false) {
let fd; let fd;
try { try {
fd = fs.openSync(filepath, 'r'); fd = fs.openSync(filepath, 'r');
const result = fs.readSync(fd, defaultBuffer, options); if (errorCode) {
assert.strictEqual(result, expected.length); assert.throws(
assert.deepStrictEqual(defaultBuffer, expected); () => 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 { } finally {
if (fd != null) fs.closeSync(fd); if (fd != null) fs.closeSync(fd);
} }
@ -31,7 +38,6 @@ for (const options of [
{ length: expected.length, position: 0 }, { length: expected.length, position: 0 },
{ offset: 0, length: expected.length, position: 0 }, { offset: 0, length: expected.length, position: 0 },
{ offset: null },
{ position: null }, { position: null },
{ position: -1 }, { position: -1 },
{ position: 0n }, { position: 0n },
@ -41,17 +47,27 @@ for (const options of [
null, null,
undefined, 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, false,
true, true,
Infinity, Infinity,
42n, 42n,
Symbol(), Symbol(),
'amString',
[],
() => {},
// Test even more malicious corner cases // Test if arbitrary entity with expected .length is not mistaken for options
'4'.repeat(expected.length), '4'.repeat(expected.length),
new String('4444'),
[4, 4, 4, 4], [4, 4, 4, 4],
]) { ]) {
runTest(Buffer.allocUnsafe(expected.length), mustNotMutateObjectDeep(options)); runTest(Buffer.allocUnsafe(expected.length), mustNotMutateObjectDeep(options), 'ERR_INVALID_ARG_TYPE');
} }