pty.c: Open PTY slave with O_NOCTTY.

Up until now, the BBS has received a SIGHUP whenever remote consoles
exited, which originally caused the BBS to exit when the BBS was
daemonized. Workarounds were added for this, beginning with commit
5952cfb.

However, the underlying cause of the SIGHUP was a single open()
call lacking O_NOCTTY, resulting in acquisition of a controlling
terminal. By providing this flag, we prevent this from happening,
ensuring that we don't receive unwanted SIGHUP signals. Previously,
if running under systemd, because we don't daemonize, SIGHUP wasn't
ignored, and thus a remote console exiting would result in SIGHUP
killing the BBS.

We still continue to ignore SIGHUP, because when running
under systemd, the BBS still receives a SIGHUP, in the same manner
as addressed in commit ea38bdc.
We use SIGUSR2 for processing reloads from systemd, so there's not
much downside to ignoring SIGHUP to ensure these things aren't
conflated. However, because we don't daemonize when running under
systemd, but because STDOUT isn't a TTY in this scenario, we now also
ignore SIGHUP if STDOUT is not a TTY. In other words, it's less
whether we're daemonized and more whether STDOUT is connected to
a terminal that determines whether we ignore SIGHUP.

Author: InterLinked1

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);

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;
 }
 

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;
 	}