restore -Wstringop-overflow for checked strcpy/strcat (PR 85259)

Message ID 9a8db8a9-acd7-665a-1300-a295e1945bf3@gmail.com
State New
Headers show
Series
  • restore -Wstringop-overflow for checked strcpy/strcat (PR 85259)
Related show

Commit Message

Martin Sebor May 14, 2018, 10:41 p.m.
r256683 committed to GCC 8 to avoiding duplicate instances of
-Wstringop-overflow warnings on some targets has the unintended
side-effect of suppressing even singleton instances of the warning
in cases such as 'strcat (strcpy (buf, "hello "), "world!")' when
_FORTIFY_SOURCE is defined.

The attached patch restores the warning for the trunk.

Since this is a regression in a security feature and the warning
isn't prone to false positives (I don't think I've seen any when
_FORTIFY_SOURCE is defined), I'd also like the fix considered for
the 8 branch.

Thanks
Martin

Patch

PR tree-optimization/85259 - Missing -Wstringop-overflow= since r256683

gcc/ChangeLog:

	PR tree-optimization/85259
	* builtins.c (compute_objsize): Handle constant offsets.
	* gimple-ssa-warn-restrict.c (maybe_diag_offset_bounds): Return
	true iff a warning has been issued.
	* gimple.h (gimple_nonartificial_location): New function.
	* tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Call
	gimple_nonartificial_location and handle -Wno-system-headers.
	(handle_builtin_stxncpy): Same.

gcc/testsuite/ChangeLog:

	PR tree-optimization/85259
	* gcc.dg/Wstringop-overflow-5.c: New test.
	* gcc.dg/Wstringop-overflow-6.c: New test.
Index: gcc/builtins.c
===================================================================
--- gcc/builtins.c	(revision 259298)
+++ gcc/builtins.c	(working copy)
@@ -3329,10 +3329,29 @@  compute_objsize (tree dest, int ostype)
 	{
 	  /* compute_builtin_object_size fails for addresses with
 	     non-constant offsets.  Try to determine the range of
-	     such an offset here and use it to adjus the constant
+	     such an offset here and use it to adjust the constant
 	     size.  */
 	  tree off = gimple_assign_rhs2 (stmt);
-	  if (TREE_CODE (off) == SSA_NAME
+	  if (TREE_CODE (off) == INTEGER_CST)
+	    {
+	      if (tree size = compute_objsize (dest, ostype))
+		{
+		  wide_int wioff = wi::to_wide (off);
+		  wide_int wisiz = wi::to_wide (size);
+
+		  /* Ignore negative offsets for now.  For others,
+		     use the lower bound as the most optimistic
+		     estimate of the (remaining) size.  */
+		  if (wi::sign_mask (wioff))
+		    ;
+		  else if (wi::ltu_p (wioff, wisiz))
+		    return wide_int_to_tree (TREE_TYPE (size),
+					     wi::sub (wisiz, wioff));
+		  else
+		    return size_zero_node;
+		}
+	    }
+	  else if (TREE_CODE (off) == SSA_NAME
 	      && INTEGRAL_TYPE_P (TREE_TYPE (off)))
 	    {
 	      wide_int min, max;
Index: gcc/gimple-ssa-warn-restrict.c
===================================================================
--- gcc/gimple-ssa-warn-restrict.c	(revision 259298)
+++ gcc/gimple-ssa-warn-restrict.c	(working copy)
@@ -1603,8 +1603,6 @@  maybe_diag_offset_bounds (location_t loc, gcall *c
 
   loc = expansion_point_location_if_in_system_header (loc);
 
-  tree type;
-
   char rangestr[2][64];
   if (ooboff[0] == ooboff[1]
       || (ooboff[0] != ref.offrange[0]
@@ -1615,6 +1613,8 @@  maybe_diag_offset_bounds (location_t loc, gcall *c
 	     (long long) ooboff[0].to_shwi (),
 	     (long long) ooboff[1].to_shwi ());
 
+  bool warned = false;
+
   if (oobref == error_mark_node)
     {
       if (ref.sizrange[0] == ref.sizrange[1])
@@ -1624,6 +1624,8 @@  maybe_diag_offset_bounds (location_t loc, gcall *c
 		 (long long) ref.sizrange[0].to_shwi (),
 		 (long long) ref.sizrange[1].to_shwi ());
 
+      tree type;
+
       if (DECL_P (ref.base)
 	  && TREE_CODE (type = TREE_TYPE (ref.base)) == ARRAY_TYPE)
 	{
@@ -1631,19 +1633,22 @@  maybe_diag_offset_bounds (location_t loc, gcall *c
 			  "%G%qD pointer overflow between offset %s "
 			  "and size %s accessing array %qD with type %qT",
 			  call, func, rangestr[0], rangestr[1], ref.base, type))
-	    inform (DECL_SOURCE_LOCATION (ref.base),
-		    "array %qD declared here", ref.base);
+	    {
+	      inform (DECL_SOURCE_LOCATION (ref.base),
+		      "array %qD declared here", ref.base);
+	      warned = true;
+	    }
 	  else
-	    warning_at (loc, OPT_Warray_bounds,
-			"%G%qD pointer overflow between offset %s "
-			"and size %s",
-			call, func, rangestr[0], rangestr[1]);
+	    warned = warning_at (loc, OPT_Warray_bounds,
+				 "%G%qD pointer overflow between offset %s "
+				 "and size %s",
+				 call, func, rangestr[0], rangestr[1]);
 	}
       else
-	warning_at (loc, OPT_Warray_bounds,
-		    "%G%qD pointer overflow between offset %s "
-		    "and size %s",
-		    call, func, rangestr[0], rangestr[1]);
+	warned = warning_at (loc, OPT_Warray_bounds,
+			     "%G%qD pointer overflow between offset %s "
+			     "and size %s",
+			     call, func, rangestr[0], rangestr[1]);
     }
   else if (oobref == ref.base)
     {
@@ -1674,22 +1679,26 @@  maybe_diag_offset_bounds (location_t loc, gcall *c
 				  "of object %qD with type %qT"),
 			     call, func, rangestr[0],
 			     ref.base, TREE_TYPE (ref.base)))
-	    inform (DECL_SOURCE_LOCATION (ref.base),
-		    "%qD declared here", ref.base);
+	    {
+	      inform (DECL_SOURCE_LOCATION (ref.base),
+		      "%qD declared here", ref.base);
+	      warned = true;
+	    }
 	}
       else if (ref.basesize < maxobjsize)
-	warning_at (loc, OPT_Warray_bounds,
-		    form
-		    ? G_("%G%qD forming offset %s is out of the bounds "
-			 "[0, %wu]")
-		    : G_("%G%qD offset %s is out of the bounds [0, %wu]"),
-		    call, func, rangestr[0], ref.basesize.to_uhwi ());
+	warned = warning_at (loc, OPT_Warray_bounds,
+			     form
+			     ? G_("%G%qD forming offset %s is out "
+				  "of the bounds [0, %wu]")
+			     : G_("%G%qD offset %s is out "
+				  "of the bounds [0, %wu]"),
+			     call, func, rangestr[0], ref.basesize.to_uhwi ());
       else
-	warning_at (loc, OPT_Warray_bounds,
-		    form
-		    ? G_("%G%qD forming offset %s is out of bounds")
-		    : G_("%G%qD offset %s is out of bounds"),
-		    call, func, rangestr[0]);
+	warned = warning_at (loc, OPT_Warray_bounds,
+			     form
+			     ? G_("%G%qD forming offset %s is out of bounds")
+			     : G_("%G%qD offset %s is out of bounds"),
+			     call, func, rangestr[0]);
     }
   else if (TREE_CODE (ref.ref) == MEM_REF)
     {
@@ -1698,24 +1707,25 @@  maybe_diag_offset_bounds (location_t loc, gcall *c
 	type = TREE_TYPE (type);
       type = TYPE_MAIN_VARIANT (type);
 
-      warning_at (loc, OPT_Warray_bounds,
-		  "%G%qD offset %s from the object at %qE is out "
-		  "of the bounds of %qT",
-		  call, func, rangestr[0], ref.base, type);
+      warned = warning_at (loc, OPT_Warray_bounds,
+			   "%G%qD offset %s from the object at %qE is out "
+			   "of the bounds of %qT",
+			   call, func, rangestr[0], ref.base, type);
     }
   else
     {
-      type = TYPE_MAIN_VARIANT (TREE_TYPE (ref.ref));
+      tree type = TYPE_MAIN_VARIANT (TREE_TYPE (ref.ref));
 
-      warning_at (loc, OPT_Warray_bounds,
-		"%G%qD offset %s from the object at %qE is out "
-		"of the bounds of referenced subobject %qD with type %qT "
-		"at offset %wu",
-		call, func, rangestr[0], ref.base, TREE_OPERAND (ref.ref, 1),
-		type, ref.refoff.to_uhwi ());
+      warned = warning_at (loc, OPT_Warray_bounds,
+			   "%G%qD offset %s from the object at %qE is out "
+			   "of the bounds of referenced subobject %qD with "
+			   "type %qT at offset %wu",
+			   call, func, rangestr[0], ref.base,
+			   TREE_OPERAND (ref.ref, 1), type,
+			   ref.refoff.to_uhwi ());
     }
 
-  return true;
+  return warned;
 }
 
 /* Check a CALL statement for restrict-violations and issue warnings
@@ -1840,13 +1850,8 @@  bool
 check_bounds_or_overlap (gcall *call, tree dst, tree src, tree dstsize,
 			 tree srcsize, bool bounds_only /* = false */)
 {
-  location_t loc = gimple_location (call);
-
-  if (tree block = gimple_block (call))
-    if (location_t *pbloc = block_nonartificial_location (block))
-      loc = *pbloc;
-
+  location_t loc = gimple_nonartificial_location (call);
   loc = expansion_point_location_if_in_system_header (loc);
 
   tree func = gimple_call_fndecl (call);
Index: gcc/gimple.h
===================================================================
--- gcc/gimple.h	(revision 260241)
+++ gcc/gimple.h	(working copy)
@@ -1796,7 +1796,20 @@  gimple_has_location (const gimple *g)
   return LOCATION_LOCUS (gimple_location (g)) != UNKNOWN_LOCATION;
 }
 
+/* Return non-artificial location information for statement G.  */
 
+static inline location_t
+gimple_nonartificial_location (const gimple *g)
+{
+  location_t *ploc = NULL;
+
+  if (tree block = gimple_block (g))
+    ploc = block_nonartificial_location (block);
+
+  return ploc ? *ploc : gimple_location (g);
+}
+
+
 /* Return the file name of the location of STMT.  */
 
 static inline const char *
Index: gcc/tree-ssa-strlen.c
===================================================================
--- gcc/tree-ssa-strlen.c	(revision 259298)
+++ gcc/tree-ssa-strlen.c	(working copy)
@@ -1947,7 +1947,9 @@  maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi
 	}
     }
 
-  location_t callloc = gimple_location (stmt);
+  location_t callloc = gimple_nonartificial_location (stmt);
+  callloc = expansion_point_location_if_in_system_header (callloc);
+
   tree func = gimple_call_fndecl (stmt);
 
   if (lenrange[0] != 0 || !wi::neg_p (lenrange[1]))
@@ -2132,7 +2134,8 @@  handle_builtin_stxncpy (built_in_function, gimple_
      to strlen(S)).  */
   strinfo *silen = get_strinfo (pss->first);
 
-  location_t callloc = gimple_location (stmt);
+  location_t callloc = gimple_nonartificial_location (stmt);
+  callloc = expansion_point_location_if_in_system_header (callloc);
 
   tree func = gimple_call_fndecl (stmt);
 
Index: gcc/testsuite/gcc.dg/Wstringop-overflow-5.c
===================================================================
--- gcc/testsuite/gcc.dg/Wstringop-overflow-5.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/Wstringop-overflow-5.c	(working copy)
@@ -0,0 +1,58 @@ 
+/* PR tree-optimization/85259 - Missing -Wstringop-overflow= since r256683
+   { dg-do compile }
+   { dg-options "-O2 -Wstringop-overflow" } */
+
+extern char* strcpy (char*, const char*);
+extern char* strcat (char*, const char*);
+
+char a1[1];
+char a2[2];
+char a3[3];
+char a4[4];
+char a5[5];
+char a6[6];
+char a7[7];
+char a8[8];
+
+/* Verify that at least one instance of -Wstringop-overflow is issued
+   for each pair of strcpy/strcat calls.  */
+
+void test_strcpy_strcat_1 (void)
+{
+  strcpy (a1, "1"), strcat (a1, "2");   /* { dg-warning "\\\[-Wstringop-overflow=]" } */
+}
+
+void test_strcpy_strcat_2 (void)
+{
+  strcpy (a2, "12"), strcat (a2, "3");   /* { dg-warning "\\\[-Wstringop-overflow=]" } */
+}
+
+void test_strcpy_strcat_3 (void)
+{
+  strcpy (a3, "123"), strcat (a3, "4");   /* { dg-warning "\\\[-Wstringop-overflow=]" } */
+}
+
+void test_strcpy_strcat_4 (void)
+{
+  strcpy (a4, "1234"), strcat (a4, "5");   /* { dg-warning "\\\[-Wstringop-overflow=]" } */
+}
+
+void test_strcpy_strcat_5 (void)
+{
+  strcpy (a5, "12345"), strcat (a5, "6");   /* { dg-warning "\\\[-Wstringop-overflow=]" } */
+}
+
+void test_strcpy_strcat_6 (void)
+{
+  strcpy (a6, "123456"), strcat (a6, "7");   /* { dg-warning "\\\[-Wstringop-overflow=]" } */
+}
+
+void test_strcpy_strcat_7 (void)
+{
+  strcpy (a7, "1234567"), strcat (a7, "8");   /* { dg-warning "\\\[-Wstringop-overflow=]" } */
+}
+
+void test_strcpy_strcat_8 (void)
+{
+  strcpy (a8, "12345678"), strcat (a8, "9");   /* { dg-warning "\\\[-Wstringop-overflow=]" } */
+}
Index: gcc/testsuite/gcc.dg/Wstringop-overflow-6.c
===================================================================
--- gcc/testsuite/gcc.dg/Wstringop-overflow-6.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/Wstringop-overflow-6.c	(working copy)
@@ -0,0 +1,59 @@ 
+/* PR tree-optimization/85259 - Missing -Wstringop-overflow= since r256683
+   { dg-do compile }
+   { dg-options "-O2 -Wstringop-overflow -ftrack-macro-expansion=0" } */
+
+#define bos1(p) __builtin_object_size (p, 1)
+#define strcat(d, s) __builtin___strcat_chk (d, s, bos1 (d))
+#define strcpy(d, s) __builtin___strcpy_chk (d, s, bos1 (d))
+
+char a1[1];
+char a2[2];
+char a3[3];
+char a4[4];
+char a5[5];
+char a6[6];
+char a7[7];
+char a8[8];
+
+/* Verify that at least one instance of -Wstringop-overflow is issued
+   for each pair of strcpy/strcat calls.  */
+
+void test_strcpy_strcat_1 (void)
+{
+  strcpy (a1, "1"), strcat (a1, "2");   /* { dg-warning "\\\[-Wstringop-overflow=]" } */
+}
+
+void test_strcpy_strcat_2 (void)
+{
+  strcpy (a2, "12"), strcat (a2, "3");   /* { dg-warning "\\\[-Wstringop-overflow=]" } */
+}
+
+void test_strcpy_strcat_3 (void)
+{
+  strcpy (a3, "123"), strcat (a3, "4");   /* { dg-warning "\\\[-Wstringop-overflow=]" } */
+}
+
+void test_strcpy_strcat_4 (void)
+{
+  strcpy (a4, "1234"), strcat (a4, "5");   /* { dg-warning "\\\[-Wstringop-overflow=]" } */
+}
+
+void test_strcpy_strcat_5 (void)
+{
+  strcpy (a5, "12345"), strcat (a5, "6");   /* { dg-warning "\\\[-Wstringop-overflow=]" } */
+}
+
+void test_strcpy_strcat_6 (void)
+{
+  strcpy (a6, "123456"), strcat (a6, "7");   /* { dg-warning "\\\[-Wstringop-overflow=]" } */
+}
+
+void test_strcpy_strcat_7 (void)
+{
+  strcpy (a7, "1234567"), strcat (a7, "8");   /* { dg-warning "\\\[-Wstringop-overflow=]" } */
+}
+
+void test_strcpy_strcat_8 (void)
+{
+  strcpy (a8, "12345678"), strcat (a8, "9");   /* { dg-warning "\\\[-Wstringop-overflow=]" } */
+}