diff mbox series

issue -Wstringop-overflow for potential overflow, not -truncation (PR 93646)

Message ID 1cd4381b-e1e9-9ae0-ab94-779eb5b07663@gmail.com
State New
Headers show
Series issue -Wstringop-overflow for potential overflow, not -truncation (PR 93646) | expand

Commit Message

Martin Sebor Feb. 10, 2020, 10:47 p.m. UTC
The reporter of RHBZ #1798636 was mislead and confused by GCC
issuing -Wstringop-truncation for a possible overflow in strncat.
It took a few iterations to appreciate this subtlety and realize
the warning was of the wrong kind.

The attached patch adjusts the logic of the function responsible
for the warning not to issue -Wstringop-truncation only for strncpy
and -Wstringop-overflow for strncat.

I'm not sure if the bug is strictly speaking a regression: GCC 8
didn't issue any warning so it could be viewed as one, but then
again, issuing a warning in this instance is intended, but not
a misleading one.

Tested on x86_64-linux.

Martin

Comments

Jeff Law Feb. 12, 2020, 6:14 p.m. UTC | #1
On Mon, 2020-02-10 at 15:47 -0700, Martin Sebor wrote:
> The reporter of RHBZ #1798636 was mislead and confused by GCC
> issuing -Wstringop-truncation for a possible overflow in strncat.
> It took a few iterations to appreciate this subtlety and realize
> the warning was of the wrong kind.
> 
> The attached patch adjusts the logic of the function responsible
> for the warning not to issue -Wstringop-truncation only for strncpy
> and -Wstringop-overflow for strncat.
> 
> I'm not sure if the bug is strictly speaking a regression: GCC 8
> didn't issue any warning so it could be viewed as one, but then
> again, issuing a warning in this instance is intended, but not
> a misleading one.
> 
> Tested on x86_64-linux.
> 
> Martin
> PR middle-end/93646 - confusing -Wstringop-truncation on strncat where -Wstringop-overflow is expected
> 
> gcc/ChangeLog:
> 
> 	PR middle-end/93646
> 	* tree-ssa-strlen.c (handle_builtin_stxncpy): Rename...
> 	(handle_builtin_stxncpy_strncat): ...to this.  Change first argument.
> 	Issue only -Wstringop-overflow strncat, never -Wstringop-truncation.
> 	(strlen_check_and_optimize_call): Adjust callee name.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR middle-end/93646
> 	* gcc.dg/Wstringop-overflow-31.c: New test.
Not strictly a regression bugfix, but I think this is OK for the trunk,
even in stage4.

jeff
diff mbox series

Patch

PR middle-end/93646 - confusing -Wstringop-truncation on strncat where -Wstringop-overflow is expected

gcc/ChangeLog:

	PR middle-end/93646
	* tree-ssa-strlen.c (handle_builtin_stxncpy): Rename...
	(handle_builtin_stxncpy_strncat): ...to this.  Change first argument.
	Issue only -Wstringop-overflow strncat, never -Wstringop-truncation.
	(strlen_check_and_optimize_call): Adjust callee name.

gcc/testsuite/ChangeLog:

	PR middle-end/93646
	* gcc.dg/Wstringop-overflow-31.c: New test.

diff --git a/gcc/testsuite/gcc.dg/Wstringop-overflow-31.c b/gcc/testsuite/gcc.dg/Wstringop-overflow-31.c
new file mode 100644
index 00000000000..24be256a547
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wstringop-overflow-31.c
@@ -0,0 +1,40 @@ 
+/* PR middle-end/93646 - confusing -Wstringop-truncation on strncat where
+   -Wstringop-overflow is expected
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+extern __SIZE_TYPE__ strlen (const char*);
+extern char* strncat (char*, const char*, __SIZE_TYPE__);
+
+
+char a[4];
+
+
+void f0 (char *d, const char *s)
+{
+  strncat (d, s, strlen (s));   // { dg-warning "specified bound depends on the length of the source argument" }
+  /* { dg-message "function 'f0'.*inlined from 'f1'" "inlining stack" { target *-*-* } 0 }  */
+
+  // Prevent f0 from being replaced by g0.
+  *d = 'f';
+}
+
+void f1 (const char *s)
+{
+  f0 (a, s);
+}
+
+
+static void g0 (char *d, const char *s)
+{
+  strncat (d, s, strlen (s));   // { dg-warning "specified bound 3 equals source length" }
+  /* { dg-message "function 'g0'.*inlined from 'g1'" "inlining stack" { target *-*-* } 0 }  */
+
+  // Prevent g0 from being replaced by f0.
+  *d = 'g';
+}
+
+void g1 (void)
+{
+  g0 (a, "123");
+}
diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c
index 1cd64302d8b..9a88a85b07c 100644
--- a/gcc/tree-ssa-strlen.c
+++ b/gcc/tree-ssa-strlen.c
@@ -192,7 +192,7 @@  struct laststmt_struct
 } laststmt;
 
 static int get_stridx_plus_constant (strinfo *, unsigned HOST_WIDE_INT, tree);
-static void handle_builtin_stxncpy (built_in_function, gimple_stmt_iterator *);
+static void handle_builtin_stxncpy_strncat (bool, gimple_stmt_iterator *);
 
 /* Sets MINMAX to either the constant value or the range VAL is in
    and returns either the constant value or VAL on success or null
@@ -2876,10 +2876,10 @@  handle_builtin_strcpy (enum built_in_function bcode, gimple_stmt_iterator *gsi,
    and if so, issue an appropriate warning.  */
 
 static void
-handle_builtin_strncat (built_in_function bcode, gimple_stmt_iterator *gsi)
+handle_builtin_strncat (built_in_function, gimple_stmt_iterator *gsi)
 {
   /* Same as stxncpy().  */
-  handle_builtin_stxncpy (bcode, gsi);
+  handle_builtin_stxncpy_strncat (true, gsi);
 }
 
 /* Return true if LEN depends on a call to strlen(SRC) in an interesting
@@ -2974,8 +2974,8 @@  is_strlen_related_p (tree src, tree len)
   return false;
 }
 
-/* Called by handle_builtin_stxncpy and by gimple_fold_builtin_strncpy
-   in gimple-fold.c.
+/* Called by handle_builtin_stxncpy_strncat and by
+   gimple_fold_builtin_strncpy in gimple-fold.c.
    Check to see if the specified bound is a) equal to the size of
    the destination DST and if so, b) if it's immediately followed by
    DST[CNT - 1] = '\0'.  If a) holds and b) does not, warn.  Otherwise,
@@ -3283,13 +3283,14 @@  maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi, tree src, tree cnt)
   return false;
 }
 
-/* Check the arguments to the built-in forms of stpncpy and strncpy for
-   out-of-bounds offsets or overlapping access, and to see if the size
-   is derived from calling strlen() on the source argument, and if so,
-   issue the appropriate warning.  */
+/* Check the arguments to the built-in forms of stpncpy, strncpy, and
+   strncat, for out-of-bounds offsets or overlapping access, and to see
+   if the size is derived from calling strlen() on the source argument,
+   and if so, issue the appropriate warning.
+   APPEND_P is true for strncat.  */
 
 static void
-handle_builtin_stxncpy (built_in_function, gimple_stmt_iterator *gsi)
+handle_builtin_stxncpy_strncat (bool append_p, gimple_stmt_iterator *gsi)
 {
   if (!strlen_to_stridx)
     return;
@@ -3385,7 +3386,8 @@  handle_builtin_stxncpy (built_in_function, gimple_stmt_iterator *gsi)
      whether its value is known.  Otherwise, issue the more generic
      -Wstringop-overflow which triggers for LEN arguments that in
      any meaningful way depend on strlen(SRC).  */
-  if (sisrc == silen
+  if (!append_p
+      && sisrc == silen
       && is_strlen_related_p (src, len)
       && warning_at (callloc, OPT_Wstringop_truncation,
 		     "%G%qD output truncated before terminating nul "
@@ -5352,7 +5354,7 @@  strlen_check_and_optimize_call (gimple_stmt_iterator *gsi, bool *zero_write,
     case BUILT_IN_STPNCPY_CHK:
     case BUILT_IN_STRNCPY:
     case BUILT_IN_STRNCPY_CHK:
-      handle_builtin_stxncpy (DECL_FUNCTION_CODE (callee), gsi);
+      handle_builtin_stxncpy_strncat (false, gsi);
       break;
 
     case BUILT_IN_MEMCPY: