diff mbox

master: Build failure in malloc with GCC 7

Message ID a0e2feeb-4f0e-29f5-f0e2-c44f848ac3a5@redhat.com
State New
Headers show

Commit Message

Florian Weimer July 12, 2017, 10:57 a.m. UTC 
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

Comments

Andreas Schwab July 12, 2017, 12:29 p.m. UTC | #1 
On Jul 12 2017, Florian Weimer <fweimer@redhat.com> wrote:

> 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.

Can __builtin_unreachable avoid the runtime overhead?

Andreas.
diff mbox

Patch

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);