Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 541be52

Browse filesBrowse files
addaleaxdanielleadams
addaleax
authored and
danielleadams
committed
fs: do not throw exception after creating FSReqCallback
Once an `FSReqCallback` instance is created, it is a GC root until the underlying fs operation has completed, meaning that it cannot be garbage collected. This is a problem when the underlying operation never starts because an exception is thrown before that happens, for example as part of parameter validation. Instead, move all potentially throwing code before the `FSReqCallback` creation. PR-URL: #35487 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
1 parent e09f7f0 commit 541be52
Copy full SHA for 541be52

File tree

Expand file treeCollapse file tree

2 files changed

+55
-34
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

2 files changed

+55
-34
lines changed
Open diff view settings
Collapse file

‎lib/fs.js‎

Copy file name to clipboardExpand all lines: lib/fs.js
+48-27Lines changed: 48 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@
2828
// See https://github.com/libuv/libuv/pull/1501.
2929
const kIoMaxLength = 2 ** 31 - 1;
3030

31+
// When using FSReqCallback, make sure to create the object only *after* all
32+
// parameter validation has happened, so that the objects are not kept in memory
33+
// in case they are created but never used due to an exception.
34+
3135
const {
3236
Map,
3337
MathMax,
@@ -198,8 +202,10 @@ function access(path, mode, callback) {
198202

199203
path = getValidatedPath(path);
200204
mode = getValidMode(mode, 'access');
205+
callback = makeCallback(callback);
206+
201207
const req = new FSReqCallback();
202-
req.oncomplete = makeCallback(callback);
208+
req.oncomplete = callback;
203209
binding.access(pathModule.toNamespacedPath(path), mode, req);
204210
}
205211

@@ -307,19 +313,19 @@ function readFile(path, options, callback) {
307313
const context = new ReadFileContext(callback, options.encoding);
308314
context.isUserFd = isFd(path); // File descriptor ownership
309315

310-
const req = new FSReqCallback();
311-
req.context = context;
312-
req.oncomplete = readFileAfterOpen;
313-
314316
if (context.isUserFd) {
315-
process.nextTick(function tick() {
316-
req.oncomplete(null, path);
317-
});
317+
process.nextTick(function tick(context) {
318+
readFileAfterOpen.call({ context }, null, path);
319+
}, context);
318320
return;
319321
}
320322

321-
path = getValidatedPath(path);
322323
const flagsNumber = stringToFlags(options.flags);
324+
path = getValidatedPath(path);
325+
326+
const req = new FSReqCallback();
327+
req.context = context;
328+
req.oncomplete = readFileAfterOpen;
323329
binding.open(pathModule.toNamespacedPath(path),
324330
flagsNumber,
325331
0o666,
@@ -416,8 +422,10 @@ function readFileSync(path, options) {
416422

417423
function close(fd, callback) {
418424
validateInt32(fd, 'fd', 0);
425+
callback = makeCallback(callback);
426+
419427
const req = new FSReqCallback();
420-
req.oncomplete = makeCallback(callback);
428+
req.oncomplete = callback;
421429
binding.close(fd, req);
422430
}
423431

@@ -590,12 +598,11 @@ function readv(fd, buffers, position, callback) {
590598

591599
validateInt32(fd, 'fd', /* min */ 0);
592600
validateBufferArray(buffers);
601+
callback = maybeCallback(callback || position);
593602

594603
const req = new FSReqCallback();
595604
req.oncomplete = wrapper;
596605

597-
callback = maybeCallback(callback || position);
598-
599606
if (typeof position !== 'number')
600607
position = null;
601608

@@ -712,12 +719,11 @@ function writev(fd, buffers, position, callback) {
712719

713720
validateInt32(fd, 'fd', 0);
714721
validateBufferArray(buffers);
722+
callback = maybeCallback(callback || position);
715723

716724
const req = new FSReqCallback();
717725
req.oncomplete = wrapper;
718726

719-
callback = maybeCallback(callback || position);
720-
721727
if (typeof position !== 'number')
722728
position = null;
723729

@@ -819,8 +825,10 @@ function ftruncate(fd, len = 0, callback) {
819825
validateInt32(fd, 'fd', 0);
820826
validateInteger(len, 'len');
821827
len = MathMax(0, len);
828+
callback = makeCallback(callback);
829+
822830
const req = new FSReqCallback();
823-
req.oncomplete = makeCallback(callback);
831+
req.oncomplete = callback;
824832
binding.ftruncate(fd, len, req);
825833
}
826834

@@ -989,8 +997,10 @@ function fstat(fd, options = { bigint: false }, callback) {
989997
options = {};
990998
}
991999
validateInt32(fd, 'fd', 0);
1000+
callback = makeStatsCallback(callback);
1001+
9921002
const req = new FSReqCallback(options.bigint);
993-
req.oncomplete = makeStatsCallback(callback);
1003+
req.oncomplete = callback;
9941004
binding.fstat(fd, options.bigint, req);
9951005
}
9961006

@@ -1001,6 +1011,7 @@ function lstat(path, options = { bigint: false }, callback) {
10011011
}
10021012
callback = makeStatsCallback(callback);
10031013
path = getValidatedPath(path);
1014+
10041015
const req = new FSReqCallback(options.bigint);
10051016
req.oncomplete = callback;
10061017
binding.lstat(pathModule.toNamespacedPath(path), options.bigint, req);
@@ -1013,6 +1024,7 @@ function stat(path, options = { bigint: false }, callback) {
10131024
}
10141025
callback = makeStatsCallback(callback);
10151026
path = getValidatedPath(path);
1027+
10161028
const req = new FSReqCallback(options.bigint);
10171029
req.oncomplete = callback;
10181030
binding.stat(pathModule.toNamespacedPath(path), options.bigint, req);
@@ -1070,9 +1082,6 @@ function symlink(target, path, type_, callback_) {
10701082
target = getValidatedPath(target, 'target');
10711083
path = getValidatedPath(path);
10721084

1073-
const req = new FSReqCallback();
1074-
req.oncomplete = callback;
1075-
10761085
if (isWindows && type === null) {
10771086
let absoluteTarget;
10781087
try {
@@ -1087,18 +1096,25 @@ function symlink(target, path, type_, callback_) {
10871096
stat(absoluteTarget, (err, stat) => {
10881097
const resolvedType = !err && stat.isDirectory() ? 'dir' : 'file';
10891098
const resolvedFlags = stringToSymlinkType(resolvedType);
1090-
binding.symlink(preprocessSymlinkDestination(target,
1091-
resolvedType,
1092-
path),
1099+
const destination = preprocessSymlinkDestination(target,
1100+
resolvedType,
1101+
path);
1102+
1103+
const req = new FSReqCallback();
1104+
req.oncomplete = callback;
1105+
binding.symlink(destination,
10931106
pathModule.toNamespacedPath(path), resolvedFlags, req);
10941107
});
10951108
return;
10961109
}
10971110
}
10981111

1112+
const destination = preprocessSymlinkDestination(target, type, path);
1113+
10991114
const flags = stringToSymlinkType(type);
1100-
binding.symlink(preprocessSymlinkDestination(target, type, path),
1101-
pathModule.toNamespacedPath(path), flags, req);
1115+
const req = new FSReqCallback();
1116+
req.oncomplete = callback;
1117+
binding.symlink(destination, pathModule.toNamespacedPath(path), flags, req);
11021118
}
11031119

11041120
function symlinkSync(target, path, type) {
@@ -1255,9 +1271,10 @@ function fchown(fd, uid, gid, callback) {
12551271
validateInt32(fd, 'fd', 0);
12561272
validateInteger(uid, 'uid', -1, kMaxUserId);
12571273
validateInteger(gid, 'gid', -1, kMaxUserId);
1274+
callback = makeCallback(callback);
12581275

12591276
const req = new FSReqCallback();
1260-
req.oncomplete = makeCallback(callback);
1277+
req.oncomplete = callback;
12611278
binding.fchown(fd, uid, gid, req);
12621279
}
12631280

@@ -1316,8 +1333,10 @@ function futimes(fd, atime, mtime, callback) {
13161333
validateInt32(fd, 'fd', 0);
13171334
atime = toUnixTimestamp(atime, 'atime');
13181335
mtime = toUnixTimestamp(mtime, 'mtime');
1336+
callback = makeCallback(callback);
1337+
13191338
const req = new FSReqCallback();
1320-
req.oncomplete = makeCallback(callback);
1339+
req.oncomplete = callback;
13211340
binding.futimes(fd, atime, mtime, req);
13221341
}
13231342

@@ -1920,8 +1939,10 @@ function copyFile(src, dest, mode, callback) {
19201939
src = pathModule._makeLong(src);
19211940
dest = pathModule._makeLong(dest);
19221941
mode = getValidMode(mode, 'copyFile');
1942+
callback = makeCallback(callback);
1943+
19231944
const req = new FSReqCallback();
1924-
req.oncomplete = makeCallback(callback);
1945+
req.oncomplete = callback;
19251946
binding.copyFile(src, dest, mode, req);
19261947
}
19271948

Collapse file

‎lib/internal/fs/read_file_context.js‎

Copy file name to clipboardExpand all lines: lib/internal/fs/read_file_context.js
+7-7Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -100,18 +100,18 @@ class ReadFileContext {
100100
}
101101

102102
close(err) {
103+
if (this.isUserFd) {
104+
process.nextTick(function tick(context) {
105+
readFileAfterClose.call({ context }, null);
106+
}, this);
107+
return;
108+
}
109+
103110
const req = new FSReqCallback();
104111
req.oncomplete = readFileAfterClose;
105112
req.context = this;
106113
this.err = err;
107114

108-
if (this.isUserFd) {
109-
process.nextTick(function tick() {
110-
req.oncomplete(null);
111-
});
112-
return;
113-
}
114-
115115
close(this.fd, req);
116116
}
117117
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.