Fix a use of uninitialised memory in an agetty error path.

get_logname() assumes that when it calls read() it initializes c and errno, which isn't always true if we hit a whitelisted error or end of file. This occasionally shows up as agetty going into an infinite loop. Fix it by just delaying ten seconds and exiting when things go wrong, similarly to the behavior after a non-whitelisted error.

Signed-off-by Steven Smith <[email protected]>

---
 term-utils/agetty.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

term-utils/agetty.c +4 -1

index 9763fcd30..bc848a25a 100644
@@ -317,6 +317,7 @@ static void termio_final(struct options *op,
 static int caps_lock(char *s);
 static speed_t bcode(char *s);
 static void usage(void) __attribute__((__noreturn__));
+static void exit_slowly(int code) __attribute__((__noreturn__));
 static void log_err(const char *, ...) __attribute__((__noreturn__))
 			       __attribute__((__format__(printf, 1, 2)));
 static void log_warn (const char *, ...)

@@ -1983,9 +1984,11 @@ static char *get_logname(struct options *op, struct termios *tp, struct chardata
 		while (cp->eol == '\0') {
 
 			char key;
+			ssize_t readres;
 
 			debug("read from FD\n");
-			if (read(STDIN_FILENO, &c, 1) < 1) {
+			readres = read(STDIN_FILENO, &c, 1);
+			if (readres < 0) {
 				debug("read failed\n");
 
 				/* The terminal could be open with O_NONBLOCK when

term-utils/agetty.c static char *get_logname(struct options *op, struct termios *tp, struct chardata · line 2006

Quoted 6 lines

term-utils/agetty.c +1 -1

@@ -2000,12 +2003,15 @@ static char *get_logname(struct options *op, struct termios *tp, struct chardata
 				case ESRCH:
 				case EINVAL:
 				case ENOENT:
-					break;
+					exit_slowly(EXIT_SUCCESS);

OK, makes sense.

Quoted 5 lines

term-utils/agetty.c +0 -0

 				default:
 					log_err(_("%s: read: %m"), op->tty);
 				}
 			}
 

term-utils/agetty.c static char *get_logname(struct options *op, struct termios *tp, struct chardata · line 2013

Quoted 2 lines

term-utils/agetty.c +2 -0

+			if (readres == 0)
+				exit_slowly(EXIT_SUCCESS);

I'm not sure about it. Maybe it would be better to assume that readres == 0 and no error is the same as c = 0. In this case functions returns NULL and we try another speed setting (if defined) for the terminal.

Quoted 6 lines

term-utils/agetty.c static char *get_logname(struct options *op, struct termios *tp, struct chardata · line 2013

Quoted 2 lines

term-utils/agetty.c +2 -0

+			if (readres == 0)
+				exit_slowly(EXIT_SUCCESS);

I'm not sure about it. Maybe it would be better to assume that readres == 0 and no error is the same as c = 0. In this case functions returns NULL and we try another speed setting (if defined) for the terminal.

Hmm, maybe. The problem with that would be what happens if there isn't another speed configured (which would match the system I was looking at when I started looking at this). In that case, get_logname() will immediately retry the read, without doing anything to clear the end of file condition, which would be an infinite loop. Even if I configured multiple bauds, the test environment which originally saw the issue is a VCONSOLE, so the loop in main() which calls get_logname() would immediately retry without actually doing anything with the configured speed, which I don't think would have helped very much.

Perhaps changing the baud on a real serial port would have cleared the end of file condition (or perhaps that's just showing my ignorance of how serial ports work)? That might make something like this more reasonable:

term-utils/agetty.c +4 -1

@@ -2000,12 +2003,15 @@ static char *get_logname(struct options *op, struct termios *tp, struct chardata
                                case ESRCH:
                                case EINVAL:
                                case ENOENT:
-                                       break;
+                                       exit_slowly(EXIT_SUCCESS);
                                default:
                                        log_err(_("%s: read: %m"), op->tty);
                                }
                        }
 
+                       if (readres == 0)
+                               c = 0;
+
                        /* Do parity bit handling. */
                        if (eightbit)
                                ascval = c;

Quoted 18 lines

term-utils/agetty.c +4 -1

@@ -2000,12 +2003,15 @@ static char *get_logname(struct options *op, struct termios *tp, struct chardata
                                case ESRCH:
                                case EINVAL:
                                case ENOENT:
-                                       break;
+                                       exit_slowly(EXIT_SUCCESS);
                                default:
                                        log_err(_("%s: read: %m"), op->tty);
                                }
                        }
 
+                       if (readres == 0)
+                               c = 0;
+
                        /* Do parity bit handling. */
                        if (eightbit)
                                ascval = c;

term-utils/agetty.c static char *get_logname(struct options *op, struct termios *tp, struct chardata · line 2042

Quoted 7 lines

term-utils/agetty.c +2 -0

@@ -2030,6 +2036,8 @@ static char *get_logname(struct options *op, struct termios *tp, struct chardata
                        switch (key) {
                        case 0:
                                *bp = 0;
+                               if (op->flags & F_VCONSOLE)
+                                       exit_slowly(EXIT_SUCCESS);
                                if (op->numspeed > 1)
                                        return NULL;

Yes, that's what I've though about. I have committed a little bit modified version:

(quoted diff) +3 -1

-                               if (op->numspeed > 1)
+                               if (op->numspeed > 1 && !(op->flags & F_VCONSOLE))
                                        return NULL;
+                               if (readres == 0)
+                                       exit_slowly(EXIT_SUCCESS);
                                break;

the speed configuration should be accepted only for !F_VCONSOLE.