master: Build failure in malloc with GCC 7

[Florian Weimer]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: malloc-assert.patch
Type: text/x-patch
Size: 1154 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20170712/38697b5d/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: malloc-assert-2.patch
Type: text/x-patch
Size: 824 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20170712/38697b5d/attachment-0001.bin>