add attribute nonstring

Message ID b8fa0da8-49a4-1f6f-9d8d-204e5924aa52@gmail.com
State New, archived
Headers

Commit Message

Martin Sebor Nov. 15, 2017, 11:05 p.m. UTC
  On 11/15/2017 03:09 PM, Steve Ellcey wrote:
> On Fri, 2017-11-10 at 16:11 -0700, Martin Sebor wrote:
>>
>> Okay, thanks.  I've read that you run periodic Glibc builds with
>> the latest GCC so I wanted to give you a heads up on this change
>> and the patch to avoid breaking them.  Unless that's not a concern
>> (do you use -Werror in those builds?) what is the best way to
>> coordinate this update?
>>
>> I CC Paul for his input in case updating tzcode first is important.
>>
>> Martin
>
> Martin, were you going to update this patch with Joseph's comments and
> resubmit?  The tz changes are checked in and the ifreq struct is taken
> care of so all that remains are the changes to cdefs.h to define
> __NONSTRING and then to use in utmp.h.  I don't have any comments on
> that change beyond what Joseph made but I did do a build and run on
> aarch64 with the latest GCC to verify that those changes work for me.

Sure.  I've got the attached patch.  According to the Linux utmp
man page:
        String fields are terminated by a null byte ('\0')
        if they are shorter than the size of the field.

The patch lets Glibc compile fine with the top of GCC 8 trunk.

As a heads up, the new attribute triggers warnings with my patch
that enhances GCC to check for string functions being passed
arrays declared with the attribute.  Some are false positives
that I'll try to handle in GCC but at least one of these looks
like a Glibc bug.  Assuming the GCC patch gets approved there
may be some more fallout for Glibc to deal with.

../sysdeps/unix/getlogin_r.c:83:23: warning: ‘strlen’ argument 1 
declared attribute ‘nonstring’ [-Wstringop-overflow=]
        size_t needed = strlen (ut->ut_user) + 1;
                        ^~~~~~~~~~~~~~~~~~~~
In file included from ../login/utmp.h:29,
                  from ../include/utmp.h:2,
                  from ../sysdeps/unix/getlogin_r.c:26,
                  from ../sysdeps/unix/sysv/linux/getlogin_r.c:25:
../sysdeps/gnu/bits/utmp.h:65:8: note: argument ‘ut_user’ declared here
    char ut_user[UT_NAMESIZE]
         ^~~~~~~

Martin
  

Comments

Joseph Myers Nov. 15, 2017, 11:14 p.m. UTC | #1
On Wed, 15 Nov 2017, Martin Sebor wrote:

> Sure.  I've got the attached patch.  According to the Linux utmp
> man page:
>        String fields are terminated by a null byte ('\0')
>        if they are shorter than the size of the field.
> 
> The patch lets Glibc compile fine with the top of GCC 8 trunk.

This patch is OK, with the corresponding change also to 
sysdeps/unix/sysv/linux/s390/bits/utmp.h unless you have reason to believe 
that version of bits/utmp.h doesn't need changing.
  
Paul Eggert Nov. 21, 2017, 5:27 a.m. UTC | #2
Joseph Myers wrote:
> This patch is OK, with the corresponding change also to
> sysdeps/unix/sysv/linux/s390/bits/utmp.h unless you have reason to believe
> that version of bits/utmp.h doesn't need changing

Are similar changes also needed to bits/utmp.h and to 
sysdeps/unix/sysv/linux/s390/s390-32/utmp32.h? They also define these arrays.

Also, manual/users.texi should document that the relevant arrays are not 
necessarily null-terminated.

Also, what about struct utmpx? It has similarly-named fields, which also appear 
to use strncpy format. This affects sysdeps/unix/sysv/linux/s390/bits/utmpx.h, 
sysdeps/unix/sysv/linux/s390/s390-32/utmpx32.h, sysdeps/gnu/bits/utmpx.h, and 
manual/users.texi.
  
Joseph Myers Nov. 21, 2017, 1:38 p.m. UTC | #3
On Tue, 21 Nov 2017, Paul Eggert wrote:

> Joseph Myers wrote:
> > This patch is OK, with the corresponding change also to
> > sysdeps/unix/sysv/linux/s390/bits/utmp.h unless you have reason to believe
> > that version of bits/utmp.h doesn't need changing
> 
> Are similar changes also needed to bits/utmp.h and to
> sysdeps/unix/sysv/linux/s390/s390-32/utmp32.h? They also define these arrays.

bits/utmp.h is not used by any current glibc configuration, though it 
could reasonably be changed.  
sysdeps/unix/sysv/linux/s390/s390-32/utmp32.h is a purely internal header, 
not an installed one, only relevant for converting between different utmp 
formats.

> Also, what about struct utmpx? It has similarly-named fields, which also
> appear to use strncpy format. This affects

I don't see anything clear in the POSIX specification of utmpx.h about 
whether these fields are NUL-terminated.  In any case, that structure was 
not an observed cause of build failures.
  

Patch

The -Wstringop-truncation option new in GCC 8 detects common misuses
of the strncat and strncpy function that may result in truncating
the copied string before the terminating NUL.  To avoid false positive
warnings for correct code that intentionally creates sequences of
characters that aren't guaranteed to be NUL-terminated, arrays that
are intended to store such sequences should be decorated with a new
nonstring attribute.  This change add this attribute to Glibc and
uses it to suppress such false positives.

ChangeLog:
	* misc/sys/cdefs.h (__attribute_nonstring__): New macro.
	* sysdeps/gnu/bits/utmp.h (struct utmp): Use it.

diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
index cfd39d5..a603cb9 100644
--- a/misc/sys/cdefs.h
+++ b/misc/sys/cdefs.h
@@ -407,6 +407,15 @@ 
 # endif
 #endif
 
+#if __GNUC_PREREQ (8, 0)
+/* Describes a char array whose address can safely be passed as the first
+   argument to strncpy and strncat, as the char array is not necessarily
+   a NUL-terminated string.  */
+# define __attribute_nonstring__ __attribute__ ((__nonstring__))
+#else
+# define __attribute_nonstring__
+#endif
+
 #if (!defined _Static_assert && !defined __cplusplus \
      && (defined __STDC_VERSION__ ? __STDC_VERSION__ : 0) < 201112 \
      && (!__GNUC_PREREQ (4, 6) || defined __STRICT_ANSI__))
diff --git a/sysdeps/gnu/bits/utmp.h b/sysdeps/gnu/bits/utmp.h
index 2ee11cb..71c9fa2 100644
--- a/sysdeps/gnu/bits/utmp.h
+++ b/sysdeps/gnu/bits/utmp.h
@@ -59,10 +59,13 @@  struct utmp
 {
   short int ut_type;           /* Type of login.  */
   pid_t ut_pid;                        /* Process ID of login process.  */
-  char ut_line[UT_LINESIZE];   /* Devicename.  */
+  char ut_line[UT_LINESIZE]
+    __attribute_nonstring__;   /* Devicename.  */
   char ut_id[4];               /* Inittab ID.  */
-  char ut_user[UT_NAMESIZE];   /* Username.  */
-  char ut_host[UT_HOSTSIZE];   /* Hostname for remote login.  */
+  char ut_user[UT_NAMESIZE]
+    __attribute_nonstring__;   /* Username.  */
+  char ut_host[UT_HOSTSIZE]
+    __attribute_nonstring__;   /* Hostname for remote login.  */
   struct exit_status ut_exit;  /* Exit status of a process marked
                                   as DEAD_PROCESS.  */
 /* The ut_session and ut_tv fields must be the same size when compiled