Bug 105078 - Maybe wrong *** buffer overflow detected ***: terminated with -D_FORTIFY_SOURCE
Summary: Maybe wrong *** buffer overflow detected ***: terminated with -D_FORTIFY_SOURCE
Status: RESOLVED WONTFIX
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 12.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
: 105709 (view as bug list)
Depends on:
Blocks:
 
Reported: 2022-03-28 09:17 UTC by Martin Liška
Modified: 2022-05-24 02:46 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Liška 2022-03-28 09:17:26 UTC
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)
Comment 1 Siddhesh Poyarekar 2022-03-28 10:24:40 UTC
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.
Comment 2 Martin Liška 2022-03-28 10:40:27 UTC
> 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.
Comment 3 Jakub Jelinek 2022-03-28 11:08:24 UTC
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)).
Comment 4 Martin Liška 2022-03-28 14:06:33 UTC
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); 
}
Comment 5 Siddhesh Poyarekar 2022-03-28 15:42:23 UTC
(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?
Comment 6 Jakub Jelinek 2022-03-28 15:52:06 UTC
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.
Comment 7 Siddhesh Poyarekar 2022-03-28 16:17:45 UTC
OK then, this one is a WONTFIX too, the libQt6 version of the function should reliably produce the intended effect qtbase needs.
Comment 8 Andrew Pinski 2022-05-24 02:46:17 UTC
*** Bug 105709 has been marked as a duplicate of this bug. ***