[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: removing coreutils' final strncpy uses
From: |
Erik Auerswald |
Subject: |
Re: removing coreutils' final strncpy uses |
Date: |
Sun, 15 Jul 2012 17:40:19 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux i686; rv:10.0.5) Gecko/20120624 Icedove/10.0.5 |
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