Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

repl: fix freeze if util.inspect throws & clean up error handling a bit #1968

Closed
wants to merge 2 commits into from
Closed

repl: fix freeze if util.inspect throws & clean up error handling a bit #1968

wants to merge 2 commits into from

Conversation

monsanto
Copy link
Contributor

If an error is thrown during the readline keypress event, the state of the streaming escape sequence decoder (added in #1601) will become corrupted. This will render the REPL (and possibly your whole terminal) completely unusable. This can be triggered by an object with a custom inspect method that throws.

Originally I modified the escape sequence decoder to avoid corruption but it proved difficult to test because the synchronous throw in keypress would end up corrupting the state of the mock streams I was using to test (although for whatever reason it would work with the real TTY streams). Felt too fragile to commit.

Instead, I decided that none of the eval machinery should ever throw. This PR adds an extra try/catch around self.writer (which is util.inspect by default) and makes some minor cleanups to make error handling more consistent.

Note: our test suite currently forbids running any of the REPL machinery in a different tick, which would sidestep most, if not all, of these problems.

cc @rlidwka

If an error is thrown during the readline `keypress` event, the state of
the streaming escape sequence decoder (added in
#1601) will become corrupted. This
will render the REPL (and possibly your whole terminal) completely
unusable. This can be triggered by an object with a custom `inspect`
method that throws.

This commit adds an extra try/catch around `self.writer` (which is
`util.inspect` by default).
@@ -157,12 +157,6 @@ function REPLServer(prompt,
}
} catch (e) {
err = e;
if (err && process.domain) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is moved to finish so all of the error handling can be in one place. It doesn't seem safe to do this here anyway since the evaluation function can be overriden. The explicit process.domain.exit is not carried over as 1) I don't understand what use it serves & I don't like to blindly c/p, 2) the other existing domain emit didn't come with an exit and 3) the repl+domain test passes without it. @isaacs you added this 2 years ago, do you remember?

@monsanto monsanto added the repl Issues and PRs related to the REPL subsystem. label Jun 13, 2015
@rlidwka
Copy link
Contributor

rlidwka commented Jun 13, 2015

keypress event in readline is considered a public api. If readline is not usable after that throws, it's a very bad thing. Suggested fix is something like this (I could do a separate pull request):

diff --git a/lib/readline.js b/lib/readline.js
index 4107c99..b8f52ee 100644
--- a/lib/readline.js
+++ b/lib/readline.js
@@ -910,7 +910,15 @@ function emitKeypressEvents(stream) {
       var r = stream[KEYPRESS_DECODER].write(b);
       if (r) {
         for (var i = 0; i < r.length; i++) {
-          stream[ESCAPE_DECODER].next(r[i]);
+          try {
+            stream[ESCAPE_DECODER].next(r[i]);
+          } catch (err) {
+            // if the generator throws (it could happen in the `keypress`
+            // event), we need to restart it.
+            stream[ESCAPE_DECODER] = emitKeys(stream);
+            stream[ESCAPE_DECODER].next();
+            throw err;
+          }
         }
       }
     } else {

Which is still bad because throw will break for loop, and the rest of the chunk will get eaten, but at least nothing will freeze permanently.

I have no opinion about other changes. Maybe a guarantee that REPL never throws is a good thing.

@Fishrock123
Copy link
Contributor

I think I'd prefer @rlidwka's fix, i.e. to fix it in readline.

@jasnell
Copy link
Member

jasnell commented Nov 16, 2015

Alternative solution was landed in #2107

@jasnell jasnell closed this Nov 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants