Skip to content

Commit 8eb0b30

Browse files
committed
fs: validate position argument in write/writeSync/promises.write
fs.read() and fs.readSync() validate the `position` argument via validatePosition() (tightened in commit ed05549). The symmetric write path was never updated: fs.write(), fs.writeSync() and fsPromises.FileHandle.write() silently coerced any non-number `position` (strings, objects, booleans, NaN, out-of-range numbers, out-of-range bigints) to `null`, which means "use the current file offset". Impact: callers relying on an ERR_OUT_OF_RANGE / ERR_INVALID_ARG_TYPE throw to reject malformed inputs instead silently got a stream-mode write at the current file offset — bypassing validation and potentially overwriting file content the caller thought it had refused. Inconsistent between read and write is a direct input- validation failure that is trivially triggerable from userland: fs.writeSync(fd, Buffer.from('PWN'), 0, 3, -2); // accepted fs.writeSync(fd, Buffer.from('PWN'), 0, 3, 'str'); // accepted fs.writeSync(fd, Buffer.from('PWN'), 0, 3, { not: 'num' }); // accepted Mirror the read-side validation in all three write entry points, add a regression test covering positional-arg, options-object and mutation-guarded-options-object invocations for sync, async and promise variants. Refs: nodejs#62674
1 parent 0b73a33 commit 8eb0b30

3 files changed

Lines changed: 135 additions & 2 deletions

File tree

lib/fs.js

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -826,8 +826,11 @@ function write(fd, buffer, offsetOrOptions, length, position, callback) {
826826
}
827827
if (typeof length !== 'number')
828828
length = buffer.byteLength - offset;
829-
if (typeof position !== 'number')
829+
if (typeof position === 'function' || position == null) {
830830
position = null;
831+
} else {
832+
validatePosition(position, 'position', length);
833+
}
831834
validateOffsetLengthWrite(offset, length, buffer.byteLength);
832835

833836
const req = new FSReqCallback();
@@ -897,6 +900,9 @@ function writeSync(fd, buffer, offsetOrOptions, length, position) {
897900
}
898901
if (typeof length !== 'number')
899902
length = buffer.byteLength - offset;
903+
if (position != null) {
904+
validatePosition(position, 'position', length);
905+
}
900906
validateOffsetLengthWrite(offset, length, buffer.byteLength);
901907
result = binding.writeBuffer(fd, buffer, offset, length, position,
902908
undefined, ctx);

lib/internal/fs/promises.js

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1379,8 +1379,11 @@ async function write(handle, buffer, offsetOrOptions, length, position) {
13791379
}
13801380
if (typeof length !== 'number')
13811381
length = buffer.byteLength - offset;
1382-
if (typeof position !== 'number')
1382+
if (position == null) {
13831383
position = null;
1384+
} else {
1385+
validatePosition(position, 'position', length);
1386+
}
13841387
validateOffsetLengthWrite(offset, length, buffer.byteLength);
13851388
const bytesWritten =
13861389
(await PromisePrototypeThen(
Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
import * as common from '../common/index.mjs';
2+
import tmpdir from '../common/tmpdir.js';
3+
import fs from 'fs';
4+
import fsp from 'fs/promises';
5+
import path from 'path';
6+
import assert from 'assert';
7+
8+
// This test ensures that the `position` argument is correctly validated by
9+
// fs.write(), fs.writeSync() and fsPromises.FileHandle.write(). The symmetric
10+
// validation was added to fs.read()/readSync() by commit ed05549e; the write
11+
// counterparts were left unchecked and silently accepted invalid values such
12+
// as strings, objects and out-of-range numbers.
13+
14+
tmpdir.refresh();
15+
const filepath = path.join(tmpdir.path, 'test-fs-write-position-validation.txt');
16+
fs.writeFileSync(filepath, Buffer.alloc(32, 0x2e));
17+
18+
const buffer = Buffer.from('xyz\n');
19+
const offset = 0;
20+
const length = buffer.byteLength;
21+
22+
async function testValidAsync(position) {
23+
return new Promise((resolve, reject) => {
24+
fs.open(filepath, 'r+', common.mustSucceed((fd) => {
25+
let callCount = 3;
26+
const handler = common.mustCall((err) => {
27+
callCount--;
28+
if (err) {
29+
fs.close(fd, common.mustSucceed());
30+
reject(err);
31+
} else if (callCount === 0) {
32+
fs.close(fd, common.mustSucceed(resolve));
33+
}
34+
}, callCount);
35+
fs.write(fd, buffer, offset, length, position, handler);
36+
fs.write(fd, buffer, { offset, length, position }, handler);
37+
fs.write(fd, buffer, common.mustNotMutateObjectDeep({ offset, length, position }), handler);
38+
}));
39+
});
40+
}
41+
42+
function testInvalidAsync(code, position) {
43+
return new Promise((resolve, reject) => {
44+
fs.open(filepath, 'r+', common.mustSucceed((fd) => {
45+
try {
46+
assert.throws(
47+
() => fs.write(fd, buffer, offset, length, position, common.mustNotCall()),
48+
{ code },
49+
);
50+
assert.throws(
51+
() => fs.write(fd, buffer, { offset, length, position }, common.mustNotCall()),
52+
{ code },
53+
);
54+
resolve();
55+
} catch (err) {
56+
reject(err);
57+
} finally {
58+
fs.close(fd, common.mustSucceed());
59+
}
60+
}));
61+
});
62+
}
63+
64+
function testInvalidSync(code, position) {
65+
const fd = fs.openSync(filepath, 'r+');
66+
try {
67+
assert.throws(
68+
() => fs.writeSync(fd, buffer, offset, length, position),
69+
{ code },
70+
);
71+
assert.throws(
72+
() => fs.writeSync(fd, buffer, { offset, length, position }),
73+
{ code },
74+
);
75+
} finally {
76+
fs.closeSync(fd);
77+
}
78+
}
79+
80+
async function testInvalidPromise(code, position) {
81+
const fh = await fsp.open(filepath, 'r+');
82+
try {
83+
await assert.rejects(
84+
fh.write(buffer, offset, length, position),
85+
{ code },
86+
);
87+
await assert.rejects(
88+
fh.write(buffer, { offset, length, position }),
89+
{ code },
90+
);
91+
} finally {
92+
await fh.close();
93+
}
94+
}
95+
96+
// Valid positions must continue to work.
97+
await testValidAsync(undefined);
98+
await testValidAsync(null);
99+
await testValidAsync(0);
100+
await testValidAsync(1);
101+
await testValidAsync(0n);
102+
await testValidAsync(1n);
103+
104+
// Out-of-range numeric positions must be rejected.
105+
for (const bad of [-2, NaN, -Infinity, Infinity, -0.999,
106+
Number.MAX_SAFE_INTEGER + 1, Number.MAX_VALUE]) {
107+
await testInvalidAsync('ERR_OUT_OF_RANGE', bad);
108+
testInvalidSync('ERR_OUT_OF_RANGE', bad);
109+
await testInvalidPromise('ERR_OUT_OF_RANGE', bad);
110+
}
111+
112+
// Out-of-range bigint positions must be rejected.
113+
for (const bad of [2n ** 63n, -(2n ** 64n)]) {
114+
await testInvalidAsync('ERR_OUT_OF_RANGE', bad);
115+
testInvalidSync('ERR_OUT_OF_RANGE', bad);
116+
await testInvalidPromise('ERR_OUT_OF_RANGE', bad);
117+
}
118+
119+
// Non-integer types must be rejected with ERR_INVALID_ARG_TYPE.
120+
for (const bad of [false, true, '1', 'not-a-number', Symbol(1), {}, []]) {
121+
await testInvalidAsync('ERR_INVALID_ARG_TYPE', bad);
122+
testInvalidSync('ERR_INVALID_ARG_TYPE', bad);
123+
await testInvalidPromise('ERR_INVALID_ARG_TYPE', bad);
124+
}

0 commit comments

Comments
 (0)