Bug 79220 - missing -Wstringop-overflow= on a memcpy overflow with a small power-of-2 size
Summary: missing -Wstringop-overflow= on a memcpy overflow with a small power-of-2 size
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 7.0
: P3 normal
Target Milestone: 9.0
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
Depends on:
Blocks: Wstringop-overflow
  Show dependency treegraph
 
Reported: 2017-01-25 00:49 UTC by Martin Sebor
Modified: 2019-01-18 21:56 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work: 9.0
Known to fail: 7.3.0, 8.2.0
Last reconfirmed: 2017-08-30 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Sebor 2017-01-25 00:49:36 UTC
Both of the two calls to memcpy in the functions in the test case below trivially overflow the destination, yet only the one in f() is diagnosed.  The one in g() is not because GCC transforms the memcpy call to assignment.  Between the two, from a security point, overflowing a buffer with unknown data is a more serious problem than overwriting it with data already present/hardcoded in the program.  GCC should avoid performing the transformation unless the destination is known to be big enough for the write.

$ cat t.c && gcc -O2 -S -Wall -Wextra -Wpedantic -fdump-tree-optimized=/dev/stdout t.c
#include <string.h>

char d[3];

void f (void)
{
  memcpy (d, "0123456789", 8);
}

void g (const char *s)
{
  memcpy (d, s, 8);
}

;; Function f (f, funcdef_no=14, decl_uid=2196, cgraph_uid=14, symbol_order=15)

f ()
{
  <bb 2> [100.00%]:
  memcpy (&d, "0123456789", 8); [tail call]
  return;

}


t.c: In function ‘f’:
t.c:7:3: warning: ‘memcpy’ writing 8 bytes into a region of size 3 overflows the destination [-Wstringop-overflow=]
   memcpy (d, "0123456789", 8);
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~

;; Function g (g, funcdef_no=15, decl_uid=2199, cgraph_uid=15, symbol_order=16)

g (const char * s)
{
  long unsigned int _3;

  <bb 2> [100.00%]:
  _3 = MEM[(char * {ref-all})s_2(D)];
  MEM[(char * {ref-all})&d] = _3;
  return;

}
Comment 1 Martin Sebor 2017-01-25 01:07:18 UTC
See also bug 77799 for a similar problem involving sprintf.
Comment 2 Eric Gallager 2017-08-30 02:22:03 UTC
Confirmed.
Comment 3 Martin Sebor 2017-09-30 16:12:08 UTC
The cause is of the missing warning is the folder (gimple_fold_builtin_memory_op in gimple-fold.c) folding all copies with power-of-two sizes less than MOVE_MAX, with no checking (see below).  MOVE_MAX is typically 8 or 8 but on some targets, including x86_64, it's as much as 16.  Although some basic simple checking could be done there, e.g., on arrays of known size, the folder runs before the full object size information is available and deferring the folding until it apparently isn't desirable.

      /* If we can perform the copy efficiently with first doing all loads
         and then all stores inline it that way.  Currently efficiently
	 means that we can load all the memory into a single integer
	 register which is what MOVE_MAX gives us.  */
      src_align = get_pointer_alignment (src);
      dest_align = get_pointer_alignment (dest);
      if (tree_fits_uhwi_p (len)
	  && compare_tree_int (len, MOVE_MAX) <= 0
	  /* ???  Don't transform copies from strings with known length this
	     confuses the tree-ssa-strlen.c.  This doesn't handle
	     the case in gcc.dg/strlenopt-8.c which is XFAILed for that
	     reason.  */
	  && !c_strlen (src, 2))
	{
	  unsigned ilen = tree_to_uhwi (len);
	  if (pow2p_hwi (ilen))
	    {
Comment 4 Martin Sebor 2018-01-11 23:16:49 UTC
In GCC 8.0 the overflow is diagnosed in both functions.  In f() by -Wstringop-overflow as before, and in both functions (perhaps surprisingly) by the newly enhanced -Warray-bounds warning (r255755).  There is still no -Wstringop-overflow for g() so the limitation hasn't really been removed yet and this bug should stay open until it is, and until the overflow in g() is diagnosed -Wstringop-overflow when -Warray-bounds is disabled.

As an aside, the -Wstringop-overflow for f() will disappear if/when the patch submitted for bug 83508 is committed.

pr79220.c: In function ‘g’:
pr79220.c:12:3: warning: ‘memcpy’ forming offset [4, 8] is out of the bounds [0, 3] of object ‘d’ with type ‘char[3]’ [-Warray-bounds]
   memcpy (d, s, 8);
   ^~~~~~~~~~~~~~~~
pr79220.c:3:6: note: ‘d’ declared here
 char d[3];
      ^
pr79220.c: In function ‘f’:
pr79220.c:7:3: warning: ‘memcpy’ forming offset [4, 8] is out of the bounds [0, 3] of object ‘d’ with type ‘char[3]’ [-Warray-bounds]
   memcpy (d, "0123456789", 8);
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
pr79220.c:3:6: note: ‘d’ declared here
 char d[3];
      ^
pr79220.c:7:3: warning: ‘memcpy’ writing 8 bytes into a region of size 3 overflows the destination [-Wstringop-overflow=]
   memcpy (d, "0123456789", 8);
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
Comment 5 Eric Gallager 2018-10-11 02:13:41 UTC
(In reply to Martin Sebor from comment #4)
> As an aside, the -Wstringop-overflow for f() will disappear if/when the
> patch submitted for bug 83508 is committed.
> 

The patch for bug 83508 was committed as r256683
Comment 6 Martin Sebor 2019-01-18 21:52:24 UTC
GCC 9 diagnoses both calls with -Warray-bounds.  With it disabled, it then issues the expected -Wstringop-overflow:

$ gcc -O2 -S -Wno-array-bounds t.c t.c: In function ‘f’:
t.c:7:3: warning: ‘memcpy’ writing 8 bytes into a region of size 3 overflows the destination [-Wstringop-overflow=]
    7 |   memcpy (d, "0123456789", 8);
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
t.c: In function ‘g’:
t.c:12:3: warning: ‘memcpy’ writing 8 bytes into a region of size 3 overflows the destination [-Wstringop-overflow=]
   12 |   memcpy (d, s, 8);
      |   ^~~~~~~~~~~~~~~~

This was fixed for GCC 9 in r268037.