From a52f15efae252f7c3b6781c13da4b63d746aaec4 Mon Sep 17 00:00:00 2001
From: Anatoli Papirovski <apapirovski@mac.com>
Date: Sat, 14 Apr 2018 09:20:07 +0200
Subject: [PATCH] timers: fix a bug in error handling

When a timeout within a list of timeouts (that consists of only
that specific timeout) throws during its execution, it's possible
for the TimerWrap handle to become shared between both that list
and an unref'd timeout created in the future. This fixes the bug
by extending error handling in timeout execution to check for
whether the current list is empty and if so do cleanup on it.

PR-URL: https://github.com/nodejs/node/pull/20497
Fixes: https://github.com/nodejs/node/issues/19970
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
---
 lib/timers.js                                 | 60 ++++++++++++-------
 test/parallel/test-timers-throw-reschedule.js | 27 +++++++++
 2 files changed, 64 insertions(+), 23 deletions(-)
 create mode 100644 test/parallel/test-timers-throw-reschedule.js

diff --git a/lib/timers.js b/lib/timers.js
index f3c3c6308433eb..e6655c2f527349 100644
--- a/lib/timers.js
+++ b/lib/timers.js
@@ -217,6 +217,17 @@ function TimersList(msecs, unrefed) {
   this.nextTick = false;
 }
 
+function deleteTimersList(list, msecs) {
+  // Either refedLists[msecs] or unrefedLists[msecs] may have been removed and
+  // recreated since the reference to `list` was created. Make sure they're
+  // the same instance of the list before destroying.
+  if (list._unrefed === true && list === unrefedLists[msecs]) {
+    delete unrefedLists[msecs];
+  } else if (list === refedLists[msecs]) {
+    delete refedLists[msecs];
+  }
+}
+
 function listOnTimeout() {
   var list = this._list;
   var msecs = list.msecs;
@@ -288,14 +299,7 @@ function listOnTimeout() {
   debug('%d list empty', msecs);
   assert(L.isEmpty(list));
 
-  // Either refedLists[msecs] or unrefedLists[msecs] may have been removed and
-  // recreated since the reference to `list` was created. Make sure they're
-  // the same instance of the list before destroying.
-  if (list._unrefed === true && list === unrefedLists[msecs]) {
-    delete unrefedLists[msecs];
-  } else if (list === refedLists[msecs]) {
-    delete refedLists[msecs];
-  }
+  deleteTimersList(list, msecs);
 
   // Do not close the underlying handle if its ownership has changed
   // (e.g it was unrefed in its callback).
@@ -329,24 +333,34 @@ function tryOnTimeout(timer, list) {
       }
     }
 
-    if (!threw) return;
+    if (threw) {
+      const { msecs } = list;
+
+      if (L.isEmpty(list)) {
+        deleteTimersList(list, msecs);
 
-    // Postpone all later list events to next tick. We need to do this
-    // so that the events are called in the order they were created.
-    const lists = list._unrefed === true ? unrefedLists : refedLists;
-    for (var key in lists) {
-      if (key > list.msecs) {
-        lists[key].nextTick = true;
+        if (!list._timer.owner)
+          list._timer.close();
+      } else {
+        // Postpone all later list events to next tick. We need to do this
+        // so that the events are called in the order they were created.
+        const lists = list._unrefed === true ? unrefedLists : refedLists;
+        for (var key in lists) {
+          if (key > msecs) {
+            lists[key].nextTick = true;
+          }
+        }
+
+        // We need to continue processing after domain error handling
+        // is complete, but not by using whatever domain was left over
+        // when the timeout threw its exception.
+        const domain = process.domain;
+        process.domain = null;
+        // If we threw, we need to process the rest of the list in nextTick.
+        process.nextTick(listOnTimeoutNT, list);
+        process.domain = domain;
       }
     }
-    // We need to continue processing after domain error handling
-    // is complete, but not by using whatever domain was left over
-    // when the timeout threw its exception.
-    const domain = process.domain;
-    process.domain = null;
-    // If we threw, we need to process the rest of the list in nextTick.
-    process.nextTick(listOnTimeoutNT, list);
-    process.domain = domain;
   }
 }
 
diff --git a/test/parallel/test-timers-throw-reschedule.js b/test/parallel/test-timers-throw-reschedule.js
new file mode 100644
index 00000000000000..b804b6effa9c69
--- /dev/null
+++ b/test/parallel/test-timers-throw-reschedule.js
@@ -0,0 +1,27 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+
+// This test checks that throwing inside a setTimeout where that Timeout
+// instance is the only one within its list of timeouts, doesn't cause
+// an issue with an unref timeout scheduled in the error handler.
+// Refs: https://github.com/nodejs/node/issues/19970
+
+const timeout = common.platformTimeout(50);
+
+const timer = setTimeout(common.mustNotCall(), 2 ** 31 - 1);
+
+process.once('uncaughtException', common.mustCall((err) => {
+  assert.strictEqual(err.message, 'setTimeout Error');
+
+  const now = Date.now();
+  setTimeout(common.mustCall(() => {
+    assert(Date.now() - now >= timeout);
+    clearTimeout(timer);
+  }), timeout).unref();
+}));
+
+setTimeout(() => {
+  throw new Error('setTimeout Error');
+}, timeout);