Bug 89689 - false warning -Wstringop-overflow=
Summary: false warning -Wstringop-overflow=
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 8.3.1
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
Depends on:
Blocks: Wstringop-overflow
  Show dependency treegraph
 
Reported: 2019-03-12 20:51 UTC by Xavier
Modified: 2022-12-19 21:36 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail: 8.3.0, 9.0
Last reconfirmed: 2019-03-12 00:00:00


Attachments
test case (380 bytes, text/x-csrc)
2019-03-12 20:51 UTC, Xavier
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Xavier 2019-03-12 20:51:14 UTC
Created attachment 45953 [details]
test case

I am testing gcc version 8.3.1 20190228

With "gcc -O" I get the following warning:
In function ‘a’,
    inlined from ‘o’ at result2.c:31:5:
result2.c:7:12: warning: ‘__builtin_memcpy’ writing 4 bytes into a region of size 1 overflows the destination [-Wstringop-overflow=]
     return __builtin___memcpy_chk(c, d, n, __builtin_object_size(c, 0));
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

With "gcc -O -Wall" I get the following warning:
In function ‘a’,
    inlined from ‘o’ at result2.c:31:5:
result2.c:7:12: warning: ‘__builtin_memcpy’ forming offset [2, 4] is out of the bounds [0, 1] of object ‘__sb_slop’ with type ‘const char[1]’ [-Warray-bounds]
     return __builtin___memcpy_chk(c, d, n, __builtin_object_size(c, 0));
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
result2.c: In function ‘o’:
result2.c:13:12: note: ‘__sb_slop’ declared here
 const char __sb_slop[1];
            ^~~~~~~~~

In both cases it looks buggy, because h.data points to "char buf[5]" and not to "const char __sb_slop[1]".

According to my tests on godbolt.org, the first warning appeared on gcc 7, the second warning appeared on gcc 8. There was no warning with gcc <= 6.

This test case does not make sense but it was derived from production code using creduce.
I do not want to disable Wstringop-overflow or Warray-bounds entirely because they look useful sometimes, and I did not find a workaround by modifying the code. Fortunately the warning can be disabled locally using pragma directives.
Comment 1 Martin Sebor 2019-03-12 23:15:15 UTC
Confirmed.  The warning is an unfortunate side-effect of an optimization triggered by the 'if (c->data != __sb_slop)' test in the set0 function.  It introduces a call to memcpy with __sb_slop as the destination.  The intermediate representation of the code can be seen in the VRP dump with the smaller test case below.  We're discussing ways to prevent this in the future but the only way to avoid it for now that I can think of other than the #pragma(*) is to either assert(**) h.data != __sb_slop just before calling memcpy, or "hide" the conditional so it's not visible when compiling the function that calls memcpy (e.g., by preventing set0 from being inlined via attribute noinline), disable the warning for this code, or disable _FORTIFY_SOURCE for this function by calling __builtin_memcpy directly.

[*] It's worth mentioning that #pragma GCC diagnostic doesn't work very well for these late warnings when inlining is involved.
[**] An assert that would work without any cost is "if (h.data == __sb_slop) __builtin_unreachable ();"

$ cat a.c && gcc -O2 -S -Wall -fdump-tree-vrp1=/dev/stdout a.c
char __sb_slop[1], buf[5];

void f (char *p)
{
  if (p != __sb_slop)
    p[0] = 0;

  __builtin___memcpy_chk (p, "abcd", 4, __builtin_object_size (p, 0));

  if (p != __sb_slop)
    p[0] = 0;

}


...
f (char * p)
{
  long unsigned int _15;
  long unsigned int _18;

  <bb 2> [local count: 1073741824]:
  if (p_4(D) != &__sb_slop)
    goto <bb 3>; [70.00%]
  else
    goto <bb 5>; [30.00%]                 ;; p_4(D) == &__sb_slop

  <bb 3> [local count: 751619278]:
  *p_4(D) = 0;
  _15 = __builtin_object_size (p_4(D), 0);
  __builtin_memcpy (p_4(D), "abcd", 4);
  *p_4(D) = 0;

  <bb 4> [local count: 1073741824]:
  return;

  <bb 5> [local count: 322122544]:
  _18 = __builtin_object_size (p_4(D), 0);
  __builtin_memcpy (p_4(D), "abcd", 4);   ;; p_4(D) == &__sb_slop
  goto <bb 4>; [100.00%]

}


a.c: In function ‘f’:
a.c:8:3: warning: ‘__builtin_memcpy’ forming offset [2, 4] is out of the bounds [0, 1] of object ‘__sb_slop’ with type ‘char[1]’ [-Warray-bounds]
    8 |   __builtin___memcpy_chk (p, "abcd", 4, __builtin_object_size (p, 0));
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
a.c:1:6: note: ‘__sb_slop’ declared here
    1 | char __sb_slop[1], buf[5];
      |      ^~~~~~~~~
Comment 2 Xavier 2019-03-13 11:37:07 UTC
Thanks for the quick answer and for the clear explanation !
I indeed thought about using an assert, I think we will go with the __builtin_unreachable suggestion.
You motivated me to open a similar issue I found : 89699.

I also had surprising errors with ignored-qualifiers and class-memaccess, I will try to get a test case for these as well.
Comment 3 Jeffrey A. Law 2019-04-25 16:23:32 UTC
It's almost certainly the case that we've duplicated the block with the call to builtin_memcpy_chk so that we can thread jumps and remove the second   if (p != __sb_slop) test.


In .forwprop2 we have:

;;   basic block 4, loop depth 0
;;    pred:       2
;;                3
  _1 = __builtin_object_size (p_4(D), 0);
  __builtin___memcpy_chk (p_4(D), "abcd", 4, _1);
  if (p_4(D) != &__sb_slop)
    goto <bb 5>; [70.00%]
  else
    goto <bb 6>; [30.00%]

Which is exactly what we'd expect.  Then objsz2 runs resulting in:

;;   basic block 4, loop depth 0
;;    pred:       2
;;                3
  _1 = __builtin_object_size (p_4(D), 0);
  __builtin_memcpy (p_4(D), "abcd", 4);
  if (p_4(D) != &__sb_slop)
    goto <bb 5>; [70.00%]
  else
    goto <bb 6>; [30.00%]

What's happened here?  Well, objsz evaluated the _b_o_s call and determined it was undeterminable (-1) and transformed the memcpy_chk to a standard memcpy.

The key point is we had an indeterminable size and transformed the _chk into a standard memcpy.

Jump threading comes along and duplicates the block.  As a result on the duplicated path we'll know that p_4 points to sb_slop and thus has a size of 1.  That causes the memcpy of 4 bytes to trigger the warning.

I would hazard a guess that the original user code didn't have the calls to _builtin_object_size and __builtin__memcpy_chk, but that those instead came in via the glibc header files.

This certainly isn't going to be fixed for gcc-9 (possibly ever).

And FWIW, I think marking things with TREE_NO_WARNING at the time we convert the memcpy_chk to a memcpy would be fundamentally wrong in the case where the __b_o_s call returned -1 (indeterminate size).  It'll hide real bugs.
Comment 4 Jeffrey A. Law 2020-01-28 19:54:11 UTC
So it occurs to me that in this specific case if DSE did a better job we would eliminate the first conditional branch which in turn would eliminate the need for jump threading and the const/copy propagation that is causing headaches.

And just to be clear that won't fix the more general case where a conditional const/copy propagation changes the destination object of checked routine like memcpy.  Even so, improving DSE is generally a good thing.

So with that in mind, this is what the key parts of the IL looks like in DSE1:

;;   basic block 2, loop depth 0, maybe hot
;;    prev block 0, next block 3, flags: (NEW, REACHABLE, VISITED)
;;    pred:       ENTRY (FALLTHRU,EXECUTABLE)
  h$data_33 = l.data;
  if (h$data_33 != &__sb_slop)
    goto <bb 3>; [70.00%]
  else
    goto <bb 4>; [30.00%]
;;    succ:       3 [70.0% (guessed)]  (TRUE_VALUE,EXECUTABLE)
;;                4 [30.0% (guessed)]  (FALSE_VALUE,EXECUTABLE)

;;   basic block 3, loop depth 0, maybe hot
;;    prev block 2, next block 4, flags: (NEW, REACHABLE, VISITED)
;;    pred:       2 [70.0% (guessed)]  (TRUE_VALUE,EXECUTABLE)
  *h$data_33 = 0;
;;    succ:       4 [always]  (FALLTHRU,EXECUTABLE)

;;   basic block 4, loop depth 0, maybe hot
;;    prev block 3, next block 5, flags: (NEW, REACHABLE, VISITED)
;;    pred:       3 [always]  (FALLTHRU,EXECUTABLE)
;;                2 [30.0% (guessed)]  (FALSE_VALUE,EXECUTABLE)
  _23 = __builtin_object_size (h$data_33, 0);
  _25 = __builtin___memcpy_chk (h$data_33, "abcd", 4, _23);

We can see that the store in BB3 will always be overwritten by the memcpy.  I'm actually a bit surprised that DSE didn't catch this and remove the *h$data_33 = 0 statement.  [ Imagine a pause here while I throw things under the debugger... ]

Ah, b_o_s is considered as potentially reading h$data_33.  If we fix that, then we end up removing the store as dead and we get exactly the cascading events we want and the bogus warning is avoided.

For reasons unknown we consider _b_o_s pure, not const.  Jakub thinks were just being very cautious given the security implications around mucking up _b_o_s.  ANyway, I'm going to dig into that history and see if we ever had it or discussed the constness of _b_o_s.
Comment 5 GCC Commits 2020-01-29 19:27:22 UTC
The master branch has been updated by Jeff Law <law@gcc.gnu.org>:

https://gcc.gnu.org/g:0de349f108d963219bd59aa374a68c15355236be

commit r10-6332-g0de349f108d963219bd59aa374a68c15355236be
Author: Jeff Law <law@redhat.com>
Date:   Wed Jan 29 12:23:53 2020 -0700

    Improve DSE which in turn eliminates the need for jump threading and block duplication for the original testcase in pr89689 which in turn eliminates the false positive -Warray-bounds warning for the original testcase.
    
    	PR tree-optimization/89689
    	* builtins.def (BUILT_IN_OBJECT_SIZE): Make it const rather than pure.
    
    	PR tree-optimization/89689
    	* gcc.dg/pr89689.c: New test.
Comment 6 Jeffrey A. Law 2020-01-29 19:31:28 UTC
Patch to improve DSE was committed to the trunk which addresses the original report.  It does not address the deeper issues raised by Martin or cases that could be trivially derived from the original by placing other live statements within the true arm of the conditional.
Comment 7 Jakub Jelinek 2020-05-07 11:56:27 UTC
GCC 10.1 has been released.
Comment 8 Richard Biener 2020-07-23 06:51:50 UTC
GCC 10.2 is released, adjusting target milestone.
Comment 9 Richard Biener 2021-04-08 12:02:05 UTC
GCC 10.3 is being released, retargeting bugs to GCC 10.4.
Comment 10 Jakub Jelinek 2022-06-28 10:36:57 UTC
GCC 10.4 is being released, retargeting bugs to GCC 10.5.
Comment 11 Andrew Pinski 2022-12-19 21:36:00 UTC
This is no longer a regression but "It does not address the deeper issues raised by Martin or cases that could be trivially derived from the original by placing other live statements within the true arm of the conditional." So marking the bug report that way ...