Bug 94647 - [10 Regression] bogus -Warray-bounds on strncpy into a larger member array from a smaller array
Summary: [10 Regression] bogus -Warray-bounds on strncpy into a larger member array fr...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 10.0
: P3 normal
Target Milestone: 10.0
Assignee: Martin Sebor
URL:
Keywords: diagnostic, patch
Depends on:
Blocks: Warray-bounds
  Show dependency treegraph
 
Reported: 2020-04-18 11:13 UTC by Matthias Klose
Modified: 2020-04-22 15:35 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work: 9.3.1
Known to fail: 10.0
Last reconfirmed: 2020-04-18 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Klose 2020-04-18 11:13:39 UTC
[forwarded from https://bugs.debian.org/958062]

seen with trunk 20200417, works with the gcc-9 branch.

$ cat testcase.c
/* Compile with gcc-10 -O2 -c testcase.c -Wall -Wformat -Werror=format-security */

#include <string.h>

struct a
{
	int pad;
	char string[512];
};

struct b
{
	int pad;
	char string[256];
};

int f(struct a *d, struct b *s)
{
	int l;

	/* No warning here, so GCC 10 assumes that d->string is properly
	 * null terminated. */
	l = strlen(d->string);

	/* Warning here, GCC 10 assumes that d->string is *not* properly
	 * null terminated */
	strncpy(d->string, s->string, sizeof(d->string));

	return l;
}


$ gcc-10 -O2 -c testcase.c -Wall -Wformat -Werror=format-security
In file included from /usr/include/string.h:495,
                 from testcase.c:3:
In function ‘strncpy’,
    inlined from ‘f’ at testcase.c:27:2:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: warning: ‘__builtin_strncpy’ offset [260, 511] from the object at ‘s’ is out of the bounds of referenced subobject ‘string’ with type ‘char[256]’ at offset 4 [-Warray-bounds]
  106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
testcase.c: In function ‘f’:
testcase.c:14:7: note: subobject ‘string’ declared here
   14 |  char string[256];
      |       ^~~~~~
Comment 1 Martin Sebor 2020-04-18 17:07:52 UTC
Confirmed with the slightly simplified test case below:

$ cat pr94647.c && gcc -O2 -S -Wall pr94647.c
char a[4], b[8];

void f (void)
{
  __builtin_strncpy (b, a, sizeof b);   // no warning
}

struct S
{ 
  char a[4], b[8];
};

void g (struct S *p)
{
  __builtin_strncpy (p->b, p->a, sizeof p->b);   // bogus -Warray-bounds
}

pr94647.c: In function ‘g’:
pr94647.c:15:3: warning: ‘__builtin_strncpy’ offset [4, 7] from the object at ‘p’ is out of the bounds of referenced subobject ‘a’ with type ‘char[4]’ at offset 0 [-Warray-bounds]
   15 |   __builtin_strncpy (p->b, p->a, sizeof p->b);   // bogus -Warray-bounds
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
pr94647.c:10:8: note: subobject ‘a’ declared here
   10 |   char a[4], b[8];
      |        ^


The spurious warning was introduced by r275981:

Author: Martin Sebor <msebor@redhat.com>
Date:   Thu Sep 19 22:15:34 2019 +0000

    PR middle-end/91631 - buffer overflow into an array member of a declared object not detected
    
    gcc/ChangeLog:
    
            PR middle-end/91631
            * builtins.c (component_size): Correct trailing array computation,
            rename to component_ref_size and move...
            (compute_objsize): Adjust.
            * gimple-ssa-warn-restrict.c (builtin_memref::refsize): New member.
            (builtin_access::strict): Do not consider mememmove.
            (builtin_access::write_off): New function.
            (builtin_memref::builtin_memref): Initialize refsize.
            (builtin_memref::set_base_and_offset): Adjust refoff and compute
            refsize.
            (builtin_memref::offset_out_of_bounds): Use ooboff input values.
            Handle refsize.
            (builtin_access::builtin_access): Intialize dstoff to destination
            refeence offset here instead of in maybe_diag_overlap.  Adjust
            referencess even to unrelated objects.  Adjust sizrange of bounded
            string functions to reflect bound.  For strcat, adjust destination
            sizrange by that of source.
            (builtin_access::strcat_overlap):  Adjust offsets and sizes
            to reflect the increase in destination sizrange above.
            (builtin_access::overlap): Do not set dstoff here but instead
            in builtin_access::builtin_access.
            (check_bounds_or_overlap): Use builtin_access::write_off.
            (maybe_diag_access_bounds): Add argument.  Add informational notes.
            (dump_builtin_memref, dump_builtin_access): New functions.
            * tree.c (component_ref_size): ...to here.
            * tree.h (component_ref_size): Declare.
            * tree-ssa-strlen (handle_builtin_strcat): Include the terminating
            nul in the size of the source string.
    
    gcc/testsuite/ChangeLog:
    
            PR middle-end/91631
            * /c-c++-common/Warray-bounds-3.c: Correct expected offsets.
            * /c-c++-common/Warray-bounds-4.c: Same.
            * gcc.dg/Warray-bounds-39.c: Remove xfails.
            * gcc.dg/Warray-bounds-45.c: New test.
            * gcc.dg/Warray-bounds-46.c: New test.
    
    From-SVN: r275981
Comment 2 Richard Biener 2020-04-20 07:01:16 UTC
I think this is also about trailing arrays not appropriately handled since
clearly s->string can be bigger than 256 chars.
Comment 3 Martin Sebor 2020-04-20 17:35:56 UTC
I'm testing a patch.

-Warray-bounds (and all other similar warnigs) only considers valid accesses to trailing arrays with zero or one element.
Comment 5 GCC Commits 2020-04-21 17:13:36 UTC
The master branch has been updated by Martin Sebor <msebor@gcc.gnu.org>:

https://gcc.gnu.org/g:3942060c4b3168307b9e2870d81e7ca15b49760a

commit r10-7854-g3942060c4b3168307b9e2870d81e7ca15b49760a
Author: Martin Sebor <msebor@redhat.com>
Date:   Tue Apr 21 10:59:24 2020 -0600

    PR middle-end/94647 - bogus -Warray-bounds on strncpy into a larger member array from a smaller array
    
    gcc/ChangeLog:
    
            PR middle-end/94647
            * gimple-ssa-warn-restrict.c (builtin_access::builtin_access): Correct
            the computation of the lower bound of the source access size.
            (builtin_access::generic_overlap): Remove a hack for setting ranges
            of overlap offsets.
    
    gcc/testsuite/ChangeLog:
    
            PR middle-end/94647
            * c-c++-common/Warray-bounds-2.c: Adjust a test case and add a new one.
            * c-c++-common/Warray-bounds-3.c: Add tests for missing warnings.
            * c-c++-common/Wrestrict.c: Invert bounds in printed ranges.
            * gcc.dg/Warray-bounds-59.c: New test.
            * gcc.dg/Wrestrict-10.c: Add a missing warning.
            * gcc.dg/Wrestrict-5.c: Adjust text of expected warning.
            * gcc.dg/Wrestrict-6.c: Expect to see a range of overlap offsets.
Comment 6 Martin Sebor 2020-04-21 17:15:07 UTC
Fix committed in comment #5.
Comment 7 Jakub Jelinek 2020-04-22 14:59:46 UTC
I see on i686-linux and powerpc64-linux (latter with -m32):
+FAIL: c-c++-common/Warray-bounds-2.c  -Wc++-compat   (test for warnings, line 192)
+FAIL: c-c++-common/Warray-bounds-2.c  -Wc++-compat  (test for excess errors)
+FAIL: c-c++-common/Warray-bounds-2.c  -std=gnu++11  (test for warnings, line 192)
+FAIL: c-c++-common/Warray-bounds-2.c  -std=gnu++11 (test for excess errors)
+FAIL: c-c++-common/Warray-bounds-2.c  -std=gnu++14  (test for warnings, line 192)
+FAIL: c-c++-common/Warray-bounds-2.c  -std=gnu++14 (test for excess errors)
+FAIL: c-c++-common/Warray-bounds-2.c  -std=gnu++17  (test for warnings, line 192)
+FAIL: c-c++-common/Warray-bounds-2.c  -std=gnu++17 (test for excess errors)
+FAIL: c-c++-common/Warray-bounds-2.c  -std=gnu++2a  (test for warnings, line 192)
+FAIL: c-c++-common/Warray-bounds-2.c  -std=gnu++2a (test for excess errors)
+FAIL: c-c++-common/Warray-bounds-2.c  -std=gnu++98  (test for warnings, line 192)
+FAIL: c-c++-common/Warray-bounds-2.c  -std=gnu++98 (test for excess errors)
Comment 8 GCC Commits 2020-04-22 15:35:13 UTC
The master branch has been updated by Martin Sebor <msebor@gcc.gnu.org>:

https://gcc.gnu.org/g:203f2b73e256c779a3ab73c5dc5efafd3d46f8e1

commit r10-7884-g203f2b73e256c779a3ab73c5dc5efafd3d46f8e1
Author: Martin Sebor <msebor@redhat.com>
Date:   Wed Apr 22 09:30:37 2020 -0600

    Fix an ILP32 failure.
    
    gcc/testsuite/ChangeLog:
    
            PR middle-end/94647
            * c-c++-common/Warray-bounds-2.c: Replace a large value harcoded
            in an expected warning with a pattern.