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

Reply via email to