FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fs: guard against undefined behavior · nodejs/node@7ef5591 · GitHub

/ node Public

Commit 7ef5591

Browse files
ronag
authored and
BethGriggs
committed
fs: guard against undefined behavior
Calling close on a file description which is currently in use is undefined behavior due to implementation details in libuv. Add a guard against this when using FileHandle. PR-URL: #34746 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 332e384 commit 7ef5591

3 files changed

Lines changed: 116 additions & 31 deletions

File tree

‎doc/api/fs.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4528,7 +4528,8 @@ added: v10.0.0
45284528
file descriptor is closed, or will be rejected if an error occurs while
45294529
closing.
45304530

4531-
Closes the file descriptor.
4531+
Closes the file handle. Will wait for any pending operation on the handle
4532+
to complete before completing.
45324533

45334534
```js
45344535
const fsPromises = require('fs').promises;

‎lib/internal/fs/promises.js‎

Lines changed: 81 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ const {
1414
MathMin,
1515
NumberIsSafeInteger,
1616
Symbol,
17+
Error,
18+
Promise,
1719
} = primordials;
1820

1921
const {
@@ -64,6 +66,11 @@ const { promisify } = require('internal/util');
6466

6567
const kHandle = Symbol('kHandle');
6668
const kFd = Symbol('kFd');
69+
const kRefs = Symbol('kRefs');
70+
const kClosePromise = Symbol('kClosePromise');
71+
const kCloseResolve = Symbol('kCloseResolve');
72+
const kCloseReject = Symbol('kCloseReject');
73+
6774
const { kUsePromises } = binding;
6875
const {
6976
JSTransferable, kDeserialize, kTransfer, kTransferList
@@ -76,6 +83,9 @@ class FileHandle extends JSTransferable {
7683
super();
7784
this[kHandle] = filehandle;
7885
this[kFd] = filehandle ? filehandle.fd : -1;
86+
87+
this[kRefs] = 1;
88+
this[kClosePromise] = null;
7989
}
8090

8191
getAsyncId() {
@@ -87,70 +97,101 @@ class FileHandle extends JSTransferable {
8797
}
8898

8999
appendFile(data, options) {
90-
return writeFile(this, data, options);
100+
return fsCall(writeFile, this, data, options);
91101
}
92102

93103
chmod(mode) {
94-
return fchmod(this, mode);
104+
return fsCall(fchmod, this, mode);
95105
}
96106

97107
chown(uid, gid) {
98-
return fchown(this, uid, gid);
108+
return fsCall(fchown, this, uid, gid);
99109
}
100110

101111
datasync() {
102-
return fdatasync(this);
112+
return fsCall(fdatasync, this);
103113
}
104114

105115
sync() {
106-
return fsync(this);
116+
return fsCall(fsync, this);
107117
}
108118

109119
read(buffer, offset, length, position) {
110-
return read(this, buffer, offset, length, position);
120+
return fsCall(read, this, buffer, offset, length, position);
111121
}
112122

113123
readv(buffers, position) {
114-
return readv(this, buffers, position);
124+
return fsCall(readv, this, buffers, position);
115125
}
116126

117127
readFile(options) {
118-
return readFile(this, options);
128+
return fsCall(readFile, this, options);
119129
}
120130

121131
stat(options) {
122-
return fstat(this, options);
132+
return fsCall(fstat, this, options);
123133
}
124134

125135
truncate(len = 0) {
126-
return ftruncate(this, len);
136+
return fsCall(ftruncate, this, len);
127137
}
128138

129139
utimes(atime, mtime) {
130-
return futimes(this, atime, mtime);
140+
return fsCall(futimes, this, atime, mtime);
131141
}
132142

133143
write(buffer, offset, length, position) {
134-
return write(this, buffer, offset, length, position);
144+
return fsCall(write, this, buffer, offset, length, position);
135145
}
136146

137147
writev(buffers, position) {
138-
return writev(this, buffers, position);
148+
return fsCall(writev, this, buffers, position);
139149
}
140150

141151
writeFile(data, options) {
142-
return writeFile(this, data, options);
152+
return fsCall(writeFile, this, data, options);
143153
}
144154

145155
close = () => {
146-
this[kFd] = -1;
147-
return this[kHandle].close();
156+
if (this[kFd] === -1) {
157+
return Promise.resolve();
158+
}
159+
160+
if (this[kClosePromise]) {
161+
return this[kClosePromise];
162+
}
163+
164+
this[kRefs]--;
165+
if (this[kRefs] === 0) {
166+
this[kFd] = -1;
167+
this[kClosePromise] = this[kHandle].close().finally(() => {
168+
this[kClosePromise] = undefined;
169+
});
170+
} else {
171+
this[kClosePromise] = new Promise((resolve, reject) => {
172+
this[kCloseResolve] = resolve;
173+
this[kCloseReject] = reject;
174+
}).finally(() => {
175+
this[kClosePromise] = undefined;
176+
this[kCloseReject] = undefined;
177+
this[kCloseResolve] = undefined;
178+
});
179+
}
180+
181+
return this[kClosePromise];
148182
}
149183

150184
[kTransfer]() {
185+
if (this[kClosePromise] || this[kRefs] > 1) {
186+
const DOMException = internalBinding('messaging').DOMException;
187+
throw new DOMException('Cannot transfer FileHandle while in use',
188+
'DataCloneError');
189+
}
190+
151191
const handle = this[kHandle];
152192
this[kFd] = -1;
153193
this[kHandle] = null;
194+
this[kRefs] = 0;
154195

155196
return {
156197
data: { handle },
@@ -168,9 +209,31 @@ class FileHandle extends JSTransferable {
168209
}
169210
}
170211

171-
function validateFileHandle(handle) {
172-
if (!(handle instanceof FileHandle))
212+
async function fsCall(fn, handle, ...args) {
213+
if (handle[kRefs] === undefined) {
173214
throw new ERR_INVALID_ARG_TYPE('filehandle', 'FileHandle', handle);
215+
}
216+
217+
if (handle.fd === -1) {
218+
// eslint-disable-next-line no-restricted-syntax
219+
const err = new Error('file closed');
220+
err.code = 'EBADF';
221+
err.syscall = fn.name;
222+
throw err;
223+
}
224+
225+
try {
226+
handle[kRefs]++;
227+
return await fn(handle, ...args);
228+
} finally {
229+
handle[kRefs]--;
230+
if (handle[kRefs] === 0) {
231+
handle[kFd] = -1;
232+
handle[kHandle]
233+
.close()
234+
.then(handle[kCloseResolve], handle[kCloseReject]);
235+
}
236+
}
174237
}
175238

176239
async function writeFileHandle(filehandle, data) {
@@ -249,7 +312,6 @@ async function open(path, flags, mode) {
249312
}
250313

251314
async function read(handle, buffer, offset, length, position) {
252-
validateFileHandle(handle);
253315
validateBuffer(buffer);
254316

255317
if (offset == null) {
@@ -280,7 +342,6 @@ async function read(handle, buffer, offset, length, position) {
280342
}
281343

282344
async function readv(handle, buffers, position) {
283-
validateFileHandle(handle);
284345
validateBufferArray(buffers);
285346

286347
if (typeof position !== 'number')
@@ -292,8 +353,6 @@ async function readv(handle, buffers, position) {
292353
}
293354

294355
async function write(handle, buffer, offset, length, position) {
295-
validateFileHandle(handle);
296-
297356
if (buffer.length === 0)
298357
return { bytesWritten: 0, buffer };
299358

@@ -321,7 +380,6 @@ async function write(handle, buffer, offset, length, position) {
321380
}
322381

323382
async function writev(handle, buffers, position) {
324-
validateFileHandle(handle);
325383
validateBufferArray(buffers);
326384

327385
if (typeof position !== 'number')
@@ -346,7 +404,6 @@ async function truncate(path, len = 0) {
346404
}
347405

348406
async function ftruncate(handle, len = 0) {
349-
validateFileHandle(handle);
350407
validateInteger(len, 'len');
351408
len = MathMax(0, len);
352409
return binding.ftruncate(handle.fd, len, kUsePromises);
@@ -364,12 +421,10 @@ async function rmdir(path, options) {
364421
}
365422

366423
async function fdatasync(handle) {
367-
validateFileHandle(handle);
368424
return binding.fdatasync(handle.fd, kUsePromises);
369425
}
370426

371427
async function fsync(handle) {
372-
validateFileHandle(handle);
373428
return binding.fsync(handle.fd, kUsePromises);
374429
}
375430

@@ -420,7 +475,6 @@ async function symlink(target, path, type_) {
420475
}
421476

422477
async function fstat(handle, options = { bigint: false }) {
423-
validateFileHandle(handle);
424478
const result = await binding.fstat(handle.fd, options.bigint, kUsePromises);
425479
return getStatsFromBinding(result);
426480
}
@@ -453,7 +507,6 @@ async function unlink(path) {
453507
}
454508

455509
async function fchmod(handle, mode) {
456-
validateFileHandle(handle);
457510
mode = parseFileMode(mode, 'mode');
458511
return binding.fchmod(handle.fd, mode, kUsePromises);
459512
}
@@ -481,7 +534,6 @@ async function lchown(path, uid, gid) {
481534
}
482535

483536
async function fchown(handle, uid, gid) {
484-
validateFileHandle(handle);
485537
validateUint32(uid, 'uid');
486538
validateUint32(gid, 'gid');
487539
return binding.fchown(handle.fd, uid, gid, kUsePromises);
@@ -504,7 +556,6 @@ async function utimes(path, atime, mtime) {
504556
}
505557

506558
async function futimes(handle, atime, mtime) {
507-
validateFileHandle(handle);
508559
atime = toUnixTimestamp(atime, 'atime');
509560
mtime = toUnixTimestamp(mtime, 'mtime');
510561
return binding.futimes(handle.fd, atime, mtime, kUsePromises);

‎test/parallel/test-worker-message-port-transfer-filehandle.js‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,3 +69,36 @@ const { once } = require('events');
6969

7070
port1.postMessage('second message');
7171
})().then(common.mustCall());
72+
73+
(async function() {
74+
// Check that a FileHandle with a read in progress cannot be transferred.
75+
const fh = await fs.open(__filename);
76+
77+
const { port1 } = new MessageChannel();
78+
79+
const readPromise = fh.readFile();
80+
assert.throws(() => {
81+
port1.postMessage(fh, [fh]);
82+
}, {
83+
message: 'Cannot transfer FileHandle while in use',
84+
name: 'DataCloneError'
85+
});
86+
87+
assert.deepStrictEqual(await readPromise, await fs.readFile(__filename));
88+
})().then(common.mustCall());
89+
90+
(async function() {
91+
// Check that filehandles with a close in progress cannot be transferred.
92+
const fh = await fs.open(__filename);
93+
94+
const { port1 } = new MessageChannel();
95+
96+
const closePromise = fh.close();
97+
assert.throws(() => {
98+
port1.postMessage(fh, [fh]);
99+
}, {
100+
message: 'Cannot transfer FileHandle while in use',
101+
name: 'DataCloneError'
102+
});
103+
await closePromise;
104+
})().then(common.mustCall());

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL