#59 Server loops with "Console input too long!" if stdin is /dev/null

[Simon McVittie]

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1032276

If the quakespasm server is run noninteractively with stdin redirected from /dev/null (for example as a systemd service), it spams the system log with "Console input too long!".

This appears to be because it ignores EOF and errors from read(), and instead assumes that exactly one character was read, repeatedly appending an uninitialized character from the stack to the console buffer until the buffer is full.

Patch to follow.

[Simon McVittie]

Patch attached, seems to work well.

[Ozkan Sezer]

I seem to have missed this one: sorry for the late reply.

I never ran qs that way. So: con_eof is static means it won't ever process
any more input after hitting con_eof, but the way qs is ran console input
is not wanted anyway?

[Ozkan Sezer]

I seem to have missed this one: sorry for the late reply.

I never ran qs that way. So: con_eof is static means it won't ever process
any more input after hitting con_eof, but the way qs is ran console input
is not wanted anyway?

Simon McVittie doesn't seem to be responding. Steven, Eric: Can you guys
review?

[Simon McVittie]

"So: con_eof is static means it won't ever process
any more input after hitting con_eof, but the way qs is ran console input
is not wanted anyway?" - pretty much.

If stdin is not a terminal (for example /dev/null or a regular file), after you have reached EOF it will continue to poll as readable (POLLIN from poll() or readfds in select()) forever, but read() will return 0 (meaning EOF), so quakespasm will constantly try to read from it.

(Previously, quakespasm was also mishandling a read of 0 bytes (EOF) as though it was a read of 1 byte, which meant it would append 1 uninitialized byte to the buffer until it ran out of buffer; that's clearly not the intended behaviour.)

If stdin is a terminal, after you press Ctrl+D it will report "end of file" with a 0-byte read, but I think there might be some special behaviour of terminals where the same terminal can have more than one "end of file"? But if the user has signalled end of file, it seems reasonable for quakespasm to stop reading anyway. If you do want to special-case terminals so that a 0 byte read is just ignored, then isatty() would tell you whether this is a terminal or not.

[Ozkan Sezer]

I see. How about never running the read loop if stdin isn't a tty, e.g. like the following (bits from q3 source):

diff --git a/Quake/sys_sdl_unix.c b/Quake/sys_sdl_unix.c
index 8820dc5..8008f5f 100644
--- a/Quake/sys_sdl_unix.c
+++ b/Quake/sys_sdl_unix.c
@@ -51,6 +51,8 @@
 qboolean       isDedicated;
 cvar_t     sys_throttle = {"sys_throttle", "0.02", CVAR_ARCHIVE};

+static qboolean stdinIsATTY;       /* from q3 */
+
 #define    MAX_HANDLES     32  /* johnfitz -- was 10 */
 static FILE        *sys_handles[MAX_HANDLES];

@@ -338,6 +340,10 @@

 void Sys_Init (void)
 {

+   const char* term = getenv("TERM");
+   stdinIsATTY = isatty(STDIN_FILENO) &&
+           !(term && (!strcmp(term, "raw") || !strcmp(term, "dumb")));
+
    memset (cwd, 0, sizeof(cwd));
    Sys_GetBasedir(host_parms->argv[0], cwd, sizeof(cwd));
    host_parms->basedir = cwd;
@@ -423,6 +429,9 @@ const char *Sys_ConsoleInput (void)
    fd_set      set;
    struct timeval  timeout;


+   if (!stdinIsATTY)
+       return NULL;
+
    FD_ZERO (&set);
    FD_SET (0, &set);   // stdin
    timeout.tv_sec = 0;
@@ -430,7 +439,10 @@ const char *Sys_ConsoleInput (void)

    while (select (1, &set, NULL, NULL, &timeout))
    {

-       read (0, &c, 1);
+       if (read(0, &c, 1) <= 0) {
+       // read error: finish processing whatever is already in the buffer (if anything.)
+           c = '\n';
+       }
        if (c == '\n' || c == '\r')
        {
            con_text[textlen] = '\0';

[Simon McVittie]

If stdin is a socket or the read end of a pipe/fifo (or even a regular file for that matter), wouldn't you want to read everything from it until EOF is reached, and then stop?

[Ozkan Sezer]

Do we change stdin to a socket or a pipe/fifo when running quakespasm as a server, either normally or in a case you described like a systemd server?

[Simon McVittie]

It's not really up to the server author what stdin is: it's the user who runs the server that decides.

[Ozkan Sezer]

Patch applied with a minor edit. Further merged the isatty and dumb terminal detection bits.

Thanks.

[Ozkan Sezer]

Patch applied with a minor edit. Further merged the isatty and dumb terminal detection bits.

Thanks.