From 5f7e0565bb22a33a99fcfecad063dc7b15873bb5 Mon Sep 17 00:00:00 2001
From: Joyee Cheung <joyeec9h3@gmail.com>
Date: Sun, 27 May 2018 06:07:29 +0800
Subject: [PATCH] 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.
---
 doc/api/fs.md                                 |  6 ++-
 lib/internal/fs/watchers.js                   | 21 ++++++++--
 .../test-fs-watch-close-when-destroyed.js     | 38 +++++++++++++++++++
 test/parallel/test-fs-watch.js                | 19 ++++++++--
 4 files changed, 74 insertions(+), 10 deletions(-)
 create mode 100644 test/parallel/test-fs-watch-close-when-destroyed.js

diff --git a/doc/api/fs.md b/doc/api/fs.md
index 724f1b419c78ac..a07c4e6ce46446 100644
--- a/doc/api/fs.md
+++ b/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
diff --git a/lib/internal/fs/watchers.js b/lib/internal/fs/watchers.js
index 685d5be5e4db3f..1007a5a13614d2 100644
--- a/lib/internal/fs/watchers.js
+++ b/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);
 };
 
diff --git a/test/parallel/test-fs-watch-close-when-destroyed.js b/test/parallel/test-fs-watch-close-when-destroyed.js
new file mode 100644
index 00000000000000..fb5239bc446eff
--- /dev/null
+++ b/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)
+);
diff --git a/test/parallel/test-fs-watch.js b/test/parallel/test-fs-watch.js
index ffa82e52c73b72..e596f32a766926 100644
--- a/test/parallel/test-fs-watch.js
+++ b/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.