From d879b6307734a41a10dd04eaa3c6a12fc06f6e4c Mon Sep 17 00:00:00 2001
From: Timothy Gu <timothygu99@gmail.com>
Date: Sat, 2 Dec 2017 18:37:56 -0800
Subject: [PATCH] src: make FSEventWrap/StatWatcher::Start more robust

PR-URL: https://github.com/nodejs/node/pull/17432
Fixes: https://github.com/nodejs/node/issues/17430
Reviewed-By: James M Snell <jasnell@gmail.com>
---
 src/fs_event_wrap.cc               |  3 +-
 src/node_stat_watcher.cc           |  8 +++++-
 test/parallel/test-fs-watch.js     |  2 ++
 test/parallel/test-fs-watchfile.js | 45 ++++++++++++++++--------------
 4 files changed, 35 insertions(+), 23 deletions(-)

diff --git a/src/fs_event_wrap.cc b/src/fs_event_wrap.cc
index 934b9d545c6ba4..85a09060a11edc 100644
--- a/src/fs_event_wrap.cc
+++ b/src/fs_event_wrap.cc
@@ -111,7 +111,8 @@ void FSEventWrap::Start(const FunctionCallbackInfo<Value>& args) {
 
   FSEventWrap* wrap;
   ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
-  CHECK_EQ(wrap->initialized_, false);
+  if (wrap->initialized_)
+    return args.GetReturnValue().Set(0);
 
   static const char kErrMsg[] = "filename must be a string or Buffer";
   if (args.Length() < 1)
diff --git a/src/node_stat_watcher.cc b/src/node_stat_watcher.cc
index cb48c652c97b29..9aa0c950591d16 100644
--- a/src/node_stat_watcher.cc
+++ b/src/node_stat_watcher.cc
@@ -111,9 +111,15 @@ void StatWatcher::Start(const FunctionCallbackInfo<Value>& args) {
   const bool persistent = args[1]->BooleanValue();
   const uint32_t interval = args[2]->Uint32Value();
 
-  if (!persistent)
+  if (uv_is_active(reinterpret_cast<uv_handle_t*>(wrap->watcher_)))
+    return;
+  // Safe, uv_ref/uv_unref are idempotent.
+  if (persistent)
+    uv_ref(reinterpret_cast<uv_handle_t*>(wrap->watcher_));
+  else
     uv_unref(reinterpret_cast<uv_handle_t*>(wrap->watcher_));
   uv_fs_poll_start(wrap->watcher_, Callback, *path, interval);
+
   wrap->ClearWeak();
 }
 
diff --git a/test/parallel/test-fs-watch.js b/test/parallel/test-fs-watch.js
index bf5fc9a8e1ed75..a82535d5378e86 100644
--- a/test/parallel/test-fs-watch.js
+++ b/test/parallel/test-fs-watch.js
@@ -64,6 +64,8 @@ for (const testCase of cases) {
       assert.strictEqual(eventType, 'change');
     assert.strictEqual(argFilename, testCase.fileName);
 
+    watcher.start();  // should not crash
+
     // end of test case
     watcher.close();
   }));
diff --git a/test/parallel/test-fs-watchfile.js b/test/parallel/test-fs-watchfile.js
index fe0d89c42e031f..163eac5ae90a22 100644
--- a/test/parallel/test-fs-watchfile.js
+++ b/test/parallel/test-fs-watchfile.js
@@ -52,27 +52,30 @@ common.refreshTmpDir();
 // time, the callback should be invoked again with proper values in stat object
 let fileExists = false;
 
-fs.watchFile(enoentFile, { interval: 0 }, common.mustCall(function(curr, prev) {
-  if (!fileExists) {
-    // If the file does not exist, all the fields should be zero and the date
-    // fields should be UNIX EPOCH time
-    assert.deepStrictEqual(curr, expectedStatObject);
-    assert.deepStrictEqual(prev, expectedStatObject);
-    // Create the file now, so that the callback will be called back once the
-    // event loop notices it.
-    fs.closeSync(fs.openSync(enoentFile, 'w'));
-    fileExists = true;
-  } else {
-    // If the ino (inode) value is greater than zero, it means that the file is
-    // present in the filesystem and it has a valid inode number.
-    assert(curr.ino > 0);
-    // As the file just got created, previous ino value should be lesser than
-    // or equal to zero (non-existent file).
-    assert(prev.ino <= 0);
-    // Stop watching the file
-    fs.unwatchFile(enoentFile);
-  }
-}, 2));
+const watcher =
+  fs.watchFile(enoentFile, { interval: 0 }, common.mustCall((curr, prev) => {
+    if (!fileExists) {
+      // If the file does not exist, all the fields should be zero and the date
+      // fields should be UNIX EPOCH time
+      assert.deepStrictEqual(curr, expectedStatObject);
+      assert.deepStrictEqual(prev, expectedStatObject);
+      // Create the file now, so that the callback will be called back once the
+      // event loop notices it.
+      fs.closeSync(fs.openSync(enoentFile, 'w'));
+      fileExists = true;
+    } else {
+      // If the ino (inode) value is greater than zero, it means that the file
+      // is present in the filesystem and it has a valid inode number.
+      assert(curr.ino > 0);
+      // As the file just got created, previous ino value should be lesser than
+      // or equal to zero (non-existent file).
+      assert(prev.ino <= 0);
+      // Stop watching the file
+      fs.unwatchFile(enoentFile);
+    }
+  }, 2));
+
+watcher.start();  // should not crash
 
 // Watch events should callback with a filename on supported systems.
 // Omitting AIX. It works but not reliably.