Hi Paul, On 3/12/23 02:44, Paul Eggert wrote: > From fab3bcdcb3f38c7f6f5c326f4ceafb3ea54bba73 Mon Sep 17 00:00:00 2001 > From: Paul Eggert <egg...@cs.ucla.edu> > Date: Sat, 11 Mar 2023 10:07:32 -0800 > Subject: [PATCH 7/8] Fix is_my_tty overruns and truncations
Is there any chance those can be fixed individually? The patch
is rather long, and complex to review. Maybe it's not possible
and all fixes depend on each other; if so, please confirm. But
if it's possible, I'd like to review it split.
>
> * libmisc/utmp.c (is_my_tty): Declare the parameter
> as a char array not char *, as it is not necessarily null-terminated.
> Avoid a read overrun when reading ut_utname. Do not silently truncate
> the string returned by ttyname; instead, do not cache an overlong
> ttyname, as the behavior is correct in this case (albeit slower).
> Use strnlen + strcpy instead of strlcpy to avoid polluting the
> cache with truncated data.
>
> Signed-off-by: Paul Eggert <egg...@cs.ucla.edu>
> ---
> libmisc/utmp.c | 42 ++++++++++++++++++++++--------------------
> 1 file changed, 22 insertions(+), 20 deletions(-)
>
> diff --git a/libmisc/utmp.c b/libmisc/utmp.c
> index ff6acee0..bf7e5675 100644
> --- a/libmisc/utmp.c
> +++ b/libmisc/utmp.c
> @@ -24,36 +24,38 @@
>
> #ident "$Id$"
>
> +enum { UT_LINE_LEN = sizeof (getutent ()->ut_line) };
Isn't UT_LINESIZE (from <utmp.h>, see utmp(5)) what you want?
alx@asus5775:~/src/linux/man-pages/man-pages/main$ grepc -tt -x. utmp man5
man5/utmp.5:72:
struct utmp {
short ut_type; /* Type of record */
pid_t ut_pid; /* PID of login process */
char ut_line[UT_LINESIZE]; /* Device name of tty \- "/dev/" */
char ut_id[4]; /* Terminal name suffix,
or inittab(5) ID */
char ut_user[UT_NAMESIZE]; /* Username */
char ut_host[UT_HOSTSIZE]; /* Hostname for remote login, or
kernel version for run\-level
messages */
struct exit_status ut_exit; /* Exit status of a process
marked as DEAD_PROCESS; not
used by Linux init(1) */
/* The ut_session and ut_tv fields must be the same size when
compiled 32\- and 64\-bit. This allows data files and shared
memory to be shared between 32\- and 64\-bit applications. */
#if __WORDSIZE == 64 && defined __WORDSIZE_COMPAT32
int32_t ut_session; /* Session ID (\fBgetsid\fP(2)),
used for windowing */
struct {
int32_t tv_sec; /* Seconds */
int32_t tv_usec; /* Microseconds */
} ut_tv; /* Time entry was made */
#else
long ut_session; /* Session ID */
struct timeval ut_tv; /* Time entry was made */
#endif
int32_t ut_addr_v6[4]; /* Internet address of remote
host; IPv4 address uses
just ut_addr_v6[0] */
char __unused[20]; /* Reserved for future use */
};
>
> /*
> * is_my_tty -- determine if "tty" is the same TTY stdin is using
> */
> -static bool is_my_tty (const char *tty)
> +static bool is_my_tty (const char tty[UT_LINE_LEN])
> {
> - /* full_tty shall be at least sizeof utmp.ut_line + 5 */
> - char full_tty[200];
> - /* tmptty shall be bigger than full_tty */
> - static char tmptty[sizeof (full_tty)+1];
> -
> - if ('/' != *tty) {
> - (void) snprintf (full_tty, sizeof full_tty, "/dev/%s", tty);
> - tty = &full_tty[0];
> - }
> + /* A null-terminated copy of tty, prefixed with "/dev/" if tty
> + is not absolute. There is no need for snprintf, as sprintf
> + cannot overrun. */
> + char full_tty[sizeof "/dev/" + UT_LINE_LEN];
> + (void) sprintf (full_tty, "%s%.*s", '/' == *tty ? "" : "/dev/",
> + UT_LINE_LEN, tty);
To be honest, I still prefer stpcpy(3). Avoiding one call is
probably not even an optimization, since sprintf(3) internals
will counter any gains, right?
I think the following reads simpler, and hopefully it's even
faster:
p = full_tty;
*p = '\0';
if (tty[0] == '/')
p = stpcpy(p, "/dev/");
strncat(p, tty, UT_LINESIZE);
After writing, since shadow isn't performance-critical, and
32 bytes will probably not take too much to catenate, I
realized we could just use strcpy(3) instead of stpcpy(3),
to simplify even more:
full_tty[0] = '\0';
if (tty[0] == '/')
strcpy(full_tty, "/dev/");
strncat(full_tty, tty, UT_LINESIZE);
>
> - if ('\0' == tmptty[0]) {
> - const char *tname = ttyname (STDIN_FILENO);
> - if (NULL != tname)
> - (void) strlcpy (tmptty, tname, sizeof tmptty);
> - }
> + /* Cache of ttyname, valid if tmptty[0]. */
> + static char tmptty[UT_LINE_LEN + 1];
>
> + const char *tname;
> if ('\0' == tmptty[0]) {
> - (void) puts (_("Unable to determine your tty name."));
> - exit (EXIT_FAILURE);
> - } else if (strncmp (tty, tmptty, sizeof (tmptty)) != 0) {
> - return false;
> + tname = ttyname (STDIN_FILENO);
ttyname(3) returns a string, according to the manual page.
> + if (! tname) {
> + (void) puts (_("Unable to determine your tty name."));
> + exit (EXIT_FAILURE);
> + }
> + if (strnlen (tname, sizeof tmptty) < sizeof tmptty) {
Which allows us using strlen(3) here.
Cheers,
Alex
> + strcpy (tmptty, tname);
> + }
> } else {
> - return true;
> + tname = tmptty;
> }
> +
> + return strcmp (full_tty, tname) == 0;
> }
>
> /*
> --
> 2.37.2
>
--
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
OpenPGP_signature
Description: OpenPGP digital signature
