Bug 92815 - [10 Regression] spurious -Wstringop-overflow writing into a flexible array of an extern struct
Summary: [10 Regression] spurious -Wstringop-overflow writing into a flexible array of...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 10.0
: P2 normal
Target Milestone: 11.0
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic, patch, wrong-code
Depends on:
Blocks: Warray-bounds flexmembers Wstringop-overflow
  Show dependency treegraph
 
Reported: 2019-12-05 00:51 UTC by Martin Sebor
Modified: 2023-07-07 08:45 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work: 11.0
Known to fail: 10.0, 10.5.0, 8.2.0, 9.2.0
Last reconfirmed: 2020-04-22 00:00:00


Attachments
Diff between assembler output produced by gcc and g++ 11 (601 bytes, patch)
2020-05-25 15:44 UTC, Arseny Solokha
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Sebor 2019-12-05 00:51:25 UTC
Diagnosing the strcpy call below isn't justified because the flexible array into which it writes is a member of an extern object that could be initialized to be large enough to fit the string (e.g., as shown in the comment).

$ cat t.c && gcc -O2 -S -Wall -Wno-array-bounds t.c
extern struct S s /* = { 5, { 1, 2, 3, 4, 5 } } */;

void g (void)
{
  __builtin_strcpy (s.ax, "123");   // bogus warning
}
t.c: In function ‘g’:
t.c:7:3: warning: ‘__builtin_memcpy’ writing 4 bytes into a region of size 0 overflows the destination [-Wstringop-overflow=]
    7 |   __builtin_strcpy (s.ax, "123");   // bogus warning
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Comment 1 Martin Sebor 2019-12-05 00:53:57 UTC
The full test case is:

struct S { int n; char ax[]; };

extern struct S s /* = { 5, { 1, 2, 3, 4, 5 } } */;

void g (void)
{
  __builtin_strcpy (s.ax, "123"); 
}


The false positive warning has been issued since r243419 (GCC 7.0):

r243419 | msebor | 2016-12-07 19:01:33 -0500 (Wed, 07 Dec 2016) | 68 lines

PR c/53562 - Add -Werror= support for -D_FORTIFY_SOURCE / __builtin___memcpy_chk
PR middle-end/77784 - duplicate warning for snprintf when n > object size
PR middle-end/78149 - missing warning on strncpy buffer overflow due to an excessive bound
PR middle-end/78138 - missing warnings on buffer overflow with non-constant source length
Comment 2 Martin Sebor 2019-12-05 00:58:16 UTC
With -Warray-bounds GCC issues:

t.c:7:3: warning: ‘__builtin_memcpy’ offset [4, 7] is out of the bounds [0, 4] of object ‘s’ with type ‘struct S’ [-Warray-bounds]
    7 |   __builtin_strcpy (s.ax, "123");
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
t.c:3:17: note: ‘s’ declared here
    3 | extern struct S s /* = { 5, { 1, 2, 3, 4, 5 } } */;
      |                 ^

The -Warray-bounds warning was in turn introduced in r255755 (in GCC 8.0):

r255755 | msebor | 2017-12-16 18:58:34 -0500 (Sat, 16 Dec 2017) | 81 lines

PR tree-optimization/78918 - missing -Wrestrict on memcpy copying over self

gcc/c-family/ChangeLog:

	PR tree-optimization/78918
	* c-common.c (check_function_restrict): Avoid checking built-ins.
	* c.opt (-Wrestrict): Include in -Wall.
Comment 3 Martin Sebor 2020-01-17 16:33:02 UTC
The is a bug in the object size pass that has existed since its introduction.  The addr_object_size() computes the size of the extern object based on its type, not taking into consideration that, as an extension, GCC allows flexible array members to be initialized to more elements than would otherwise fit into the struct.  To avoid this, the function needs to conservatively fail for uninitialized extern structs with a flexible array member.

The modified test case below illustrates it.  With _FORTIFY_SOURCE defined, it aborts.

$ cat pr92815.c && gcc -O2 -Wall -c pr92815.c && gcc -D_FORTIFY_SOURCE=2 -DMAIN -O2 -Wall pr92815.c pr92815.o && ./a.out
#include <string.h>

struct S { int n; char ax[]; };

#if MAIN
extern struct S s;
__attribute__ ((noipa)) void g (void)
{
  strcpy (s.ax, "123");
}
int main (void) { g (); }
#else
struct S s = { 5, { 1, 2, 3, 4, 5 } };
#endif

In file included from /usr/include/string.h:494,
                 from pr92815.c:1:
In function ‘strcpy’,
    inlined from ‘g’ at pr92815.c:9:3:
/usr/include/bits/string_fortified.h:90:10: warning: ‘__builtin___strcpy_chk’ writing 4 bytes into a region of size 0 [-Wstringop-overflow=]
   90 |   return __builtin___strcpy_chk (__dest, __src, __bos (__dest));
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
pr92815.c: In function ‘g’:
pr92815.c:6:17: note: at offset 0 to object ‘s’ with size 4 declared here
    6 | extern struct S s;
      |                 ^
In file included from /usr/include/string.h:494,
                 from pr92815.c:1:
In function ‘strcpy’,
    inlined from ‘g’ at pr92815.c:9:3:
/usr/include/bits/string_fortified.h:90:10: warning: ‘__builtin___memcpy_chk’ offset [4, 7] is out of the bounds [0, 4] of object ‘s’ with type ‘struct S’ [-Warray-bounds]
   90 |   return __builtin___strcpy_chk (__dest, __src, __bos (__dest));
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
pr92815.c: In function ‘g’:
pr92815.c:6:17: note: ‘s’ declared here
    6 | extern struct S s;
      |                 ^
*** buffer overflow detected ***: ./a.out terminated
Aborted (core dumped)
Comment 4 Jakub Jelinek 2020-03-04 09:35:26 UTC
GCC 8.4.0 has been released, adjusting target milestone.
Comment 6 GCC Commits 2020-05-18 21:25:12 UTC
The master branch has been updated by Martin Sebor <msebor@gcc.gnu.org>:

https://gcc.gnu.org/g:7a41fcde6c67faafab8c8ee2a31140999383dcef

commit r11-470-g7a41fcde6c67faafab8c8ee2a31140999383dcef
Author: Martin Sebor <msebor@redhat.com>
Date:   Mon May 18 15:24:12 2020 -0600

    PR middle-end/92815 - spurious -Wstringop-overflow writing into a flexible array of an extern struct
    
    gcc/ChangeLog:
    
            PR middle-end/92815
            * tree-object-size.c (decl_init_size): New function.
            (addr_object_size): Call it.
            * tree.h (last_field): Declare.
            (first_field): Add attribute nonnull.
    
    gcc/testsuite/ChangeLog:
    
            PR middle-end/92815
            * gcc.dg/Warray-bounds-56.c: Remove xfails.
            * gcc.dg/builtin-object-size-20.c: New test.
            * gcc.dg/builtin-object-size-21.c: New test.
Comment 7 Martin Sebor 2020-05-18 21:26:03 UTC
Fixed on trunk for now.
Comment 8 GCC Commits 2020-05-18 22:33:07 UTC
The master branch has been updated by Martin Sebor <msebor@gcc.gnu.org>:

https://gcc.gnu.org/g:8edf0adb6616bd717312d9b305c7d7c9a6b7a171

commit r11-472-g8edf0adb6616bd717312d9b305c7d7c9a6b7a171
Author: Martin Sebor <msebor@redhat.com>
Date:   Mon May 18 16:31:13 2020 -0600

    PR middle-end/92815 - spurious -Wstringop-overflow writing into a flexible array of an extern struct
    
    Adjust test to avoid failures in ILP32 mode.
    
    gcc/testsuite/ChangeLog:
    
            PR middle-end/92815
            * gcc.dg/builtin-object-size-20.c: Adjust to avoid failures in
            ILP32 mode.
Comment 9 Arseny Solokha 2020-05-25 15:44:45 UTC
Created attachment 48593 [details]
Diff between assembler output produced by gcc and g++ 11

Though there are no flexible array members in C++, is it expected for g++ to behave differently w/ this testcase?

While compiling it w/ gcc yeilds the following, which is expected:

% gcc-11.0.0 -c gcc/testsuite/gcc.dg/builtin-object-size-21.c
gcc/testsuite/gcc.dg/builtin-object-size-21.c:29:14: error: size of variable 'xm3_4' is too large
   29 | struct Ax_m3 xm3_4 = { { 0 }, { 1, 2, 3, 3 } };   // { dg-error "too large" }
      |              ^~~~~
gcc/testsuite/gcc.dg/builtin-object-size-21.c:43:14: error: size of variable 'xmx_1' is too large
   43 | struct Ax_mx xmx_1 = { { 0 }, { 1 } };            // { dg-error "too large" }
      |

W/ g++ I have

% g++-11.0.0 -c gcc/testsuite/gcc.dg/builtin-object-size-21.c
/tmp/ccPBumhF.s: Assembler messages:
/tmp/ccPBumhF.s: Fatal error: can't write 14 bytes to section .text of /tmp/ccj6lymE.o: 'file truncated'
/usr/lib/gcc/x86_64-unknown-linux-gnu/11.0.0/../../../../x86_64-unknown-linux-gnu/bin/as: BFD (Gentoo 2.34 p4) 2.34.0 assertion fail /var/tmp/portage/sys-devel/binutils-2.34-r1/work/binutils-2.34/bfd/elf.c:3169
/tmp/ccPBumhF.s: Fatal error: can't close /tmp/ccj6lymE.o: file truncated

because of

[pid 1165642] openat(AT_FDCWD, "builtin-object-size-21.o", O_RDWR|O_CREAT|O_TRUNC, 0666) = 3
<…>
[pid 1165642] fstat(3, {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
[pid 1165642] lseek(3, -9223372036854775808, SEEK_SET) = -1 EINVAL (Invalid argument)

Or, when writing to an "endless" file w/ -o /dev/null:

[pid 1164514] lseek(3, -9223372036854775808, SEEK_SET) = 0
[pid 1164514] read(3, "", 640)          = 0
[pid 1164514] lseek(3, 640, SEEK_CUR)   = 0
[pid 1164514] write(3, "\0builtin-object-size-21.c\0xm3_0\0"..., 96) = 96
[pid 1164514] lseek(3, 0, SEEK_SET)     = 0
[pid 1164514] read(3, "", 4096)         = 0
[pid 1164514] lseek(3, 64, SEEK_CUR)    = 0
[pid 1164514] write(3, "UH\211\345\220]\303UH\211\345\220]\303", 14) = 14
[pid 1164514] lseek(3, 0, SEEK_SET)     = 0
[pid 1164514] read(3, "", 4096)         = 0
[pid 1164514] lseek(3, 96, SEEK_CUR)    = 0
[pid 1164514] write(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 4096) = 4096
[pid 1164514] write(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 4096) = 4096
[pid 1164514] write(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 4096) = 4096
Comment 10 Jakub Jelinek 2021-05-14 09:52:27 UTC
GCC 8 branch is being closed.
Comment 11 Richard Biener 2021-06-01 08:15:42 UTC
GCC 9.4 is being released, retargeting bugs to GCC 9.5.
Comment 12 Richard Biener 2022-05-27 09:41:41 UTC
GCC 9 branch is being closed
Comment 13 Jakub Jelinek 2022-06-28 10:39:06 UTC
GCC 10.4 is being released, retargeting bugs to GCC 10.5.
Comment 14 Richard Biener 2023-07-07 08:45:17 UTC
Fixed in GCC 11.