[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config

[Alejandro Colomar]

Hi Paul,

On 3/12/23 02:44, Paul Eggert wrote:
> From fab3bcdcb3f38c7f6f5c326f4ceafb3ea54bba73 Mon Sep 17 00:00:00 2001
> From: Paul Eggert <eggert at 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 <eggert at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://alioth-lists.debian.net/pipermail/pkg-shadow-devel/attachments/20230312/a33b8cf7/attachment.sig>