[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PATCH] flexmember: port better to GCC + valgrind
|
From: |
Paul Eggert |
|
Subject: |
[PATCH] flexmember: port better to GCC + valgrind |
|
Date: |
Wed, 7 Sep 2016 02:03:00 -0700 |
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 77938ef..8391aab 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,37 @@
+2016-09-07 Paul Eggert <address@hidden>
+
+ 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 <address@hidden>
getprogname: port to Solaris 10
diff --git a/lib/flexmember.h b/lib/flexmember.h
new file mode 100644
index 0000000..00b084c
--- /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 <http://www.gnu.org/licenses/>.
+
+ 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 9b3f716..bd9e587 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 a0c00d3..f41c06f 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 331d23b..52461b1 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 a2b7c86..5b2ff9d 100644
--- a/lib/glob.c
+++ b/lib/glob.c
@@ -90,6 +90,8 @@
#include <fnmatch.h>
+#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 2db5109..3dada5d 100644
--- a/lib/idcache.c
+++ b/lib/idcache.c
@@ -27,6 +27,7 @@
#include <unistd.h>
+#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 81ab213..f86bd87 100644
--- a/lib/localename.c
+++ b/lib/localename.c
@@ -33,6 +33,8 @@
#include <locale.h>
#include <string.h>
+#include "flexmember.h"
+
#if HAVE_USELOCALE
/* Mac OS X 10.5 defines the locale_t type in <xlocale.h>. */
# 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 10a006c..8bfe988 100644
--- a/lib/time_rz.c
+++ b/lib/time_rz.c
@@ -32,6 +32,7 @@
#include <stdlib.h>
#include <string.h>
+#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 baa9ff8..155ae9b 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 <stddef.h>
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 5556f04..5107676 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 a5a49f9..ab30236 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 919d7fc..f32fe5f 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 603796e..193c7bd 100644
--- a/modules/localename
+++ b/modules/localename
@@ -9,6 +9,7 @@ m4/intlmacosx.m4
m4/lcmessage.m4
Depends-on:
+flexmember
strdup
lock
langinfo
--
2.7.4
- [PATCH] flexmember: port better to GCC + valgrind,
Paul Eggert <=