* [PATCH] Fix a use of uninitialised memory in an agetty error path.
@ 2017-11-17 16:44 Steven Smith
2017-11-20 11:02 ` Karel Zak
0 siblings, 1 reply; 5+ messages in thread
From: Steven Smith @ 2017-11-17 16:44 UTC (permalink / raw)
To: util-linux; +Cc: sos22
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 <sos22@srcf.ucam.org>
---
term-utils/agetty.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/term-utils/agetty.c b/term-utils/agetty.c
index 9763fcd30..bc848a25a 100644
--- a/term-utils/agetty.c
+++ b/term-utils/agetty.c
@@ -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
@@ -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)
+ exit_slowly(EXIT_SUCCESS);
+
/* Do parity bit handling. */
if (eightbit)
ascval = c;
@@ -2317,6 +2323,13 @@ static void dolog(int priority, const char *fmt, va_list ap)
#endif /* USE_SYSLOG */
}
+static void exit_slowly(int code)
+{
+ /* Be kind to init(8). */
+ sleep(10);
+ exit(code);
+}
+
static void log_err(const char *fmt, ...)
{
va_list ap;
@@ -2325,9 +2338,7 @@ static void log_err(const char *fmt, ...)
dolog(LOG_ERR, fmt, ap);
va_end(ap);
- /* Be kind to init(8). */
- sleep(10);
- exit(EXIT_FAILURE);
+ exit_slowly(EXIT_FAILURE);
}
static void log_warn(const char *fmt, ...)
--
2.11.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix a use of uninitialised memory in an agetty error path.
2017-11-17 16:44 [PATCH] Fix a use of uninitialised memory in an agetty error path Steven Smith
@ 2017-11-20 11:02 ` Karel Zak
2017-11-20 19:06 ` Steven Smith
0 siblings, 1 reply; 5+ messages in thread
From: Karel Zak @ 2017-11-20 11:02 UTC (permalink / raw)
To: Steven Smith; +Cc: util-linux, sos22
On Fri, Nov 17, 2017 at 08:44:33AM -0800, Steven Smith wrote:
> 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 <sos22@srcf.ucam.org>
> ---
> term-utils/agetty.c | 21 ++++++++++++++++-----
> 1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/term-utils/agetty.c b/term-utils/agetty.c
> index 9763fcd30..bc848a25a 100644
> --- a/term-utils/agetty.c
> +++ b/term-utils/agetty.c
> @@ -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
> @@ -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.
> default:
> log_err(_("%s: read: %m"), op->tty);
> }
> }
>
> + 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.
Karel
--
Karel Zak <kzak@redhat.com>
http://karelzak.blogspot.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix a use of uninitialised memory in an agetty error path.
2017-11-20 11:02 ` Karel Zak
@ 2017-11-20 19:06 ` Steven Smith
2017-11-21 10:23 ` Karel Zak
0 siblings, 1 reply; 5+ messages in thread
From: Steven Smith @ 2017-11-20 19:06 UTC (permalink / raw)
To: Karel Zak; +Cc: Steven Smith, util-linux
> > @@ -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.
>
> > default:
> > log_err(_("%s: read: %m"), op->tty);
> > }
> > }
> >
> > + 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:
@@ -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;
@@ -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;
break;
(Some side information: a couple of my test environments used to have problems
every couple of weeks because gettys went into infinite loops and contended
with the work the VMs were supposed to be doing. I didn't have debug symbols
then, but strace said that getty was repeatedly calling read(STDIN) and
getting back 0. I don't have a reliable way of reproducing the problem, but I
applied my first patch six months ago and I've not seen the misbehaviour
since then, so I'm reasonably confident it fixed the bug I was seeing.)
Thank you for the review.
Steven.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix a use of uninitialised memory in an agetty error path.
2017-11-20 19:06 ` Steven Smith
@ 2017-11-21 10:23 ` Karel Zak
2017-11-22 16:15 ` Steven Smith
0 siblings, 1 reply; 5+ messages in thread
From: Karel Zak @ 2017-11-21 10:23 UTC (permalink / raw)
To: Steven Smith; +Cc: util-linux
On Mon, Nov 20, 2017 at 02:06:23PM -0500, Steven Smith wrote:
> > > + 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:
>
> @@ -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;
> @@ -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:
- 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. IMHO
the current code is weak (independently on your read() issue:-)
The another weak thing is that agetty don't care if all speeds has
been already tested. The next_speed() increments baud_index again and
again... maybe we need another exit_slow() here too.
Karel
--
Karel Zak <kzak@redhat.com>
http://karelzak.blogspot.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix a use of uninitialised memory in an agetty error path.
2017-11-21 10:23 ` Karel Zak
@ 2017-11-22 16:15 ` Steven Smith
0 siblings, 0 replies; 5+ messages in thread
From: Steven Smith @ 2017-11-22 16:15 UTC (permalink / raw)
To: Karel Zak; +Cc: Steven Smith, util-linux, sos22
> > @@ -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;
> > @@ -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:
>
> - 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.
That's great, thank you.
Steven.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-11-22 16:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-17 16:44 [PATCH] Fix a use of uninitialised memory in an agetty error path Steven Smith
2017-11-20 11:02 ` Karel Zak
2017-11-20 19:06 ` Steven Smith
2017-11-21 10:23 ` Karel Zak
2017-11-22 16:15 ` Steven Smith
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.