From 4c32543a4f05706ca3a8c8d583a5fb35d0e58828 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Wed, 7 Sep 2016 02:01:42 -0700 Subject: [PATCH] flexmember: port better to GCC + valgrind MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit With a char[] flexible array member in a struct with nontrivial alignment, GCC-generated code can access past the end of the array, because GCC assumes there are padding bytes to get the struct aligned. So the common idiom of malloc (offsetof (struct s, m), n) does not properly allocate an n-byte trailing member, as malloc’s argument should be the next multiple of alignof (struct s). See GCC Bug#66661: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66661 Although C11 apparently permits this GCC optimization (i.e., there was a bug in Gnulib not in GCC), possibly this is a defect in C11. See the thread containing: https://gcc.gnu.org/ml/gcc-patches/2016-09/msg00317.html * lib/flexmember.h: New file. * lib/fnmatch.c, lib/fts.c, lib/glob.c, lib/idcache.c: * lib/localename.c, lib/time_rz.c: Include flexmember.h. * lib/fnmatch_loop.c (struct patternlist): * lib/localename.c (struct hash_node): Use FLEXIBLE_ARRAY_MEMBER. * lib/fnmatch_loop.c (EXT): * lib/fts.c (fts_alloc): * lib/glob.c (glob_in_dir): * lib/idcache.c (getuser, getuidbyname, getgroup, getgidbyname): * lib/localename.c (gl_lock_define_initialized): * lib/time_rz.c (tzalloc): Use FLEXSIZEOF instead of offsetof. * m4/flexmember.m4 (AC_C_FLEXIBLE_ARRAY_MEMBER): Check that the size of the struct can be taken. * modules/flexmember (Files): Add lib/flexmember.h. * modules/fnmatch, modules/glob, modules/localename (Depends-on): Add flexmember. --- ChangeLog | 34 ++++++++++++++++++++++++++++++++++ lib/flexmember.h | 39 +++++++++++++++++++++++++++++++++++++++ lib/fnmatch.c | 2 ++ lib/fnmatch_loop.c | 4 ++-- lib/fts.c | 13 ++----------- lib/glob.c | 5 ++++- lib/idcache.c | 9 +++++---- lib/localename.c | 6 ++++-- lib/time_rz.c | 3 ++- m4/flexmember.m4 | 8 +++++--- modules/flexmember | 1 + modules/fnmatch | 1 + modules/glob | 1 + modules/localename | 1 + 14 files changed, 103 insertions(+), 24 deletions(-) create mode 100644 lib/flexmember.h diff --git a/ChangeLog b/ChangeLog index 77938ef938..8391aab9bd 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,37 @@ +2016-09-07 Paul Eggert + + flexmember: port better to GCC + valgrind + With a char[] flexible array member in a struct with nontrivial + alignment, GCC-generated code can access past the end of the + array, because GCC assumes there are padding bytes to get the + struct aligned. So the common idiom of malloc (offsetof (struct + s, m), n) does not properly allocate an n-byte trailing member, as + malloc’s argument should be the next multiple of alignof (struct s). + See GCC Bug#66661: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66661 + Although C11 apparently permits this GCC optimization (i.e., there + was a bug in Gnulib not in GCC), possibly this is a defect in C11. + See the thread containing: + https://gcc.gnu.org/ml/gcc-patches/2016-09/msg00317.html + * lib/flexmember.h: New file. + * lib/fnmatch.c, lib/fts.c, lib/glob.c, lib/idcache.c: + * lib/localename.c, lib/time_rz.c: + Include flexmember.h. + * lib/fnmatch_loop.c (struct patternlist): + * lib/localename.c (struct hash_node): + Use FLEXIBLE_ARRAY_MEMBER. + * lib/fnmatch_loop.c (EXT): + * lib/fts.c (fts_alloc): + * lib/glob.c (glob_in_dir): + * lib/idcache.c (getuser, getuidbyname, getgroup, getgidbyname): + * lib/localename.c (gl_lock_define_initialized): + * lib/time_rz.c (tzalloc): + Use FLEXSIZEOF instead of offsetof. + * m4/flexmember.m4 (AC_C_FLEXIBLE_ARRAY_MEMBER): + Check that the size of the struct can be taken. + * modules/flexmember (Files): Add lib/flexmember.h. + * modules/fnmatch, modules/glob, modules/localename (Depends-on): + Add flexmember. + 2016-09-06 Paul Eggert getprogname: port to Solaris 10 diff --git a/lib/flexmember.h b/lib/flexmember.h new file mode 100644 index 0000000000..00b084c985 --- /dev/null +++ b/lib/flexmember.h @@ -0,0 +1,39 @@ +/* Sizes of structs with flexible array members. + + Copyright 2016 Free Software Foundation, Inc. + + This program is free software: you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . + + Written by Paul Eggert. */ + +/* Nonzero multiple of alignment of TYPE, suitable for FLEXSIZEOF below. + On older platforms without _Alignof, use a pessimistic bound that is + safe in practice even if FLEXIBLE_ARRAY_MEMBER is 1. + On newer platforms, use _Alignof to get a tighter bound. */ + +#if !defined __STDC_VERSION__ || __STDC_VERSION__ < 201112 +# define _GL_XALLOC_ALIGNOF(type) (sizeof (type) & ~ (sizeof (type) - 1)) +#else +# define _GL_XALLOC_ALIGNOF(type) _Alignof (type) +#endif + +/* Upper bound on the size of a struct of type TYPE with a flexible + array member named MEMBER that is followed by N bytes of other data. + This is not simply sizeof (TYPE) + N, since it may require + alignment and FLEXIBLE_ARRAY_MEMBER may be 1. Yield a value less + than N if and only if arithmetic overflow occurs. */ + +#define FLEXSIZEOF(type, member, n) \ + ((offsetof (type, member) + _GL_XALLOC_ALIGNOF (type) - 1 + (n)) \ + & ~ (_GL_XALLOC_ALIGNOF (type) - 1)) diff --git a/lib/fnmatch.c b/lib/fnmatch.c index 9b3f716103..bd9e587bac 100644 --- a/lib/fnmatch.c +++ b/lib/fnmatch.c @@ -67,6 +67,8 @@ extern int fnmatch (const char *pattern, const char *string, int flags); # define SIZE_MAX ((size_t) -1) #endif +#include "flexmember.h" + /* We often have to test for FNM_FILE_NAME and FNM_PERIOD being both set. */ #define NO_LEADING_PERIOD(flags) \ ((flags & (FNM_FILE_NAME | FNM_PERIOD)) == (FNM_FILE_NAME | FNM_PERIOD)) diff --git a/lib/fnmatch_loop.c b/lib/fnmatch_loop.c index a0c00d3456..f41c06f89a 100644 --- a/lib/fnmatch_loop.c +++ b/lib/fnmatch_loop.c @@ -1031,7 +1031,7 @@ EXT (INT opt, const CHAR *pattern, const CHAR *string, const CHAR *string_end, struct patternlist { struct patternlist *next; - CHAR str[1]; + CHAR str[FLEXIBLE_ARRAY_MEMBER]; } *list = NULL; struct patternlist **lastp = &list; size_t pattern_len = STRLEN (pattern); @@ -1083,7 +1083,7 @@ EXT (INT opt, const CHAR *pattern, const CHAR *string, const CHAR *string_end, ? pattern_len \ : p - startp + 1UL); \ plensize = plen * sizeof (CHAR); \ - newpsize = offsetof (struct patternlist, str) + plensize; \ + newpsize = FLEXSIZEOF (struct patternlist, str, plensize); \ if ((size_t) -1 / sizeof (CHAR) < plen \ || newpsize < offsetof (struct patternlist, str) \ || ALLOCA_LIMIT <= newpsize) \ diff --git a/lib/fts.c b/lib/fts.c index 331d23bf88..52461b18c5 100644 --- a/lib/fts.c +++ b/lib/fts.c @@ -76,6 +76,7 @@ static char sccsid[] = "@(#)fts.c 8.6 (Berkeley) 8/14/94"; /* FIXME - use fcntl(F_DUPFD_CLOEXEC)/openat(O_CLOEXEC) once they are supported. */ # include "cloexec.h" +# include "flexmember.h" # include "openat.h" # include "same-inode.h" #endif @@ -1925,17 +1926,7 @@ fts_alloc (FTS *sp, const char *name, register size_t namelen) * The file name is a variable length array. Allocate the FTSENT * structure and the file name in one chunk. */ - len = offsetof(FTSENT, fts_name) + namelen + 1; - /* Align the allocation size so that it works for FTSENT, - so that trailing padding may be referenced by direct access - to the flexible array members, without triggering undefined behavior - by accessing bytes beyond the heap allocation. This implicit access - was seen for example with ISDOT() and GCC 5.1.1 at -O2. - Do not use alignof (FTSENT) here, since C11 prohibits - taking the alignment of a structure containing a flexible - array member. */ - len += alignof (max_align_t) - 1; - len &= ~ (alignof (max_align_t) - 1); + len = FLEXSIZEOF(FTSENT, fts_name, namelen + 1); if ((p = malloc(len)) == NULL) return (NULL); diff --git a/lib/glob.c b/lib/glob.c index a2b7c86c3a..5b2ff9d623 100644 --- a/lib/glob.c +++ b/lib/glob.c @@ -90,6 +90,8 @@ #include +#include "flexmember.h" + #ifdef _SC_GETPW_R_SIZE_MAX # define GETPW_R_SIZE_MAX() sysconf (_SC_GETPW_R_SIZE_MAX) #else @@ -1677,7 +1679,8 @@ glob_in_dir (const char *pattern, const char *directory, int flags, struct globnames *newnames; size_t count = names->count * 2; size_t nameoff = offsetof (struct globnames, name); - size_t size = nameoff + count * sizeof (char *); + size_t size = FLEXSIZEOF (struct globnames, name, + count * sizeof (char *)); if ((SIZE_MAX - nameoff) / 2 / sizeof (char *) < names->count) goto memory_error; diff --git a/lib/idcache.c b/lib/idcache.c index 2db5109b4c..3dada5d21a 100644 --- a/lib/idcache.c +++ b/lib/idcache.c @@ -27,6 +27,7 @@ #include +#include "flexmember.h" #include "xalloc.h" #ifdef __DJGPP__ @@ -83,7 +84,7 @@ getuser (uid_t uid) { struct passwd *pwent = getpwuid (uid); char const *name = pwent ? pwent->pw_name : ""; - match = xmalloc (offsetof (struct userid, name) + strlen (name) + 1); + match = xmalloc (FLEXSIZEOF (struct userid, name, strlen (name) + 1)); match->id.u = uid; strcpy (match->name, name); @@ -127,7 +128,7 @@ getuidbyname (const char *user) } #endif - tail = xmalloc (offsetof (struct userid, name) + strlen (user) + 1); + tail = xmalloc (FLEXSIZEOF (struct userid, name, strlen (user) + 1)); strcpy (tail->name, user); /* Add to the head of the list, so most recently used is first. */ @@ -165,7 +166,7 @@ getgroup (gid_t gid) { struct group *grent = getgrgid (gid); char const *name = grent ? grent->gr_name : ""; - match = xmalloc (offsetof (struct userid, name) + strlen (name) + 1); + match = xmalloc (FLEXSIZEOF (struct userid, name, strlen (name) + 1)); match->id.g = gid; strcpy (match->name, name); @@ -209,7 +210,7 @@ getgidbyname (const char *group) } #endif - tail = xmalloc (offsetof (struct userid, name) + strlen (group) + 1); + tail = xmalloc (FLEXSIZEOF (struct userid, name, strlen (group) + 1)); strcpy (tail->name, group); /* Add to the head of the list, so most recently used is first. */ diff --git a/lib/localename.c b/lib/localename.c index 81ab213043..f86bd874d0 100644 --- a/lib/localename.c +++ b/lib/localename.c @@ -33,6 +33,8 @@ #include #include +#include "flexmember.h" + #if HAVE_USELOCALE /* Mac OS X 10.5 defines the locale_t type in . */ # if defined __APPLE__ && defined __MACH__ @@ -2619,7 +2621,7 @@ string_hash (const void *x) struct hash_node { struct hash_node * volatile next; - char contents[100]; /* has variable size */ + char contents[FLEXIBLE_ARRAY_MEMBER]; }; # define HASH_TABLE_SIZE 257 @@ -2646,7 +2648,7 @@ struniq (const char *string) size = strlen (string) + 1; new_node = (struct hash_node *) - malloc (offsetof (struct hash_node, contents[0]) + size); + malloc (FLEXSIZEOF (struct hash_node, contents, size)); if (new_node == NULL) /* Out of memory. Return a statically allocated string. */ return "C"; diff --git a/lib/time_rz.c b/lib/time_rz.c index 10a006ca2e..8bfe988479 100644 --- a/lib/time_rz.c +++ b/lib/time_rz.c @@ -32,6 +32,7 @@ #include #include +#include "flexmember.h" #include "time-internal.h" #if !HAVE_TZSET @@ -94,7 +95,7 @@ tzalloc (char const *name) { size_t name_size = name ? strlen (name) + 1 : 0; size_t abbr_size = name_size < ABBR_SIZE_MIN ? ABBR_SIZE_MIN : name_size + 1; - timezone_t tz = malloc (offsetof (struct tm_zone, abbrs) + abbr_size); + timezone_t tz = malloc (FLEXSIZEOF (struct tm_zone, abbrs, abbr_size)); if (tz) { tz->next = NULL; diff --git a/m4/flexmember.m4 b/m4/flexmember.m4 index baa9ff8fb2..155ae9b812 100644 --- a/m4/flexmember.m4 +++ b/m4/flexmember.m4 @@ -1,4 +1,4 @@ -# serial 3 +# serial 4 # Check for flexible array member support. # Copyright (C) 2006, 2009-2016 Free Software Foundation, Inc. @@ -19,8 +19,10 @@ AC_DEFUN([AC_C_FLEXIBLE_ARRAY_MEMBER], #include struct s { int n; double d[]; };]], [[int m = getchar (); - struct s *p = malloc (offsetof (struct s, d) - + m * sizeof (double)); + size_t nbytes = offsetof (struct s, d) + m * sizeof (double); + nbytes += sizeof (struct s) - 1; + nbytes -= nbytes % sizeof (struct s); + struct s *p = malloc (nbytes); p->d[0] = 0.0; return p->d != (double *) NULL;]])], [ac_cv_c_flexmember=yes], diff --git a/modules/flexmember b/modules/flexmember index 5556f041d8..510767619d 100644 --- a/modules/flexmember +++ b/modules/flexmember @@ -2,6 +2,7 @@ Description: Flexible array member support Files: +lib/flexmember.h m4/flexmember.m4 Depends-on: diff --git a/modules/fnmatch b/modules/fnmatch index a5a49f9d92..ab302361ba 100644 --- a/modules/fnmatch +++ b/modules/fnmatch @@ -12,6 +12,7 @@ Depends-on: extensions snippet/arg-nonnull alloca [test -n "$FNMATCH_H"] +flexmember [test -n "$FNMATCH_H"] stdbool [test -n "$FNMATCH_H"] wchar [test -n "$FNMATCH_H"] wctype-h [test -n "$FNMATCH_H"] diff --git a/modules/glob b/modules/glob index 919d7fc0f5..f32fe5f36f 100644 --- a/modules/glob +++ b/modules/glob @@ -18,6 +18,7 @@ alloca [test -n "$GLOB_H"] closedir [test -n "$GLOB_H"] d-type [test -n "$GLOB_H"] dirfd [test -n "$GLOB_H"] +flexmember [test -n "$GLOB_H"] fnmatch [test -n "$GLOB_H"] getlogin_r [test -n "$GLOB_H"] memchr [test -n "$GLOB_H"] diff --git a/modules/localename b/modules/localename index 603796e067..193c7bd754 100644 --- a/modules/localename +++ b/modules/localename @@ -9,6 +9,7 @@ m4/intlmacosx.m4 m4/lcmessage.m4 Depends-on: +flexmember strdup lock langinfo -- 2.39.5