This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: master: Build failure in malloc with GCC 7


On 07/12/2017 12:07 PM, Florian Weimer wrote:
> On 07/12/2017 10:50 AM, Andreas Schwab wrote:
>> On Jul 12 2017, Florian Weimer <fweimer@redhat.com> wrote:
>>
>>> This is from a recent build in Fedora rawhide:
>>>
>>> malloc.c:1590:50: error: array subscript is above array bounds
>>> [-Werror=array-bounds]
>>>  #define fastbin(ar_ptr, idx) ((ar_ptr)->fastbinsY[idx])
>>>                               ~~~~~~~~~~~~~~~~~~~~^~~~~~
>>> malloc.c:3572:26: note: in expansion of macro 'fastbin'
>>>        mfastbinptr *fb = &fastbin (av, idx);
>>>                           ^~~~~~~
>>
>> I don't see that here (with r249772).
> 
> Thanks.  It turns out it only happens with -O3, so it's no surprise no
> one else has seen it yet.  It's also related to the thread cache
> changes.  The full error message is this:
> 
> malloc.c: In function ‘tcache_init.part.9’:
> malloc.c:3572:25: error: array subscript is above array bounds
> [-Werror=array-bounds]
>        mfastbinptr *fb = &fastbin (av, idx);
>                          ^
> malloc.c:1590:50: error: array subscript is above array bounds
> [-Werror=array-bounds]
>  #define fastbin(ar_ptr, idx) ((ar_ptr)->fastbinsY[idx])
>                               ~~~~~~~~~~~~~~~~~~~~^~~~~~
> malloc.c:3572:26: note: in expansion of macro ‘fastbin’
>        mfastbinptr *fb = &fastbin (av, idx);
>                           ^~~~~~~
> malloc.c:1590:50: error: array subscript is above array bounds
> [-Werror=array-bounds]
>  #define fastbin(ar_ptr, idx) ((ar_ptr)->fastbinsY[idx])
>                               ~~~~~~~~~~~~~~~~~~~~^~~~~~
> malloc.c:3572:26: note: in expansion of macro ‘fastbin’
>        mfastbinptr *fb = &fastbin (av, idx);
> 
> I guess we need to debug this and determine whether it's a real problem
> with the code, or a GCC bug.

_int_malloc is inlined into tcache_init, and the allocation size is
constant-propagated into it.  GCC does not realize that global_max_fast
is limited MAX_FAST_SIZE, so it compiles the true branch of the if
statement:

  if ((unsigned long) (nb) <= (unsigned long) (get_max_fast ()))
    {
      idx = fastbin_index (nb);
      mfastbinptr *fb = &fastbin (av, idx);
      mchunkptr pp = *fb;
      REMOVE_FB (fb, victim, pp);
      if (victim != 0)

under the assumption that nb == sizeof (tcache_perthread_struct) == 576,
which is larger than MAX_FAST_SIZE, so the fastbin access is compiled
into an OOB array subscript.  GCC does not proceed to eliminate this
code, even though it has undefined behavior and will never execute in
practice.

The attached patch adds an assert which reveals to the GCC optimizers
that the global_max_fast can never MAX_FAST_SIZE, so this particular
issue goes away.  However, there is a cost in terms of code size because
it affects many places in malloc.

The second patch is a bit of a hack.  I'm not sure if we want to go
there.  It only affects the copy of _int_malloc which is inlined into
tcache_init (due to the __builtin_constant_p) and eliminates the true
branch of the if statement with its broken code.

Thanks,
Florian
diff --git a/malloc/malloc.c b/malloc/malloc.c
index 54e406b..138e278 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -1658,6 +1658,9 @@ typedef struct malloc_chunk *mfastbinptr;
 #define arena_is_corrupt(A)	(((A)->flags & ARENA_CORRUPTION_BIT))
 #define set_arena_corrupt(A)	((A)->flags |= ARENA_CORRUPTION_BIT)
 
+/* Maximum size of memory handled in fastbins.  */
+static INTERNAL_SIZE_T global_max_fast;
+
 /*
    Set value of max_fast.
    Use impossibly small value if 0.
@@ -1668,8 +1671,13 @@ typedef struct malloc_chunk *mfastbinptr;
 #define set_max_fast(s) \
   global_max_fast = (((s) == 0)						      \
                      ? SMALLBIN_WIDTH : ((s + SIZE_SZ) & ~MALLOC_ALIGN_MASK))
-#define get_max_fast() global_max_fast
 
+static inline size_t
+get_max_fast (void)
+{
+  assert (global_max_fast <= MAX_FAST_SIZE);
+  return global_max_fast;
+}
 
 /*
    ----------- Internal state representation and initialization -----------
@@ -1797,9 +1805,6 @@ static struct malloc_par mp_ =
 #endif
 };
 
-/* Maximum size of memory handled in fastbins.  */
-static INTERNAL_SIZE_T global_max_fast;
-
 /*
    Initialize a malloc_state struct.
 
diff --git a/malloc/malloc.c b/malloc/malloc.c
index 54e406b..07f8fb6 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3566,6 +3566,14 @@ _int_malloc (mstate av, size_t bytes)
   while ((pp = catomic_compare_and_exchange_val_acq (fb, victim->fd, victim)) \
 	 != victim);					\
 
+  /* _int_malloc can be inlined to a caller with a constant size
+     argument.  In this case, the compiler will see an out-of-bounds
+     array access in the true branch of the if statement below if it
+     cannot show that global_max_fast cannot be larger than
+     MAX_FAST_SIZE.  The assert shows the compiler that this cannot
+     happen.  */
+  assert (!__builtin_constant_p (nb) || global_max_fast <= MAX_FAST_SIZE);
+
   if ((unsigned long) (nb) <= (unsigned long) (get_max_fast ()))
     {
       idx = fastbin_index (nb);

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]