diff --git a/bbs/bbs.c b/bbs/bbs.c index 2a9e232..a7f9d85 100644 --- a/bbs/bbs.c +++ b/bbs/bbs.c @@ -972,23 +972,34 @@ static void set_signals(void) /* Use sigaction instead of signal, since it's the more modern and well-behaving API: */ sigaction(SIGINT, &sigint_handler, NULL); sigaction(SIGTERM, &sigint_handler, NULL); /* Almost identical handling, but without a confirmation */ - if (!option_nofork) { - /* If daemonized, we get a SIGHUP whenever a remote sysop console disconnects... ignore it, - * or the BBS will get killed by the signal. - * Frequently used by daemonized processes to reread their configuration, - * for now we just ignore it. - * If running in the foreground, then allow SIGHUP to terminate as usual. + if (!option_nofork || !isatty(STDOUT_FILENO)) { + /* SIGHUP is conventionally used by daemon processes to reload their configuration, + * since they don't typically need to deal with TTYs closing. * - * Also, note that installing SIG_IGN (via &ignore_sig_handler) - * is critically important here. Per sigaction(2), on execve, - * handled signals are reset to the default, while ignored - * signals are left unchanged. + * If the BBS is run with a foreground console though, then it may make sense to + * allow SIGHUP to be processed per its prescribed meaning and terminate the BBS. + * + * However, not all instances of the BBS running in the foreground mean that + * we should process SIGHUP. For example, under systemd, the BBS is run in the + * foreground (not daemonized), but we won't have a TTY available, + * so it doesn't make sense to process SIGHUP in this case, either. + * + * For both of these scenarios, we explicitly ignore SIGHUP using SIG_IGN. + * Note that installing SIG_IGN (via &ignore_sig_handler) is critically important here. + * Per sigaction(2), on execve, handled signals are reset to the default, + * while ignored signals are left unchanged. * * Using SIG_IGN for SIGHUP avoids an issue with BBS restarts where * as soon as we execvp to restart, the BBS is killed with a SIGHUP signal. * Previously, this would occur sporadically (and only when the BBS is daemonized). * (stracing the BBS prior to the restart would make this happen consistently.) - * With SIG_IGN, the signal is reliably ignored, including across restarts. */ + * With SIG_IGN, the signal is reliably ignored, including across restarts. + * + * Because we ignore SIGHUP, in some cases even while running as a service, + * we can't use SIGHUP for its alternate conventional use of triggering a reload. + * Instead, we use SIGUSR2 for that. This allows us to safely ignore SIGHUP + * when we don't need to receive it from a foreground console disconnect. + */ sigaction(SIGHUP, &ignore_sig_handler, NULL); } sigaction(SIGWINCH, &sigwinch_handler, NULL); diff --git a/bbs/fd.c b/bbs/fd.c index 5d25250..d509356 100644 --- a/bbs/fd.c +++ b/bbs/fd.c @@ -211,6 +211,28 @@ int __bbs_open(const char *file, int line, const char *func, const char *path, i va_list ap; int mode; +/* Proactively ensure we don't inadvertently acquire controlling terminal. + * Okay to enable in production. */ +#define CHECK_FOR_CONTROLLING_TERMINAL + +/* Always add the O_NOCTTY flag blindly to every open(). + * While this is correct and harmless, this should not normally be used since it masks bugs, + * the bugs being not using O_NOCTTY at the appropriate calling site. + * Only use for debugging when things have already gone wrong. */ +/* #define PREVENT_ACCIDENTAL_CONTROLLING_TERMINAL */ + +#ifdef PREVENT_ACCIDENTAL_CONTROLLING_TERMINAL +#if O_NOCTTY != 0 + /* Since there is really never a scenario in which we want to acquire controlling terminal + * by opening something, it is always safe to do this. + * In fact, on many non-Linux systems, O_NOCTTY is 0 (which honestly makes more sense). + * However, to be semantic, the open call itself should have O_NOCTTY if needed. + * Therefore, rather than always blindly adding this flag, + * this should only be done for debugging (since doing this will make it work properly). */ + flags |= O_NOCTTY; +#endif +#endif /* PREVENT_ACCIDENTAL_CONTROLLING_TERMINAL */ + if (flags & O_CREAT) { char sflags[80]; va_start(ap, flags); @@ -240,6 +262,28 @@ int __bbs_open(const char *file, int line, const char *func, const char *path, i res = open(path, flags); STORE_COMMON_HELPER(res, "open", "\"%s\",%d", path, flags); } + +#ifdef CHECK_FOR_CONTROLLING_TERMINAL + /* We do not want to ever have any controlling terminal. + * Otherwise, when it exits, the BBS receives a SIGHUP. + * In the worst case, this could cause the BBS to exit if the signal isn't ignored. + * At best, it's wasting a signal that can be used for other purposes. */ + if (res != -1 && isatty(res) && !(flags & O_NOCTTY)) { + /* We only need to bother checking if the returned file descriptor is a TTY to begin with, + * and if the original flags did NOT include O_NOCTTY, since if they did, it wouldn't be. */ + int ctty = open("/dev/tty", 0); /* If it's not -1, that means we're a controlling terminal (BAD!) */ + /* If we got here, this assertion is almost certainly going to fail... but confirm, to be sure */ + if (bbs_assertion_failed(ctty == -1)) { + /* Technically, just because we happen to have a controlling terminal now, + * doesn't mean this open is what actually caused that. + * Something else may have happened just prior to the open() call, or just after it. + * But there's a good chance this is the offender. */ + __bbs_log(LOG_WARNING, 0, file, line, func, "Yikes, we just acquired a controlling terminal!\n"); + close(ctty); + } + } +#endif + return res; } diff --git a/bbs/pty.c b/bbs/pty.c index f2a6f8e..b3e665d 100644 --- a/bbs/pty.c +++ b/bbs/pty.c @@ -124,8 +124,12 @@ int bbs_openpty(int *amaster, int *aslave, char *name, const struct termios *ter } /* Open the slave here and just return the file descriptor of the slave, - * rather than returning its name and making the caller open the slave. */ - slave = open(slavename, O_RDWR); + * rather than returning its name and making the caller open the slave. + * + * O_NOCTTY is necessary here, specifically, to prevent inadverent acquisition of controlling terminal, + * e.g. by remote sysop consoles. Without this, when the BBS is run under systemd, + * when a remote console closes, a SIGHUP is sent to the BBS, killing it. */ + slave = open(slavename, O_RDWR | O_NOCTTY); if (slave == -1) { return -1; }