From c78de13238444d0b91607bff49195c38da669ce5 Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Thu, 17 May 2018 16:25:36 +0200
Subject: [PATCH] repl: handle uncaughtException properly

When running the REPL as standalone program it's now possible to use
`process.on('uncaughtException', listener)`. It is going to use those
listeners from now on and the regular error output is suppressed.

It also fixes the issue that REPL instances started inside of an
application would silence all application errors. It is now prohibited
to add the exception listener in such REPL instances. Trying to add
such listeners throws an `ERR_INVALID_REPL_INPUT` error.

Fixes: https://github.com/nodejs/node/issues/19998

PR-URL: https://github.com/nodejs/node/pull/27151
Fixes: https://github.com/nodejs/node/issues/19998
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
---
 doc/api/errors.md                             | 11 ++-
 doc/api/repl.md                               | 34 ++++++++-
 lib/internal/errors.js                        |  1 +
 lib/repl.js                                   | 59 +++++++++++----
 .../parallel/test-repl-pretty-custom-stack.js |  2 -
 .../test-repl-uncaught-exception-async.js     | 44 ++++++++++++
 ...test-repl-uncaught-exception-standalone.js | 38 ++++++++++
 test/parallel/test-repl-uncaught-exception.js | 72 +++++++++++++++++++
 8 files changed, 244 insertions(+), 17 deletions(-)
 create mode 100644 test/parallel/test-repl-uncaught-exception-async.js
 create mode 100644 test/parallel/test-repl-uncaught-exception-standalone.js
 create mode 100644 test/parallel/test-repl-uncaught-exception.js

diff --git a/doc/api/errors.md b/doc/api/errors.md
index 2cb4bc067e24b8..9420e07b16c871 100644
--- a/doc/api/errors.md
+++ b/doc/api/errors.md
@@ -1310,8 +1310,14 @@ An invalid `options.protocol` was passed to `http.request()`.
 <a id="ERR_INVALID_REPL_EVAL_CONFIG"></a>
 ### ERR_INVALID_REPL_EVAL_CONFIG
 
-Both `breakEvalOnSigint` and `eval` options were set in the REPL config, which
-is not supported.
+Both `breakEvalOnSigint` and `eval` options were set in the [`REPL`][] config,
+which is not supported.
+
+<a id="ERR_INVALID_REPL_INPUT"></a>
+### ERR_INVALID_REPL_INPUT
+
+The input may not be used in the [`REPL`][]. All prohibited inputs are
+documented in the [`REPL`][]'s documentation.
 
 <a id="ERR_INVALID_RETURN_PROPERTY"></a>
 ### ERR_INVALID_RETURN_PROPERTY
@@ -2307,6 +2313,7 @@ such as `process.stdout.on('data')`.
 [`Class: assert.AssertionError`]: assert.html#assert_class_assert_assertionerror
 [`ERR_INVALID_ARG_TYPE`]: #ERR_INVALID_ARG_TYPE
 [`EventEmitter`]: events.html#events_class_eventemitter
+[`REPL`]: repl.html
 [`Writable`]: stream.html#stream_class_stream_writable
 [`child_process`]: child_process.html
 [`cipher.getAuthTag()`]: crypto.html#crypto_cipher_getauthtag
diff --git a/doc/api/repl.md b/doc/api/repl.md
index e0ec27b7e3bada..563817c632297f 100644
--- a/doc/api/repl.md
+++ b/doc/api/repl.md
@@ -138,16 +138,47 @@ global or scoped variable, the input `fs` will be evaluated on-demand as
 ```
 
 #### Global Uncaught Exceptions
+<!-- YAML
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/27151
+    description: The `'uncaughtException'` event is from now on triggered if the
+                 repl is used as standalone program.
+-->
 
 The REPL uses the [`domain`][] module to catch all uncaught exceptions for that
 REPL session.
 
 This use of the [`domain`][] module in the REPL has these side effects:
 
-* Uncaught exceptions do not emit the [`'uncaughtException'`][] event.
+* Uncaught exceptions only emit the [`'uncaughtException'`][] event if the
+  `repl` is used as standalone program. If the `repl` is included anywhere in
+  another application, adding a listener for this event will throw an
+  [`ERR_INVALID_REPL_INPUT`][] exception.
 * Trying to use [`process.setUncaughtExceptionCaptureCallback()`][] throws
   an [`ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE`][] error.
 
+As standalone program:
+
+```js
+process.on('uncaughtException', () => console.log('Uncaught'));
+
+throw new Error('foobar');
+// Uncaught
+```
+
+When used in another application:
+
+```js
+process.on('uncaughtException', () => console.log('Uncaught'));
+// TypeError [ERR_INVALID_REPL_INPUT]: Listeners for `uncaughtException`
+// cannot be used in the REPL
+
+throw new Error('foobar');
+// Thrown:
+// Error: foobar
+```
+
 #### Assignment of the `_` (underscore) variable
 <!-- YAML
 changes:
@@ -661,6 +692,7 @@ For an example of running a REPL instance over [curl(1)][], see:
 [`'uncaughtException'`]: process.html#process_event_uncaughtexception
 [`--experimental-repl-await`]: cli.html#cli_experimental_repl_await
 [`ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE`]: errors.html#errors_err_domain_cannot_set_uncaught_exception_capture
+[`ERR_INVALID_REPL_INPUT`]: errors.html#errors_err_invalid_repl_input
 [`domain`]: domain.html
 [`process.setUncaughtExceptionCaptureCallback()`]: process.html#process_process_setuncaughtexceptioncapturecallback_fn
 [`readline.InterfaceCompleter`]: readline.html#readline_use_of_the_completer_function
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
index 51e9fcba400ce8..52587f1f4e0971 100644
--- a/lib/internal/errors.js
+++ b/lib/internal/errors.js
@@ -895,6 +895,7 @@ E('ERR_INVALID_PROTOCOL',
   TypeError);
 E('ERR_INVALID_REPL_EVAL_CONFIG',
   'Cannot specify both "breakEvalOnSigint" and "eval" for REPL', TypeError);
+E('ERR_INVALID_REPL_INPUT', '%s', TypeError);
 E('ERR_INVALID_RETURN_PROPERTY', (input, name, prop, value) => {
   return `Expected a valid ${input} to be returned for the "${prop}" from the` +
          ` "${name}" function but got ${value}.`;
diff --git a/lib/repl.js b/lib/repl.js
index d439d205a40e0c..e4a366664c10d6 100644
--- a/lib/repl.js
+++ b/lib/repl.js
@@ -72,6 +72,7 @@ const {
   ERR_CANNOT_WATCH_SIGINT,
   ERR_INVALID_ARG_TYPE,
   ERR_INVALID_REPL_EVAL_CONFIG,
+  ERR_INVALID_REPL_INPUT,
   ERR_SCRIPT_EXECUTION_INTERRUPTED
 } = require('internal/errors').codes;
 const { sendInspectorCommand } = require('internal/util/inspector');
@@ -101,10 +102,13 @@ let processTopLevelAwait;
 
 const parentModule = module;
 const replMap = new WeakMap();
+const domainSet = new WeakSet();
 
 const kBufferedCommandSymbol = Symbol('bufferedCommand');
 const kContextId = Symbol('contextId');
 
+let addedNewListener = false;
+
 try {
   // Hack for require.resolve("./relative") to work properly.
   module.filename = path.resolve('repl');
@@ -204,6 +208,28 @@ function REPLServer(prompt,
     throw new ERR_INVALID_REPL_EVAL_CONFIG();
   }
 
+  // Add this listener only once and use a WeakSet that contains the REPLs
+  // domains. Otherwise we'd have to add a single listener to each REPL instance
+  // and that could trigger the `MaxListenersExceededWarning`.
+  if (!options[kStandaloneREPL] && !addedNewListener) {
+    process.prependListener('newListener', (event, listener) => {
+      if (event === 'uncaughtException' &&
+          process.domain &&
+          listener.name !== 'domainUncaughtExceptionClear' &&
+          domainSet.has(process.domain)) {
+        // Throw an error so that the event will not be added and the current
+        // domain takes over. That way the user is notified about the error
+        // and the current code evaluation is stopped, just as any other code
+        // that contains an error.
+        throw new ERR_INVALID_REPL_INPUT(
+          'Listeners for `uncaughtException` cannot be used in the REPL');
+      }
+    });
+    addedNewListener = true;
+  }
+
+  domainSet.add(this._domain);
+
   let rli = this;
   Object.defineProperty(this, 'rli', {
     get: deprecate(() => rli,
@@ -264,7 +290,7 @@ function REPLServer(prompt,
       // statement rather than an object literal.  So, we first try
       // to wrap it in parentheses, so that it will be interpreted as
       // an expression.  Note that if the above condition changes,
-      // lib/internal/repl/recoverable.js needs to be changed to match.
+      // lib/internal/repl/utils.js needs to be changed to match.
       code = `(${code.trim()})\n`;
       wrappedCmd = true;
     }
@@ -461,22 +487,31 @@ function REPLServer(prompt,
       }
     }
 
-    if (errStack === '') {
-      errStack = `Thrown: ${self.writer(e)}\n`;
-    } else {
-      const ln = errStack.endsWith('\n') ? '' : '\n';
-      errStack = `Thrown:\n${errStack}${ln}`;
-    }
-
     if (!self.underscoreErrAssigned) {
       self.lastError = e;
     }
 
     const top = replMap.get(self);
-    top.outputStream.write(errStack);
-    top.clearBufferedCommand();
-    top.lines.level = [];
-    top.displayPrompt();
+    if (options[kStandaloneREPL] &&
+        process.listenerCount('uncaughtException') !== 0) {
+      process.nextTick(() => {
+        process.emit('uncaughtException', e);
+        top.clearBufferedCommand();
+        top.lines.level = [];
+        top.displayPrompt();
+      });
+    } else {
+      if (errStack === '') {
+        errStack = `Thrown: ${self.writer(e)}\n`;
+      } else {
+        const ln = errStack.endsWith('\n') ? '' : '\n';
+        errStack = `Thrown:\n${errStack}${ln}`;
+      }
+      top.outputStream.write(errStack);
+      top.clearBufferedCommand();
+      top.lines.level = [];
+      top.displayPrompt();
+    }
   });
 
   self.resetContext();
diff --git a/test/parallel/test-repl-pretty-custom-stack.js b/test/parallel/test-repl-pretty-custom-stack.js
index a37cf04263433d..b65287c5f55b1f 100644
--- a/test/parallel/test-repl-pretty-custom-stack.js
+++ b/test/parallel/test-repl-pretty-custom-stack.js
@@ -40,8 +40,6 @@ process.on('uncaughtException', (e) => {
   throw e;
 });
 
-process.on('exit', () => (Error.prepareStackTrace = origPrepareStackTrace));
-
 const tests = [
   {
     // test .load for a file that throws
diff --git a/test/parallel/test-repl-uncaught-exception-async.js b/test/parallel/test-repl-uncaught-exception-async.js
new file mode 100644
index 00000000000000..7b14ebd5d94caa
--- /dev/null
+++ b/test/parallel/test-repl-uncaught-exception-async.js
@@ -0,0 +1,44 @@
+'use strict';
+
+// This verifies that adding an `uncaughtException` listener in an REPL instance
+// does not suppress errors in the whole application. Adding such listener
+// should throw.
+
+require('../common');
+const ArrayStream = require('../common/arraystream');
+const repl = require('repl');
+const assert = require('assert');
+
+let accum = '';
+
+const output = new ArrayStream();
+output.write = (data) => accum += data.replace('\r', '');
+
+const r = repl.start({
+  prompt: '',
+  input: new ArrayStream(),
+  output,
+  terminal: false,
+  useColors: false,
+  global: false
+});
+
+r.write(
+  'process.nextTick(() => {\n' +
+  '  process.on("uncaughtException", () => console.log("Foo"));\n' +
+  '  throw new TypeError("foobar");\n' +
+  '});\n'
+);
+r.write(
+  'setTimeout(() => {\n' +
+  '  throw new RangeError("abc");\n' +
+  '}, 1);console.log()\n'
+);
+r.close();
+
+setTimeout(() => {
+  const len = process.listenerCount('uncaughtException');
+  process.removeAllListeners('uncaughtException');
+  assert.strictEqual(len, 0);
+  assert(/ERR_INVALID_REPL_INPUT.*(?!Type)RangeError: abc/s.test(accum));
+}, 2);
diff --git a/test/parallel/test-repl-uncaught-exception-standalone.js b/test/parallel/test-repl-uncaught-exception-standalone.js
new file mode 100644
index 00000000000000..ae7b214fefae9e
--- /dev/null
+++ b/test/parallel/test-repl-uncaught-exception-standalone.js
@@ -0,0 +1,38 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const cp = require('child_process');
+const child = cp.spawn(process.execPath, ['-i']);
+let output = '';
+
+child.stdout.setEncoding('utf8');
+child.stdout.on('data', (data) => {
+  output += data;
+});
+
+child.on('exit', common.mustCall(() => {
+  const results = output.split('\n');
+  results.shift();
+  assert.deepStrictEqual(
+    results,
+    [
+      'Type ".help" for more information.',
+      // x\n
+      '> Thrown:',
+      'ReferenceError: x is not defined',
+      // Added `uncaughtException` listener.
+      '> short',
+      'undefined',
+      // x\n
+      '> Foobar',
+      '> '
+    ]
+  );
+}));
+
+child.stdin.write('x\n');
+child.stdin.write(
+  'process.on("uncaughtException", () => console.log("Foobar"));' +
+  'console.log("short")\n');
+child.stdin.write('x\n');
+child.stdin.end();
diff --git a/test/parallel/test-repl-uncaught-exception.js b/test/parallel/test-repl-uncaught-exception.js
new file mode 100644
index 00000000000000..447b69b1d4a29e
--- /dev/null
+++ b/test/parallel/test-repl-uncaught-exception.js
@@ -0,0 +1,72 @@
+'use strict';
+require('../common');
+const ArrayStream = require('../common/arraystream');
+const assert = require('assert');
+const repl = require('repl');
+
+let count = 0;
+
+function run({ command, expected }) {
+  let accum = '';
+
+  const output = new ArrayStream();
+  output.write = (data) => accum += data.replace('\r', '');
+
+  const r = repl.start({
+    prompt: '',
+    input: new ArrayStream(),
+    output,
+    terminal: false,
+    useColors: false
+  });
+
+  r.write(`${command}\n`);
+  if (typeof expected === 'string') {
+    assert.strictEqual(accum, expected);
+  } else {
+    assert(expected.test(accum), accum);
+  }
+
+  // Verify that the repl is still working as expected.
+  accum = '';
+  r.write('1 + 1\n');
+  assert.strictEqual(accum, '2\n');
+  r.close();
+  count++;
+}
+
+const tests = [
+  {
+    command: 'x',
+    expected: 'Thrown:\n' +
+              'ReferenceError: x is not defined\n'
+  },
+  {
+    command: 'process.on("uncaughtException", () => console.log("Foobar"));\n',
+    expected: /^Thrown:\nTypeError \[ERR_INVALID_REPL_INPUT]: Listeners for `/
+  },
+  {
+    command: 'x;\n',
+    expected: 'Thrown:\n' +
+              'ReferenceError: x is not defined\n'
+  },
+  {
+    command: 'process.on("uncaughtException", () => console.log("Foobar"));' +
+             'console.log("Baz");\n',
+    expected: /^Thrown:\nTypeError \[ERR_INVALID_REPL_INPUT]: Listeners for `/
+  },
+  {
+    command: 'console.log("Baz");' +
+             'process.on("uncaughtException", () => console.log("Foobar"));\n',
+    expected: /^Baz\nThrown:\nTypeError \[ERR_INVALID_REPL_INPUT]:.*uncaughtException/
+  }
+];
+
+process.on('exit', () => {
+  // To actually verify that the test passed we have to make sure no
+  // `uncaughtException` listeners exist anymore.
+  process.removeAllListeners('uncaughtException');
+  assert.strictEqual(count, tests.length);
+});
+
+tests.forEach(run);