From 5872705796d89d48bc4bf5f0b35678b0bd148ec2 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 24 May 2019 16:11:19 +0200 Subject: [PATCH] src: restore stdio on program exit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Record the state of the stdio file descriptors on start-up and restore them to that state on exit. This should prevent issues where node.js sometimes leaves stdio in raw or non-blocking mode. This is a reworked version of commit c2c9c0c3d3 from May 2018 that was reverted in commit 14dc17df38 from June 2018. The revert was a little light on details but I infer that the problem was caused by a missing call to `uv_tty_reset_mode()`. Apropos the NOLINT comments: cpplint doesn't understand do/while statements, it thinks they're while statements without a body. Fixes: https://github.com/nodejs/node/issues/14752 Fixes: https://github.com/nodejs/node/issues/21020 Original-PR-URL: https://github.com/nodejs/node/pull/20592 PR-URL: https://github.com/nodejs/node/pull/24260 Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Refael Ackermann (רפאל פלחי) Reviewed-By: Anna Henningsen --- src/node.cc | 94 ++++++++++++++++++++++++++++++++++++--- src/node_errors.cc | 2 +- src/node_internals.h | 1 + src/node_main_instance.cc | 2 +- 4 files changed, 91 insertions(+), 8 deletions(-) diff --git a/src/node.cc b/src/node.cc index e168d62c51bb3b..388a9ea4486a88 100644 --- a/src/node.cc +++ b/src/node.cc @@ -108,6 +108,7 @@ #else #include #include // getrlimit, setrlimit +#include // tcgetattr, tcsetattr #include // STDIN_FILENO, STDERR_FILENO #endif @@ -194,7 +195,7 @@ void WaitForInspectorDisconnect(Environment* env) { #ifdef __POSIX__ void SignalExit(int signo, siginfo_t* info, void* ucontext) { - uv_tty_reset_mode(); + ResetStdio(); raise(signo); } #endif // __POSIX__ @@ -486,7 +487,7 @@ void TrapWebAssemblyOrContinue(int signo, siginfo_t* info, void* ucontext) { if (prev != nullptr) { prev(signo, info, ucontext); } else { - uv_tty_reset_mode(); + ResetStdio(); raise(signo); } } @@ -516,6 +517,16 @@ void RegisterSignalHandler(int signal, #endif // __POSIX__ +#ifdef __POSIX__ +static struct { + int flags; + bool isatty; + struct stat stat; + struct termios termios; +} stdio[1 + STDERR_FILENO]; +#endif // __POSIX__ + + inline void PlatformInit() { #ifdef __POSIX__ #if HAVE_INSPECTOR @@ -526,9 +537,9 @@ inline void PlatformInit() { #endif // HAVE_INSPECTOR // Make sure file descriptors 0-2 are valid before we start logging anything. - for (int fd = STDIN_FILENO; fd <= STDERR_FILENO; fd += 1) { - struct stat ignored; - if (fstat(fd, &ignored) == 0) + for (auto& s : stdio) { + const int fd = &s - stdio; + if (fstat(fd, &s.stat) == 0) continue; // Anything but EBADF means something is seriously wrong. We don't // have to special-case EINTR, fstat() is not interruptible. @@ -536,6 +547,8 @@ inline void PlatformInit() { ABORT(); if (fd != open("/dev/null", O_RDWR)) ABORT(); + if (fstat(fd, &s.stat) != 0) + ABORT(); } #if HAVE_INSPECTOR @@ -558,6 +571,27 @@ inline void PlatformInit() { } #endif // !NODE_SHARED_MODE + // Record the state of the stdio file descriptors so we can restore it + // on exit. Needs to happen before installing signal handlers because + // they make use of that information. + for (auto& s : stdio) { + const int fd = &s - stdio; + int err; + + do + s.flags = fcntl(fd, F_GETFL); + while (s.flags == -1 && errno == EINTR); // NOLINT + CHECK_NE(s.flags, -1); + + if (!isatty(fd)) continue; + s.isatty = true; + + do + err = tcgetattr(fd, &s.termios); + while (err == -1 && errno == EINTR); // NOLINT + CHECK_EQ(err, 0); + } + RegisterSignalHandler(SIGINT, SignalExit, true); RegisterSignalHandler(SIGTERM, SignalExit, true); @@ -611,6 +645,54 @@ inline void PlatformInit() { #endif // _WIN32 } + +// Safe to call more than once and from signal handlers. +void ResetStdio() { + uv_tty_reset_mode(); +#ifdef __POSIX__ + for (auto& s : stdio) { + const int fd = &s - stdio; + + struct stat tmp; + if (-1 == fstat(fd, &tmp)) { + CHECK_EQ(errno, EBADF); // Program closed file descriptor. + continue; + } + + bool is_same_file = + (s.stat.st_dev == tmp.st_dev && s.stat.st_ino == tmp.st_ino); + if (!is_same_file) continue; // Program reopened file descriptor. + + int flags; + do + flags = fcntl(fd, F_GETFL); + while (flags == -1 && errno == EINTR); // NOLINT + CHECK_NE(flags, -1); + + // Restore the O_NONBLOCK flag if it changed. + if (O_NONBLOCK & (flags ^ s.flags)) { + flags &= ~O_NONBLOCK; + flags |= s.flags & O_NONBLOCK; + + int err; + do + err = fcntl(fd, F_SETFL, flags); + while (err == -1 && errno == EINTR); // NOLINT + CHECK_NE(err, -1); + } + + if (s.isatty) { + int err; + do + err = tcsetattr(fd, TCSANOW, &s.termios); + while (err == -1 && errno == EINTR); // NOLINT + CHECK_NE(err, -1); + } + } +#endif // __POSIX__ +} + + int ProcessGlobalArgs(std::vector* args, std::vector* exec_args, std::vector* errors, @@ -866,7 +948,7 @@ void Init(int* argc, } InitializationResult InitializeOncePerProcess(int argc, char** argv) { - atexit([] () { uv_tty_reset_mode(); }); + atexit(ResetStdio); PlatformInit(); per_process::node_start_time = uv_hrtime(); diff --git a/src/node_errors.cc b/src/node_errors.cc index baf08b30458677..50469b09a8a151 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -230,7 +230,7 @@ void AppendExceptionLine(Environment* env, Mutex::ScopedLock lock(per_process::tty_mutex); env->set_printed_error(true); - uv_tty_reset_mode(); + ResetStdio(); PrintErrorString("\n%s", source.c_str()); return; } diff --git a/src/node_internals.h b/src/node_internals.h index 9f6f5b19ad8d0d..691b954dc4d591 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -91,6 +91,7 @@ void PrintCaughtException(v8::Isolate* isolate, const v8::TryCatch& try_catch); void WaitForInspectorDisconnect(Environment* env); +void ResetStdio(); // Safe to call more than once and from signal handlers. #ifdef __POSIX__ void SignalExit(int signal, siginfo_t* info, void* ucontext); #endif diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index 5c84dd64e6fa64..f49b9fbb4d3f30 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -145,7 +145,7 @@ int NodeMainInstance::Run() { env->set_can_call_into_js(false); env->stop_sub_worker_contexts(); - uv_tty_reset_mode(); + ResetStdio(); env->RunCleanup(); RunAtExit(env.get());