Re: removing coreutils' final strncpy uses

[Erik Auerswald]

Hi,

On 07/15/2012 03:57 PM, Jim Meyering wrote:
> Remember the strncpy prohibition I added to "make syntax-check"
> not too long ago?  I exempted the uses in pinky.c and who.c
> because those programs don't really matter and their uses were
> not obviously bad.  Plus, I just didn't feel like it.
>
> Well, today I did, so removed them and, along the way, was surprised
> to see that while not officially wrong, they were unwarranted:  they
> would uselessly zero-pad out to the end of each destination buffer.

From reading the man pages of strncpy and stpncpy, stpncpy does the 
same. From the stpncpy man page on debian/unstable (date 2011-09-28, 
from Linux man-pages 3.40):

DESCRIPTION
 The stpncpy() function copies at most n characters from the string
 pointed to by src, including the terminating null byte ('\0'), to the
 array pointed to by dest. Exactly n characters are written at dest.
 If the length strlen(src) is smaller than n, the remaining characters
 in the array pointed to by dest are filled with null bytes ('\0'), If
 the length strlen(src) is greater or equal to n, the string pointed to
 by dest will not be null-terminated.

As written above the copied string may be unterminated after stpncpy as 
well as after strncpy. What exactly is won by using stpncpy over strncpy?

> diff --git a/src/pinky.c b/src/pinky.c
> index 597bc56..c01b124 100644
> --- a/src/pinky.c
> +++ b/src/pinky.c
> @@ -208,21 +208,14 @@ print_entry (const STRUCT_UTMP *utmp_ent)
>   #define DEV_DIR_LEN (sizeof (DEV_DIR_WITH_TRAILING_SLASH) - 1)
>
>     char line[sizeof (utmp_ent->ut_line) + DEV_DIR_LEN + 1];
> +  char *p = line;
>
>     /* Copy ut_line into LINE, prepending '/dev/' if ut_line is not
>        already an absolute file name.  Some system may put the full,
>        absolute file name in ut_line.  */
> -  if (utmp_ent->ut_line[0] == '/')
> -    {
> -      strncpy (line, utmp_ent->ut_line, sizeof (utmp_ent->ut_line));
> -      line[sizeof (utmp_ent->ut_line)] = '\0';
> -    }
> -  else
> -    {
> -      strcpy (line, DEV_DIR_WITH_TRAILING_SLASH);
> -      strncpy (line + DEV_DIR_LEN, utmp_ent->ut_line, sizeof 
> utmp_ent->ut_line);
> -      line[DEV_DIR_LEN + sizeof (utmp_ent->ut_line)] = '\0';
> -    }
> +  if ( ! IS_ABSOLUTE_FILE_NAME (utmp_ent->ut_line))
> +    p = stpcpy (p, DEV_DIR_WITH_TRAILING_SLASH);
> +  stpncpy (p, utmp_ent->ut_line, sizeof (utmp_ent->ut_line));

I don't see any guarantee that line is null-terminated.

>     if (stat (line,&stats) == 0)
>       {
> @@ -242,8 +235,7 @@ print_entry (const STRUCT_UTMP *utmp_ent)
>         struct passwd *pw;
>         char name[UT_USER_SIZE + 1];
>
> -      strncpy (name, UT_USER (utmp_ent), UT_USER_SIZE);
> -      name[UT_USER_SIZE] = '\0';
> +      stpncpy (name, UT_USER (utmp_ent), UT_USER_SIZE);
>         pw = getpwnam (name);
>         if (pw == NULL)
>           /* TRANSLATORS: Real name is unknown; at most 19 characters. */
> @@ -284,8 +276,7 @@ print_entry (const STRUCT_UTMP *utmp_ent)
>         char *display = NULL;
>
>         /* Copy the host name into UT_HOST, and ensure it's nul terminated. */
> -      strncpy (ut_host, utmp_ent->ut_host, (int) sizeof (utmp_ent->ut_host));
> -      ut_host[sizeof (utmp_ent->ut_host)] = '\0';
> +      stpncpy (ut_host, utmp_ent->ut_host, sizeof (utmp_ent->ut_host));

I don't see any guarantee that utmp_ent->ut_host is null-terminated.

> diff --git a/src/who.c b/src/who.c
> index c875b1d..3ad8004 100644
> --- a/src/who.c
> +++ b/src/who.c
> @@ -342,23 +342,15 @@ print_user (const STRUCT_UTMP *utmp_ent, time_t boottime)
>   #define DEV_DIR_LEN (sizeof (DEV_DIR_WITH_TRAILING_SLASH) - 1)
>
>     char line[sizeof (utmp_ent->ut_line) + DEV_DIR_LEN + 1];
> +  char *p = line;
>     PIDSTR_DECL_AND_INIT (pidstr, utmp_ent);
>
>     /* Copy ut_line into LINE, prepending '/dev/' if ut_line is not
>        already an absolute file name.  Some systems may put the full,
>        absolute file name in ut_line.  */
> -  if (utmp_ent->ut_line[0] == '/')
> -    {
> -      strncpy (line, utmp_ent->ut_line, sizeof (utmp_ent->ut_line));
> -      line[sizeof (utmp_ent->ut_line)] = '\0';
> -    }
> -  else
> -    {
> -      strcpy (line, DEV_DIR_WITH_TRAILING_SLASH);
> -      strncpy (line + DEV_DIR_LEN, utmp_ent->ut_line,
> -               sizeof (utmp_ent->ut_line));
> -      line[DEV_DIR_LEN + sizeof (utmp_ent->ut_line)] = '\0';
> -    }
> +  if ( ! IS_ABSOLUTE_FILE_NAME (utmp_ent->ut_line))
> +    p = stpcpy (p, DEV_DIR_WITH_TRAILING_SLASH);
> +  stpncpy (p, utmp_ent->ut_line, sizeof (utmp_ent->ut_line));

I don't see any guarantee that line is null-terminated.

> @@ -384,8 +376,7 @@ print_user (const STRUCT_UTMP *utmp_ent, time_t boottime)
>         char *display = NULL;
>
>         /* Copy the host name into UT_HOST, and ensure it's nul terminated. */
> -      strncpy (ut_host, utmp_ent->ut_host, sizeof (utmp_ent->ut_host));
> -      ut_host[sizeof (utmp_ent->ut_host)] = '\0';
> +      stpncpy (ut_host, utmp_ent->ut_host, sizeof (utmp_ent->ut_host));

I don't see any guarantee that utmp_ent->ut_host is null-terminated.

Disclaimer: I have read the man page found on my system only, I don't 
have experience using str[n]cpy. But I was curious about this change and 
don't see the improvement.

Thanks,
Erik