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: https://github.com/nodejs/node/pull/20985
Fixes: https://github.com/nodejs/node/issues/20738
Fixes: https://github.com/nodejs/node/issues/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>

doc/api/fs.md

@@ -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
@@ -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

lib/internal/fs/watchers.js

@@ -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',
@@ -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;
   }
 
@@ -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);
 };
 

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

@@ -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)
+);

test/parallel/test-fs-watch.js

@@ -46,7 +46,8 @@ 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);
@@ -54,7 +55,11 @@ for (const testCase of cases) {
     }
     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);
@@ -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.