[Bug tree-optimization/105078] New: Maybe wrong *** buffer overflow detected ***: terminated with -D_FORTIFY_SOURCE

[Bug tree-optimization/105078] New: Maybe wrong *** buffer overflow detected ***: terminated with -D_FORTIFY_SOURCE

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105078

            Bug ID: 105078
           Summary: Maybe wrong *** buffer overflow detected ***:
                    terminated with -D_FORTIFY_SOURCE
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: marxin at gcc dot gnu.org
                CC: fabian@ritter-vogt.de, siddhesh at gcc dot gnu.org
  Target Milestone: ---

It's what was slightly mentioned here:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104964#c6

Thanks to Fabian for the reduced test-case:

$ cat qt.C
#include <cstdlib> 
#include <cstddef> 
#include <unistd.h> 

struct QArrayData { 
        int size; 
        ptrdiff_t offset; // in bytes from beginning of header 

        void *data() { 
                return reinterpret_cast<char *>(this) + offset; 
        } 

        static QArrayData *allocate(size_t size, size_t alignment) { 
                size_t headerSize = sizeof(QArrayData); 
                headerSize += (alignment - alignof(QArrayData)); 
                QArrayData *header = static_cast<QArrayData
*>(::malloc(headerSize + size)); 
                header->size = size; 
                header->offset = headerSize; 
                return header; 
        } 
}; 

template <class T> 
struct QTypedArrayData : QArrayData { 
        T *data() { return static_cast<T *>(QArrayData::data()); } 
        class AlignmentDummy { QArrayData header; T data; }; 

        static QTypedArrayData *allocate(size_t size) { 
                return static_cast<QTypedArrayData
*>(QArrayData::allocate(size, alignof(AlignmentDummy))); 
        } 
}; 

int main() 
{ 
        int size = 256; 
        auto *data = QTypedArrayData<char>::allocate(size); 
        return readlink("asdf", data->data(), data->size - 1); 
}

$ g++ qt.C -D_FORTIFY_SOURCE=2 -O2 && ./a.out
In file included from /usr/include/features.h:490,
                 from
/home/marxin/bin/gcc/include/c++/12.0.1/x86_64-pc-linux-gnu/bits/os_defines.h:39,
                 from
/home/marxin/bin/gcc/include/c++/12.0.1/x86_64-pc-linux-gnu/bits/c++config.h:655,
                 from /home/marxin/bin/gcc/include/c++/12.0.1/cstdlib:41,
                 from qt.C:1:
In function ‘ssize_t readlink(const char*, char*, size_t)’,
    inlined from ‘int main()’ at qt.C:37:24:
/usr/include/bits/unistd.h:119:10: warning: ‘ssize_t __readlink_alias(const
char*, char*, size_t)’ writing 255 bytes into a region of size 0 overflows the
destination [-Wstringop-overflow=]
  119 |   return __glibc_fortify (readlink, __len, sizeof (char),
      |          ^~~~~~~~~~~~~~~
qt.C: In function ‘int main()’:
qt.C:24:8: note: at offset 16 into destination object
‘QTypedArrayData<char>::<anonymous>’ of size 16
   24 | struct QTypedArrayData : QArrayData {
      |        ^~~~~~~~~~~~~~~
/usr/include/bits/unistd.h:104:16: note: in a call to function ‘ssize_t
__readlink_alias(const char*, char*, size_t)’ declared with attribute ‘access
(write_only, 2, 3)’
  104 | extern ssize_t __REDIRECT_NTH (__readlink_alias,
      |                ^~~~~~~~~~~~~~
In function ‘ssize_t readlink(const char*, char*, size_t)’,
    inlined from ‘int main()’ at qt.C:37:24:
/usr/include/bits/unistd.h:119:10: warning: ‘ssize_t __readlink_chk(const
char*, char*, size_t, size_t)’ writing 255 bytes into a region of size 0
overflows the destination [-Wstringop-overflow=]
  119 |   return __glibc_fortify (readlink, __len, sizeof (char),
      |          ^~~~~~~~~~~~~~~
qt.C: In function ‘int main()’:
qt.C:24:8: note: at offset 16 into destination object
‘QTypedArrayData<char>::<anonymous>’ of size 16
   24 | struct QTypedArrayData : QArrayData {
      |        ^~~~~~~~~~~~~~~
In file included from /usr/include/unistd.h:1214,
                 from qt.C:3:
/usr/include/bits/unistd.h:100:16: note: in a call to function ‘ssize_t
__readlink_chk(const char*, char*, size_t, size_t)’ declared with attribute
‘access (write_only, 2, 3)’
  100 | extern ssize_t __readlink_chk (const char *__restrict __path,
      |                ^~~~~~~~~~~~~~
In function ‘ssize_t readlink(const char*, char*, size_t)’,
    inlined from ‘int main()’ at qt.C:37:24:
/usr/include/bits/unistd.h:119:10: warning: call to ‘__readlink_chk_warn’
declared with attribute warning: readlink called with bigger length than size
of destination buffer [-Wattribute-warning]
  119 |   return __glibc_fortify (readlink, __len, sizeof (char),
      |          ^~~~~~~~~~~~~~~
*** buffer overflow detected ***: terminated
Aborted (core dumped)

[Bug tree-optimization/105078] Maybe wrong *** buffer overflow detected ***: terminated with -D_FORTIFY_SOURCE

[siddhesh at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105078

--- Comment #1 from Siddhesh Poyarekar <siddhesh at gcc dot gnu.org> ---
With gcc12:

Computing maximum subobject size for _11:
Visiting use-def links for _11
Visiting use-def links for _10
Computing maximum object size for header_12:
Visiting use-def links for header_12
header_12: maximum object size 272
_10: maximum subobject size 16
_11: maximum subobject size 0
Simplified
  _2 = __builtin_object_size (_11, 1);
 to 0
gimple_simplified to if (0 != 0)
gimple_simplified to _4 = 0;
int main ()
{
  struct QArrayData * header;
  long unsigned int _2;
  int _3;
  bool _4;
  int _5;
  long int iftmp.1_6;
  long int iftmp.2_7;
  long int iftmp.2_8;
  long int iftmp.1_9;
  struct QArrayData * _10;
  void * _11;

  <bb 2> [local count: 1073741824]:
  header_12 = malloc (272);
  header_12->size = 256;
  header_12->offset = 16;
  _10 = &MEM[(struct QTypedArrayData *)header_12].D.4557;
  _11 = _10 + 16;
  _2 = __builtin_object_size (_11, 1);
  _4 = 0;
  _5 = __builtin_constant_p (_4);
  if (_5 != 0)
    goto <bb 3>; [50.00%]
  else
    goto <bb 6>; [50.00%]
...


with gcc11:


;; Function main (main, funcdef_no=54, decl_uid=4523, cgraph_uid=48,
symbol_order=47) (executed once)

Computing maximum subobject size for _11:
Visiting use-def links for _11
Visiting use-def links for header_12
_11: maximum subobject size 256
header_12: maximum subobject size 272
Simplified
  _2 = __builtin_object_size (_11, 1);
 to 256
gimple_simplified to if (0 != 0)
gimple_simplified to if (1 != 0)
gimple_simplified to _4 = 1;
int main ()
{
  struct QArrayData * header;
  long unsigned int _2;
  int _3;
  bool _4;
  int _5;
  long int iftmp.1_6;
  long int iftmp.2_7;
  long int iftmp.2_8;
  long int iftmp.1_9;
  void * _11;

  <bb 2> [local count: 1073741823]:
  header_12 = malloc (272);
  header_12->size = 256;
  header_12->offset = 16;
  _11 = &MEM <struct QArrayData> [(void *)header_12 + 16B];
  _2 = __builtin_object_size (_11, 1);
  _4 = 1;
  _5 = __builtin_constant_p (_4);
  if (_5 != 0)
    goto <bb 3>; [50.00%]
  else
    goto <bb 5>; [50.00%]
...

The

    &MEM <struct QArrayData> [(void *)header_12 + 16B]

vs
   _10 = &MEM[(struct QTypedArrayData *)header_12].D.4557;
   _11 = _10 + 16;

appears to be the difference, where the gcc11 version allows the full size
(272) to be seen while the cast to QTypedArrayData in the latter allows only
the sizeof (QTypedArrayData) to be seen as subobject size.

[Bug tree-optimization/105078] Maybe wrong *** buffer overflow detected ***: terminated with -D_FORTIFY_SOURCE

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105078

--- Comment #2 from Martin Liška <marxin at gcc dot gnu.org> ---
> appears to be the difference, where the gcc11 version allows the full size
> (272) to be seen while the cast to QTypedArrayData in the latter allows only
> the sizeof (QTypedArrayData) to be seen as subobject size.

Started with r12-2270-gdddb6ffdc5c25264.

[Bug tree-optimization/105078] Maybe wrong *** buffer overflow detected ***: terminated with -D_FORTIFY_SOURCE

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105078

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
This changed with r12-2270-gdddb6ffdc5c25264dd75ad82dad8e48a0718d2d9
Before that commit it has been the forwprop2 pass that changed
  _6 = &MEM[(struct QTypedArrayData *)header_8].D.2415;
  _7 = _6 + 16;
to
  _7 = &MEM <struct QArrayData> [(void *)header_8 + 16B];
and that was done before objsz2 pass, but now objsz is done before fwprop2
(intentionally).
The D.2415 there is the artificial base object.
I'd say the C++ derived class case is similar to:
struct S { int size; __PTRDIFF_TYPE__ offset; };
struct T { struct S base; };
__SIZE_TYPE__ foo (struct T *p) { return __builtin_object_size ((char *)
&p->base + sizeof (struct S), 1); }
__SIZE_TYPE__ bar (struct T *p) { return __builtin_object_size ((char *) p +
sizeof (struct S), 1); }
We've been since forever returning 0 from foo and all ones from bar, because in
the first case we are crossing subobject boundary.
I guess what would work is add char payload[]; to QArrayData and base the
pointer on address of that (+ offset - offsetof(QArrayData, payload)).

[Bug tree-optimization/105078] Maybe wrong *** buffer overflow detected ***: terminated with -D_FORTIFY_SOURCE

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105078

--- Comment #4 from Martin Liška <marxin at gcc dot gnu.org> ---
Note the libQt6 version of the function looking approximately like this:

#include <cstdlib>
#include <cstdint>
#include <unistd.h>

struct QArrayData { 
        int size; 

        __attribute__((malloc))
        static void *allocate(QArrayData **pdata, size_t size, size_t
alignment) { 
                size_t headerSize = sizeof(QArrayData); 
                headerSize += (alignment - alignof(QArrayData)); 
                *pdata = static_cast<QArrayData *>(::malloc(headerSize +
size)); 
                (*pdata)->size = size; 
                return reinterpret_cast<void *>(uintptr_t(*pdata) +
headerSize); 
        } 
}; 

template <class T> 
struct QTypedArrayData : QArrayData { 
        class AlignmentDummy { QArrayData header; T data; }; 

        static QTypedArrayData *allocate(size_t size) { 
                QArrayData *d; 
                QArrayData::allocate(&d, size, alignof(AlignmentDummy)); 
                return static_cast<QTypedArrayData *>(d); 
        } 

        static T *dataStart(QArrayData *data, size_t alignment) { 
                void *start = reinterpret_cast<void *>((uintptr_t(data) +
sizeof(QArrayData) + alignment - 1) & ~(alignment - 1)); 
                return static_cast<T *>(start); 
        } 
}; 

int main() 
{ 
        int size = 256; 
        auto *data = QTypedArrayData<char>::allocate(size); 
        return readlink("asdf", data->dataStart(data,
alignof(QTypedArrayData<char>::AlignmentDummy)), data->size - 1); 
}

[Bug tree-optimization/105078] Maybe wrong *** buffer overflow detected ***: terminated with -D_FORTIFY_SOURCE

[siddhesh at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105078

--- Comment #5 from Siddhesh Poyarekar <siddhesh at gcc dot gnu.org> ---
(In reply to Martin Liška from comment #4)
> Note the libQt6 version of the function looking approximately like this:
> 

This doesn't warn anymore (and doesn't crash either) because objsz cannot get
past the bit_and_expr:

  gimple_assign <plus_expr, _11, data.0_10, 7, NULL>
  gimple_assign <bit_and_expr, _12, _11, 18446744073709551612, NULL>
  gimple_assign <nop_expr, start_13, _12, NULL, NULL>
  gimple_call <__builtin_object_size, _2, start_13, 1>

However, ISTM from the remaining IR that thanks to the casts, it will see the
right size if it got past the bit_and_expr.

Jakub, should we recognize bit_and_expr in objsz for gcc13?

[Bug tree-optimization/105078] Maybe wrong *** buffer overflow detected ***: terminated with -D_FORTIFY_SOURCE

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105078

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
No, IMHO we shouldn't look through that.
We should leave some way for Qt to actually implement what they want...
And doing the computation on uintptr_t certainly looks like that.

[Bug tree-optimization/105078] Maybe wrong *** buffer overflow detected ***: terminated with -D_FORTIFY_SOURCE

[siddhesh at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105078

Siddhesh Poyarekar <siddhesh at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |WONTFIX

--- Comment #7 from Siddhesh Poyarekar <siddhesh at gcc dot gnu.org> ---
OK then, this one is a WONTFIX too, the libQt6 version of the function should
reliably produce the intended effect qtbase needs.

[Bug tree-optimization/105078] Maybe wrong *** buffer overflow detected ***: terminated with -D_FORTIFY_SOURCE

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105078

--- Comment #8 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
*** Bug 105709 has been marked as a duplicate of this bug. ***