Re: removing coreutils' final strncpy uses

[Jim Meyering]

Erik Auerswald wrote:
> 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?
...
>
> 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.

I knew I shouldn't have pushed that without review.

Introducing bugs is definitely not an improvement.
Thanks for spotting that.

Here's a proposed fix:

>From 8de642149caac9e9114eb3ad3e3d71d151f691a3 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sun, 15 Jul 2012 18:18:03 +0200
Subject: [PATCH] pinky,who: fix bug in latest change

* src/system.h (stzncpy): New function.
* src/pinky.c (print_entry): Use stzncpy, not stpncpy.
The latter does not NUL-terminate.  I assumed that strncpy was
the only function with such a horrible API.  Today I learned that
stpncpy also may not NUL-terminate its result.
* src/who.c (print_user): Likewise.
Thanks to Erik Auerswald for spotting my error.
---
 src/pinky.c  |  6 +++---
 src/system.h | 14 ++++++++++++++
 src/who.c    |  4 ++--
 3 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/src/pinky.c b/src/pinky.c
index c01b124..385949a 100644
--- a/src/pinky.c
+++ b/src/pinky.c
@@ -215,7 +215,7 @@ print_entry (const STRUCT_UTMP *utmp_ent)
      absolute file name in ut_line.  */
   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));
+  stzncpy (p, utmp_ent->ut_line, sizeof (utmp_ent->ut_line));

   if (stat (line, &stats) == 0)
     {
@@ -235,7 +235,7 @@ print_entry (const STRUCT_UTMP *utmp_ent)
       struct passwd *pw;
       char name[UT_USER_SIZE + 1];

-      stpncpy (name, UT_USER (utmp_ent), UT_USER_SIZE);
+      stzncpy (name, UT_USER (utmp_ent), UT_USER_SIZE);
       pw = getpwnam (name);
       if (pw == NULL)
         /* TRANSLATORS: Real name is unknown; at most 19 characters. */
@@ -276,7 +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. */
-      stpncpy (ut_host, utmp_ent->ut_host, sizeof (utmp_ent->ut_host));
+      stzncpy (ut_host, utmp_ent->ut_host, sizeof (utmp_ent->ut_host));

       /* Look for an X display.  */
       display = strchr (ut_host, ':');
diff --git a/src/system.h b/src/system.h
index 5e3b3cb..6907603 100644
--- a/src/system.h
+++ b/src/system.h
@@ -626,6 +626,20 @@ The following directory is part of the cycle:\n  %s\n"), \
     }                                  \
   while (0)

+/* Like stpncpy, but do ensure that the result is NUL-terminated,
+   and do not NUL-pad out to LEN.  I.e., when strnlen (src, len) == len,
+   this function writes a NUL byte into dest[len].  Thus, the destination
+   buffer must be at least LEN+1 bytes long.  */
+static inline char *
+stzncpy (char *dest, char const *src, size_t len)
+{
+  char const *src_end = src + len;
+  while (src < src_end && *src)
+    *dest++ = *src++;
+  *dest = 0;
+  return dest;
+}
+
 #ifndef ARRAY_CARDINALITY
 # define ARRAY_CARDINALITY(Array) (sizeof (Array) / sizeof *(Array))
 #endif
diff --git a/src/who.c b/src/who.c
index 3ad8004..00a6e37 100644
--- a/src/who.c
+++ b/src/who.c
@@ -350,7 +350,7 @@ print_user (const STRUCT_UTMP *utmp_ent, time_t boottime)
      absolute file name in ut_line.  */
   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));
+  stzncpy (p, utmp_ent->ut_line, sizeof (utmp_ent->ut_line));

   if (stat (line, &stats) == 0)
     {
@@ -376,7 +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. */
-      stpncpy (ut_host, utmp_ent->ut_host, sizeof (utmp_ent->ut_host));
+      stzncpy (ut_host, utmp_ent->ut_host, sizeof (utmp_ent->ut_host));

       /* Look for an X display.  */
       display = strchr (ut_host, ':');
--
1.7.11.2.194.g7bdb748