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 c09bfd8

Browse filesBrowse files
joyeecheungtargos
joyeecheung
authored and
targos
committed
fs: do not crash when using a closed fs event watcher
Before this commit, when the user calls methods on a closed or errored fs event watcher, they could hit a crash since the FSEventWrap in C++ land may have already been destroyed with the internal pointer set to nullptr. This commit makes sure that the user cannot hit crashes like that, instead the methods calling on a closed watcher will be noops. Also explicitly documents that the watchers should not be used in `close` and `error` event handlers. PR-URL: #20985 Fixes: #20738 Fixes: #20297 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Ron Korving <ron@ronkorving.nl> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Backport-PR-URL: #21172
1 parent b4b7d36 commit c09bfd8
Copy full SHA for c09bfd8

File tree

Expand file treeCollapse file tree

4 files changed

+74
-10
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

4 files changed

+74
-10
lines changed
Open diff view settings
Collapse file

‎doc/api/fs.md‎

Copy file name to clipboardExpand all lines: doc/api/fs.md
+4-2Lines changed: 4 additions & 2 deletions
  • Display the source diff
  • Display the rich diff
Original file line numberDiff line numberDiff line change
@@ -325,7 +325,8 @@ fs.watch('./tmp', { encoding: 'buffer' }, (eventType, filename) => {
325325
added: v10.0.0
326326
-->
327327

328-
Emitted when the watcher stops watching for changes.
328+
Emitted when the watcher stops watching for changes. The closed
329+
`fs.FSWatcher` object is no longer usable in the event handler.
329330

330331
### Event: 'error'
331332
<!-- YAML
@@ -334,7 +335,8 @@ added: v0.5.8
334335

335336
* `error` {Error}
336337

337-
Emitted when an error occurs while watching the file.
338+
Emitted when an error occurs while watching the file. The errored
339+
`fs.FSWatcher` object is no longer usable in the event handler.
338340

339341
### watcher.close()
340342
<!-- YAML
Collapse file

‎lib/internal/fs/watchers.js‎

Copy file name to clipboardExpand all lines: lib/internal/fs/watchers.js
+17-4Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,11 @@ function FSWatcher() {
100100
// after the handle is closed, and to fire both UV_RENAME and UV_CHANGE
101101
// if they are set by libuv at the same time.
102102
if (status < 0) {
103-
this._handle.close();
103+
if (this._handle !== null) {
104+
// We don't use this.close() here to avoid firing the close event.
105+
this._handle.close();
106+
this._handle = null; // make the handle garbage collectable
107+
}
104108
const error = errors.uvException({
105109
errno: status,
106110
syscall: 'watch',
@@ -120,13 +124,17 @@ util.inherits(FSWatcher, EventEmitter);
120124
// 1. Throw an Error if it's the first time .start() is called
121125
// 2. Return silently if .start() has already been called
122126
// on a valid filename and the wrap has been initialized
127+
// 3. Return silently if the watcher has already been closed
123128
// This method is a noop if the watcher has already been started.
124129
FSWatcher.prototype.start = function(filename,
125130
persistent,
126131
recursive,
127132
encoding) {
133+
if (this._handle === null) { // closed
134+
return;
135+
}
128136
assert(this._handle instanceof FSEvent, 'handle must be a FSEvent');
129-
if (this._handle.initialized) {
137+
if (this._handle.initialized) { // already started
130138
return;
131139
}
132140

@@ -148,13 +156,18 @@ FSWatcher.prototype.start = function(filename,
148156
}
149157
};
150158

151-
// This method is a noop if the watcher has not been started.
159+
// This method is a noop if the watcher has not been started or
160+
// has already been closed.
152161
FSWatcher.prototype.close = function() {
162+
if (this._handle === null) { // closed
163+
return;
164+
}
153165
assert(this._handle instanceof FSEvent, 'handle must be a FSEvent');
154-
if (!this._handle.initialized) {
166+
if (!this._handle.initialized) { // not started
155167
return;
156168
}
157169
this._handle.close();
170+
this._handle = null; // make the handle garbage collectable
158171
process.nextTick(emitCloseNT, this);
159172
};
160173

Collapse file

‎test/parallel/test-fs-watch-close-when-destroyed.js‎

Copy file name to clipboard
+38Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
'use strict';
2+
3+
// This tests that closing a watcher when the underlying handle is
4+
// already destroyed will result in a noop instead of a crash.
5+
6+
const common = require('../common');
7+
const tmpdir = require('../common/tmpdir');
8+
const fs = require('fs');
9+
const path = require('path');
10+
11+
tmpdir.refresh();
12+
const root = path.join(tmpdir.path, 'watched-directory');
13+
fs.mkdirSync(root);
14+
15+
const watcher = fs.watch(root, { persistent: false, recursive: false });
16+
17+
// The following listeners may or may not be invoked.
18+
19+
watcher.addListener('error', () => {
20+
setTimeout(
21+
() => { watcher.close(); }, // Should not crash if it's invoked
22+
common.platformTimeout(10)
23+
);
24+
});
25+
26+
watcher.addListener('change', () => {
27+
setTimeout(
28+
() => { watcher.close(); },
29+
common.platformTimeout(10)
30+
);
31+
});
32+
33+
fs.rmdirSync(root);
34+
// Wait for the listener to hit
35+
setTimeout(
36+
common.mustCall(() => {}),
37+
common.platformTimeout(100)
38+
);
Collapse file

‎test/parallel/test-fs-watch.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-fs-watch.js
+15-4Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -46,15 +46,20 @@ for (const testCase of cases) {
4646
fs.writeFileSync(testCase.filePath, content1);
4747

4848
let interval;
49-
const watcher = fs.watch(testCase[testCase.field]);
49+
const pathToWatch = testCase[testCase.field];
50+
const watcher = fs.watch(pathToWatch);
5051
watcher.on('error', (err) => {
5152
if (interval) {
5253
clearInterval(interval);
5354
interval = null;
5455
}
5556
assert.fail(err);
5657
});
57-
watcher.on('close', common.mustCall());
58+
watcher.on('close', common.mustCall(() => {
59+
watcher.close(); // Closing a closed watcher should be a noop
60+
// Starting a closed watcher should be a noop
61+
watcher.start();
62+
}));
5863
watcher.on('change', common.mustCall(function(eventType, argFilename) {
5964
if (interval) {
6065
clearInterval(interval);
@@ -66,10 +71,16 @@ for (const testCase of cases) {
6671
assert.strictEqual(eventType, 'change');
6772
assert.strictEqual(argFilename, testCase.fileName);
6873

69-
watcher.start(); // Starting a started watcher should be a noop
70-
// End of test case
74+
// Starting a started watcher should be a noop
75+
watcher.start();
76+
watcher.start(pathToWatch);
77+
7178
watcher.close();
79+
80+
// We document that watchers cannot be used anymore when it's closed,
81+
// here we turn the methods into noops instead of throwing
7282
watcher.close(); // Closing a closed watcher should be a noop
83+
watcher.start(); // Starting a closed watcher should be a noop
7384
}));
7485

7586
// Long content so it's actually flushed. toUpperCase so there's real change.

0 commit comments

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