Bug 82646 - bogus -Wstringop-overflow with -D_FORTIFY_SOURCE=2 on strncpy with range to a member array
Summary: bogus -Wstringop-overflow with -D_FORTIFY_SOURCE=2 on strncpy with range to a...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 8.0
: P3 normal
Target Milestone: ---
Assignee: Martin Sebor
URL:
Keywords: diagnostic, patch
Depends on:
Blocks:
 
Reported: 2017-10-21 02:05 UTC by Martin Sebor
Modified: 2017-12-06 17:59 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail: 7.2.0, 8.0
Last reconfirmed: 2017-12-05 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-10-21 02:05:11 UTC
In contrast to bug 82645, when compiling with -D_FORTIFY_SOURCE=2 GCC issues a bogus -Wstringop-overflow on the following test case that then runs successfully to completion (i.e., no abort):

$ (set -x && cat y.c && gcc -O2 -Wall y.c && gcc -D_FORTIFY_SOURCE=2 -O2 -Wall y.c && ./a.out)
+ cat y.c
#include <string.h>

struct S { char a[5]; void (*pf)(void); };

void __attribute__ ((weak))
sink (const char *s)
{
  __builtin_printf ("%.7s\n", s);
}

void __attribute__ ((weak))
g (struct S *p, int n)
{
  if (n < 5) n = 5;

  strncpy (p->a, "1234567", n);
  sink (p->a);
}

int main (void)
{
  struct S s = { };
  g (&s, 1);
}
+ gcc -O2 -Wall y.c
+ gcc -D_FORTIFY_SOURCE=2 -O2 -Wall y.c
In file included from /usr/include/string.h:635:0,
                 from y.c:1:
In function ‘strncpy’,
    inlined from ‘g’ at y.c:16:3:
/usr/include/bits/string3.h:126:10: warning: ‘__builtin___strncpy_chk’ writing 7 bytes into a region of size 5 overflows the destination [-Wstringop-overflow=]
   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ ./a.out
12345
Comment 1 Jeffrey A. Law 2017-12-05 00:46:14 UTC
This test looks bogus to me.

"g" boils down to:

g (struct S * p, int n)
{
  long unsigned int _1;
  char[5] * _2;

;;   basic block 2, loop depth 0, count 1073741825 (estimated locally), maybe hot
;;    prev block 0, next block 1, flags: (NEW, REACHABLE, VISITED)
;;    pred:       ENTRY [always]  count:1073741826 (estimated locally) (FALLTHRU,EXECUTABLE)
  n_7 = MAX_EXPR <n_4(D), 5>;
  _1 = (long unsigned int) n_7;
  _2 = &p_5(D)->a;
  __builtin___strncpy_chk (_2, "1234567", _1, 5);
  sink (_2);
  return;
;;    succ:       EXIT [always (guessed)]  count:1073741825 (estimated locally) (EXECUTABLE)

}

We can pretty easily see that _1 can exceed "7" and thus we could do an out-of-bounds write.  THe fact that it doesn't is because main passes in the value of 1.  MAX (1, 5) is 5, thus no runtime failure.  Pass in a large value to g and you'll get a nice runtime failure.
Comment 2 Martin Sebor 2017-12-05 03:14:32 UTC
This is my bad for letting these bugs sit so long without fixing them.

-Wstringop-overflow is meant to warn only for provable overflow.  In g(), the overflow is possible but not inevitable.  The only call to the function in the program is with an argument that guarantees the overflow doesn't happen, and so the warning should not be issued.

The bug here is in the maybe_emit_chk_warning() function in builtins.c called to handle __builtin___strncpy_chk.  The function passes the strncpy() bound as the maxlen argument to check_sizes() when it should pass it as the size argument analogously to the check_strncpy_sizes() function called for __builtin_strncpy.

The following patch fixes the problem.  Let me run the full regression test suite and submit it.

diff --git a/gcc/builtins.c b/gcc/builtins.c
index 097e1b7..3278c7f 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -9862,6 +9862,8 @@ maybe_emit_chk_warning (tree exp, enum built_in_function fcode)
      (such as __strcat_chk).  */
   tree maxlen = NULL_TREE;
 
+  tree count = NULL_TREE;
+
   switch (fcode)
     {
     case BUILT_IN_STRCPY_CHK:
@@ -9888,7 +9890,7 @@ maybe_emit_chk_warning (tree exp, enum built_in_function fcode)
     case BUILT_IN_STRNCPY_CHK:
     case BUILT_IN_STPNCPY_CHK:
       srcstr = CALL_EXPR_ARG (exp, 1);
-      maxlen = CALL_EXPR_ARG (exp, 2);
+      count = CALL_EXPR_ARG (exp, 2);
       objsize = CALL_EXPR_ARG (exp, 3);
       break;
 
@@ -9911,7 +9913,7 @@ maybe_emit_chk_warning (tree exp, enum built_in_function fcode)
     }
 
   check_sizes (OPT_Wstringop_overflow_, exp,
-              /*size=*/NULL_TREE, maxlen, srcstr, objsize);
+              count, maxlen, srcstr, objsize);
 }
 
 /* Emit warning if a buffer overflow is detected at compile time
Comment 3 Jeffrey A. Law 2017-12-05 20:07:41 UTC
Sorry.  I shouldn't have closed this.
Comment 4 Martin Sebor 2017-12-05 23:48:27 UTC
Patch posted for review:
https://gcc.gnu.org/ml/gcc-patches/2017-12/msg00277.html
Comment 5 Martin Sebor 2017-12-06 17:59:33 UTC
Author: msebor
Date: Wed Dec  6 17:59:01 2017
New Revision: 255448

URL: https://gcc.gnu.org/viewcvs?rev=255448&root=gcc&view=rev
Log:
PR tree-optimization/82646 - bogus -Wstringop-overflow with -D_FORTIFY_SOURCE=2 on strncpy with range to a member array

gcc/ChangeLog:

	PR tree-optimization/82646
	* builtins.c (maybe_emit_chk_warning): Use size as the bound for
	strncpy, not maxlen.

gcc/testsuite/ChangeLog:

	PR tree-optimization/82646
	* gcc.dg/builtin-stringop-chk-1.c: Adjust.
	* gcc.dg/builtin-stringop-chk-9.c: New test.
	* g++.dg/ext/strncpy-chk1.C: Adjust.


Added:
    trunk/gcc/testsuite/gcc.dg/builtin-stringop-chk-9.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/builtins.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/g++.dg/ext/strncpy-chk1.C
    trunk/gcc/testsuite/gcc.dg/builtin-stringop-chk-1.c
Comment 6 Martin Sebor 2017-12-06 17:59:49 UTC
Fixed in r255448.