Skip to content

Commit

Permalink
fs: do not crash when using a closed fs event watcher
Browse files Browse the repository at this point in the history
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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Ron Korving <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

Backport-PR-URL: #21172
  • Loading branch information
joyeecheung authored and targos committed Jun 13, 2018
1 parent b4b7d36 commit c09bfd8
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 10 deletions.
6 changes: 4 additions & 2 deletions doc/api/fs.md
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,8 @@ fs.watch('./tmp', { encoding: 'buffer' }, (eventType, filename) => {
added: v10.0.0
-->

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

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

* `error` {Error}

Emitted when an error occurs while watching the file.
Emitted when an error occurs while watching the file. The errored
`fs.FSWatcher` object is no longer usable in the event handler.

### watcher.close()
<!-- YAML
Expand Down
21 changes: 17 additions & 4 deletions lib/internal/fs/watchers.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,11 @@ function FSWatcher() {
// after the handle is closed, and to fire both UV_RENAME and UV_CHANGE
// if they are set by libuv at the same time.
if (status < 0) {
this._handle.close();
if (this._handle !== null) {
// We don't use this.close() here to avoid firing the close event.
this._handle.close();
this._handle = null; // make the handle garbage collectable
}
const error = errors.uvException({
errno: status,
syscall: 'watch',
Expand All @@ -120,13 +124,17 @@ util.inherits(FSWatcher, EventEmitter);
// 1. Throw an Error if it's the first time .start() is called
// 2. Return silently if .start() has already been called
// on a valid filename and the wrap has been initialized
// 3. Return silently if the watcher has already been closed
// This method is a noop if the watcher has already been started.
FSWatcher.prototype.start = function(filename,
persistent,
recursive,
encoding) {
if (this._handle === null) { // closed
return;
}
assert(this._handle instanceof FSEvent, 'handle must be a FSEvent');
if (this._handle.initialized) {
if (this._handle.initialized) { // already started
return;
}

Expand All @@ -148,13 +156,18 @@ FSWatcher.prototype.start = function(filename,
}
};

// This method is a noop if the watcher has not been started.
// This method is a noop if the watcher has not been started or
// has already been closed.
FSWatcher.prototype.close = function() {
if (this._handle === null) { // closed
return;
}
assert(this._handle instanceof FSEvent, 'handle must be a FSEvent');
if (!this._handle.initialized) {
if (!this._handle.initialized) { // not started
return;
}
this._handle.close();
this._handle = null; // make the handle garbage collectable
process.nextTick(emitCloseNT, this);
};

Expand Down
38 changes: 38 additions & 0 deletions test/parallel/test-fs-watch-close-when-destroyed.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
'use strict';

// This tests that closing a watcher when the underlying handle is
// already destroyed will result in a noop instead of a crash.

const common = require('../common');
const tmpdir = require('../common/tmpdir');
const fs = require('fs');
const path = require('path');

tmpdir.refresh();
const root = path.join(tmpdir.path, 'watched-directory');
fs.mkdirSync(root);

const watcher = fs.watch(root, { persistent: false, recursive: false });

// The following listeners may or may not be invoked.

watcher.addListener('error', () => {
setTimeout(
() => { watcher.close(); }, // Should not crash if it's invoked
common.platformTimeout(10)
);
});

watcher.addListener('change', () => {
setTimeout(
() => { watcher.close(); },
common.platformTimeout(10)
);
});

fs.rmdirSync(root);
// Wait for the listener to hit
setTimeout(
common.mustCall(() => {}),
common.platformTimeout(100)
);
19 changes: 15 additions & 4 deletions test/parallel/test-fs-watch.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,20 @@ for (const testCase of cases) {
fs.writeFileSync(testCase.filePath, content1);

let interval;
const watcher = fs.watch(testCase[testCase.field]);
const pathToWatch = testCase[testCase.field];
const watcher = fs.watch(pathToWatch);
watcher.on('error', (err) => {
if (interval) {
clearInterval(interval);
interval = null;
}
assert.fail(err);
});
watcher.on('close', common.mustCall());
watcher.on('close', common.mustCall(() => {
watcher.close(); // Closing a closed watcher should be a noop
// Starting a closed watcher should be a noop
watcher.start();
}));
watcher.on('change', common.mustCall(function(eventType, argFilename) {
if (interval) {
clearInterval(interval);
Expand All @@ -66,10 +71,16 @@ for (const testCase of cases) {
assert.strictEqual(eventType, 'change');
assert.strictEqual(argFilename, testCase.fileName);

watcher.start(); // Starting a started watcher should be a noop
// End of test case
// Starting a started watcher should be a noop
watcher.start();
watcher.start(pathToWatch);

watcher.close();

// We document that watchers cannot be used anymore when it's closed,
// here we turn the methods into noops instead of throwing
watcher.close(); // Closing a closed watcher should be a noop
watcher.start(); // Starting a closed watcher should be a noop
}));

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

0 comments on commit c09bfd8

Please sign in to comment.