Commit Message
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
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.
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.
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.
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.
@@ -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__))
@@ -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