On 02/02/2018 10:35 AM, Bruno Haible wrote:
Done:
Thanks. Some comments, with a proposed patch attached:
# define malloca(N) \
- ((N) < 4032 - sa_increment \
- ? (void *) ((char *) alloca ((N) + sa_increment) + sa_increment) \
+ ((N) < 4032 - (2 * sa_alignment_max - 1) \
+ ? (void *) (((uintptr_t) alloca ((N) + 2 * sa_alignment_max - 1) \
+ + (2 * sa_alignment_max - 1)) \
+ & ~(uintptr_t)(2 * sa_alignment_max - 1)) \
: mmalloca (N))
This can cause problems when -fcheck-pointer-bounds is in effect, since
converting a pointer to uintptr_t and back means that GCC won't connect
the resulting pointer to the original and this messes up bounds checking
on the result.
! /* Type for holding very small pointer differences. */
! typedef unsigned char small_t;
There should be a compile-time check guaranteeing that small_t is wide
enough.
! size_t nplus = n + sizeof (small_t) + 2 * sa_alignment_max - 1;
For expressions like these, it's a bit better to parenthesize the value
added to N, mostly because it makes it clearer to the reader that we're
just adding a constant. Also, on (admittedly-weird) platforms where
SIZE_MAX <= INT_MAX, it avoids undefined behavior in some
(admittedly-unusual) cases.
! void
freea (void *p)
{
! /* Determine whether p was a non-NULL pointer returned by mmalloca(). */
! if ((uintptr_t) p & sa_alignment_max)
This should be "((uintptr_t) p & (2 * sa_alignment_max - 1))", to make
it more likely that a runtime error is detected if a garbage pointer is
passed to freea.
From 381dccd214bae32992664a5bab432d166bdecd00 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Fri, 2 Feb 2018 14:07:19 -0800
Subject: [PATCH] malloca, xmalloca: port to -fcheck-pointer-bounds
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* lib/malloca.c (small_t): Verify that it is wide enough.
(mmalloca) [HAVE_ALLOCA]: Be a bit safer about size arithmetic.
Use malloca_alignoff to avoid issues with -fcheck-pointer-bounds.
(freea): Always use ‘free’ when the pointer’s low-order bits
(sa_alignment_max - 1) are nonzero. This is cheap and can catch
bad pointers earlier.
* lib/malloca.h (malloca_alignoff): New macro.
(malloca) [HAVE_ALLOCA]: Use it.
* lib/xmalloca.h (xmalloca) [HAVE_ALLOCA]: Use it.
* modules/malloca (Depends-on): Add verify.
---
ChangeLog | 14 ++++++++++++++
lib/malloca.c | 21 +++++++++++++--------
lib/malloca.h | 25 ++++++++++++++++++++++---
lib/xmalloca.h | 6 +++---
modules/malloca | 1 +
5 files changed, 53 insertions(+), 14 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 898d27592..30eec2148 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,17 @@
+2018-02-02 Paul Eggert <egg...@cs.ucla.edu>
+
+ malloca, xmalloca: port to -fcheck-pointer-bounds
+ * lib/malloca.c (small_t): Verify that it is wide enough.
+ (mmalloca) [HAVE_ALLOCA]: Be a bit safer about size arithmetic.
+ Use malloca_alignoff to avoid issues with -fcheck-pointer-bounds.
+ (freea): Always use ‘free’ when the pointer’s low-order bits
+ (sa_alignment_max - 1) are nonzero. This is cheap and can catch
+ bad pointers earlier.
+ * lib/malloca.h (malloca_alignoff): New macro.
+ (malloca) [HAVE_ALLOCA]: Use it.
+ * lib/xmalloca.h (xmalloca) [HAVE_ALLOCA]: Use it.
+ * modules/malloca (Depends-on): Add verify.
+
2018-02-02 Bruno Haible <br...@clisp.org>
malloca, xmalloca: Make multithread-safe.
diff --git a/lib/malloca.c b/lib/malloca.c
index 5741cba6f..f764add4d 100644
--- a/lib/malloca.c
+++ b/lib/malloca.c
@@ -21,6 +21,8 @@
/* Specification. */
#include "malloca.h"
+#include "verify.h"
+
/* The speed critical point in this file is freea() applied to an alloca()
result: it must be fast, to match the speed of alloca(). The speed of
mmalloca() and freea() in the other case are not critical, because they
@@ -34,14 +36,15 @@
/* Type for holding very small pointer differences. */
typedef unsigned char small_t;
+verify (2 * sa_alignment_max - 1 <= (small_t) -1);
void *
mmalloca (size_t n)
{
#if HAVE_ALLOCA
- /* Allocate one more word, used to determine the address to pass to freea(),
+ /* Allocate one more byte, used to determine the address to pass to freea(),
and room for the alignment ≡ sa_alignment_max mod 2*sa_alignment_max. */
- size_t nplus = n + sizeof (small_t) + 2 * sa_alignment_max - 1;
+ size_t nplus = n + (sizeof (small_t) + 2 * sa_alignment_max - 1);
if (nplus >= n)
{
@@ -49,10 +52,9 @@ mmalloca (size_t n)
if (mem != NULL)
{
- char *p =
- (char *)((((uintptr_t)mem + sizeof (small_t) + sa_alignment_max -
1)
- & ~(uintptr_t)(2 * sa_alignment_max - 1))
- + sa_alignment_max);
+ char *p = malloca_alignoff (mem + (sizeof (small_t)
+ + sa_alignment_max - 1),
+ 2 * sa_alignment_max, sa_alignment_max);
/* Here p >= mem + sizeof (small_t),
and p <= mem + sizeof (small_t) + 2 * sa_alignment_max - 1
hence p + n <= mem + nplus.
@@ -78,8 +80,11 @@ mmalloca (size_t n)
void
freea (void *p)
{
- /* Determine whether p was a non-NULL pointer returned by mmalloca(). */
- if ((uintptr_t) p & sa_alignment_max)
+ /* Determine whether P was a non-NULL pointer returned by mmalloca().
+ Although the mask sa_alignment_max would work just as well with
+ correct code, the mask (2 * sa_alignment_max - 1) can improve
+ error detection if P is a garbage pointer. */
+ if ((uintptr_t) p & (2 * sa_alignment_max - 1))
{
void *mem = (char *) p - ((small_t *) p)[-1];
free (mem);
diff --git a/lib/malloca.h b/lib/malloca.h
index cbc8fe7ab..c45705974 100644
--- a/lib/malloca.h
+++ b/lib/malloca.h
@@ -51,15 +51,34 @@ extern "C" {
# define safe_alloca(N) ((void) (N), NULL)
#endif
+/* Return the pointer P, first aligned down to the previous multiple
+ of the alignment A, and then with offset O added. A should be a
+ power of 2; O should be nonnegative and less than A. Ordinarily do
+ this simply by ANDing out unwanted bits from the uintptr_t
+ representation before adding O. But if __CHKP__ is defined, do not
+ convert back from uintptr_t, so that the compiler tracks that the
+ resulting pointer is derived from the original. Note that all
+ known __CHKP__ compilers support expression statements. */
+#ifdef __CHKP__
+# define malloca_alignoff(p, a, o) \
+ ({ char *malloca_p = (char *) (p); \
+ ptrdiff_t malloca_o = o; \
+ ptrdiff_t malloca_x = (uintptr_t) malloca_p & ((a) - 1); \
+ (void *) (malloca_p + (malloca_o - malloca_x)); })
+#else
+# define malloca_alignoff(p, a, o) \
+ ((void *) ((char *) (((uintptr_t) (p) + ((a) - 1)) & ~ ((a) - 1)) + (o)))
+# endif
+
/* malloca(N) is a safe variant of alloca(N). It allocates N bytes of
memory allocated on the stack, that must be freed using freea() before
the function returns. Upon failure, it returns NULL. */
#if HAVE_ALLOCA
# define malloca(N) \
((N) < 4032 - (2 * sa_alignment_max - 1) \
- ? (void *) (((uintptr_t) alloca ((N) + 2 * sa_alignment_max - 1) \
- + (2 * sa_alignment_max - 1)) \
- & ~(uintptr_t)(2 * sa_alignment_max - 1)) \
+ ? malloca_alignoff (((char *) alloca ((N) + (2 * sa_alignment_max - 1)) \
+ + (2 * sa_alignment_max - 1)), \
+ 2 * sa_alignment_max, 0) \
: mmalloca (N))
#else
# define malloca(N) \
diff --git a/lib/xmalloca.h b/lib/xmalloca.h
index 14fc1b9dc..a2d110db6 100644
--- a/lib/xmalloca.h
+++ b/lib/xmalloca.h
@@ -33,9 +33,9 @@ extern "C" {
#if HAVE_ALLOCA
# define xmalloca(N) \
((N) < 4032 - (2 * sa_alignment_max - 1) \
- ? (void *) (((uintptr_t) alloca ((N) + 2 * sa_alignment_max - 1) \
- + (2 * sa_alignment_max - 1)) \
- & ~(uintptr_t)(2 * sa_alignment_max - 1)) \
+ ? malloca_alignoff (((char *) alloca ((N) + (2 * sa_alignment_max - 1)) \
+ + (2 * sa_alignment_max - 1)), \
+ 2 * sa_alignment_max, 0) \
: xmmalloca (N))
extern void * xmmalloca (size_t n);
#else
diff --git a/modules/malloca b/modules/malloca
index 8f5ab644a..0ae3fe08d 100644
--- a/modules/malloca
+++ b/modules/malloca
@@ -11,6 +11,7 @@ m4/longlong.m4
Depends-on:
alloca-opt
stdint
+verify
xalloc-oversized
configure.ac:
--
2.14.3
