public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] prevent -Wno-system-headers from suppressing -Wstringop-overflow (PR 79214)
@ 2017-01-25 21:43 Martin Sebor
  2017-01-27 21:09 ` Jeff Law
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Martin Sebor @ 2017-01-25 21:43 UTC (permalink / raw)
  To: Jeff Law, Gcc Patch List

[-- Attachment #1: Type: text/plain, Size: 742 bytes --]

While putting together examples for the GCC 7 changes document
I noticed that a few of the buffer overflow warnings issued by
-Wstringop-overflow are defeated by Glibc's macros for string
manipulation functions like strncat and strncpy.

While testing my fix I also noticed that I had missed a couple
of functions when implementing the warning: memmove and stpcpy.

The attached patch adds handlers for those and fixes the three
bugs below I raised for these omissions.

Is this patch okay for trunk?

PR preprocessor/79214 -  -Wno-system-header defeats strncat buffer
   overflow warnings
PR middle-end/79222 - missing -Wstringop-overflow= on a stpcpy overflow
PR middle-end/79223 - missing -Wstringop-overflow on a memmove overflow

Martin

[-- Attachment #2: gcc-79214.diff --]
[-- Type: text/x-patch, Size: 13966 bytes --]

PR preprocessor/79214 -  -Wno-system-header defeats strncat buffer overflow warnings
PR middle-end/79222 - missing -Wstringop-overflow= on a stpcpy overflow
PR middle-end/79223 - missing -Wstringop-overflow on a memmove overflow

gcc/ChangeLog:

	PR preprocessor/79214
	PR middle-end/79222
	PR middle-end/79223
	* builtins.c (check_sizes): Add inlinining context and issue
	warnings even when -Wno-system-headers is set.
	(check_strncat_sizes): Same.
	(expand_builtin_strncat): Same.
	(expand_builtin_memmove): New function.
	(expand_builtin_stpncpy): Same.
	(expand_builtin): Handle memmove and stpncpy.

gcc/testsuite/ChangeLog:

	PR preprocessor/79214
	PR middle-end/79222
	PR middle-end/79223
	* gcc.dg/pr79214.c: New test.
	* gcc.dg/pr79214.h: New test header.
	* gcc.dg/pr79222.c: New test.
	* gcc.dg/pr79223.c: New test.
	* gcc.dg/pr78138.c: Adjust.

Index: gcc/builtins.c
===================================================================
--- gcc/builtins.c	(revision 244844)
+++ gcc/builtins.c	(working copy)
@@ -121,6 +121,7 @@ static rtx builtin_memcpy_read_str (void *, HOST_W
 static rtx expand_builtin_memcpy (tree, rtx);
 static rtx expand_builtin_memcpy_with_bounds (tree, rtx);
 static rtx expand_builtin_memcpy_args (tree, tree, tree, rtx, tree);
+static rtx expand_builtin_memmove (tree, rtx);
 static rtx expand_builtin_mempcpy (tree, rtx, machine_mode);
 static rtx expand_builtin_mempcpy_with_bounds (tree, rtx, machine_mode);
 static rtx expand_builtin_mempcpy_args (tree, tree, tree, rtx,
@@ -129,6 +130,7 @@ static rtx expand_builtin_strcat (tree, rtx);
 static rtx expand_builtin_strcpy (tree, rtx);
 static rtx expand_builtin_strcpy_args (tree, tree, rtx);
 static rtx expand_builtin_stpcpy (tree, rtx, machine_mode);
+static rtx expand_builtin_stpncpy (tree, rtx);
 static rtx expand_builtin_strncat (tree, rtx);
 static rtx expand_builtin_strncpy (tree, rtx);
 static rtx builtin_memset_gen_str (void *, HOST_WIDE_INT, machine_mode);
@@ -3123,6 +3125,7 @@ check_sizes (int opt, tree exp, tree size, tree ma
   if (range[0] && tree_int_cst_lt (maxobjsize, range[0]))
     {
       location_t loc = tree_nonartificial_location (exp);
+      loc = expansion_point_location_if_in_system_header (loc);
 
       if (range[0] == range[1])
 	warning_at (loc, opt,
@@ -3155,10 +3158,11 @@ check_sizes (int opt, tree exp, tree size, tree ma
 	  unsigned HOST_WIDE_INT uwir0 = tree_to_uhwi (range[0]);
 
 	  location_t loc = tree_nonartificial_location (exp);
+	  loc = expansion_point_location_if_in_system_header (loc);
 
 	  if (at_least_one)
 	    warning_at (loc, opt,
-			"%K%qD: writing at least %wu byte into a region "
+			"%K%qD writing at least %wu byte into a region "
 			"of size %wu overflows the destination",
 			exp, get_callee_fndecl (exp), uwir0,
 			tree_to_uhwi (objsize));
@@ -3165,7 +3169,7 @@ check_sizes (int opt, tree exp, tree size, tree ma
 	  else if (range[0] == range[1])
 	    warning_at (loc, opt,
 			(uwir0 == 1
-			 ? G_("%K%qD: writing %wu byte into a region "
+			 ? G_("%K%qD writing %wu byte into a region "
 			      "of size %wu overflows the destination")
 			 : G_("%K%qD writing %wu bytes into a region "
 			      "of size %wu overflows the destination")),
@@ -3173,7 +3177,7 @@ check_sizes (int opt, tree exp, tree size, tree ma
 			tree_to_uhwi (objsize));
 	  else
 	    warning_at (loc, opt,
-			"%K%qD: writing between %wu and %wu bytes "
+			"%K%qD writing between %wu and %wu bytes "
 			"into a region of size %wu overflows "
 			"the destination",
 			exp, get_callee_fndecl (exp), uwir0,
@@ -3194,6 +3198,7 @@ check_sizes (int opt, tree exp, tree size, tree ma
       if (range[0] && objsize && tree_fits_uhwi_p (objsize))
 	{
 	  location_t loc = tree_nonartificial_location (exp);
+	  loc = expansion_point_location_if_in_system_header (loc);
 
 	  if (tree_int_cst_lt (maxobjsize, range[0]))
 	    {
@@ -3302,6 +3307,24 @@ expand_builtin_memcpy (tree exp, rtx target)
   return expand_builtin_memcpy_args (dest, src, len, target, exp);
 }
 
+/* Check a call EXP to the memmove built-in for validity.
+   Return NULL_RTX on both success and failure.  */
+
+static rtx
+expand_builtin_memmove (tree exp, rtx)
+{
+  if (!validate_arglist (exp,
+ 			 POINTER_TYPE, POINTER_TYPE, INTEGER_TYPE, VOID_TYPE))
+    return NULL_RTX;
+
+  tree dest = CALL_EXPR_ARG (exp, 0);
+  tree len = CALL_EXPR_ARG (exp, 2);
+
+  check_memop_sizes (exp, dest, len);
+
+  return NULL_RTX;
+}
+
 /* Expand an instrumented call EXP to the memcpy builtin.
    Return NULL_RTX if we failed, the caller should emit a normal call,
    otherwise try to get the result in TARGET, if convenient (and in
@@ -3612,6 +3635,13 @@ expand_builtin_stpcpy (tree exp, rtx target, machi
   dst = CALL_EXPR_ARG (exp, 0);
   src = CALL_EXPR_ARG (exp, 1);
 
+  if (warn_stringop_overflow)
+    {
+      tree destsize = compute_dest_size (dst, warn_stringop_overflow - 1);
+      check_sizes (OPT_Wstringop_overflow_,
+		   exp, /*size=*/NULL_TREE, /*maxlen=*/NULL_TREE, src, destsize);
+    }
+
   /* If return value is ignored, transform stpcpy into strcpy.  */
   if (target == const0_rtx && builtin_decl_implicit (BUILT_IN_STRCPY))
     {
@@ -3672,6 +3702,47 @@ expand_builtin_stpcpy (tree exp, rtx target, machi
     }
 }
 
+/* Check a call EXP to the stpncpy built-in for validity.
+   Return NULL_RTX on both success and failure.  */
+
+static rtx
+expand_builtin_stpncpy (tree exp, rtx)
+{
+  if (!validate_arglist (exp,
+			 POINTER_TYPE, POINTER_TYPE, INTEGER_TYPE, VOID_TYPE)
+      || !warn_stringop_overflow)
+    return NULL_RTX;
+
+  tree dest = CALL_EXPR_ARG (exp, 0);
+  tree src = CALL_EXPR_ARG (exp, 1);
+
+  /* The number of bytes to write (not the maximum).  */
+  tree len = CALL_EXPR_ARG (exp, 2);
+  /* The length of the source sequence.  */
+  tree slen = c_strlen (src, 1);
+
+  /* Try to determine the range of lengths that the source expression
+     refers to.  */
+  tree lenrange[2];
+  if (slen)
+    lenrange[0] = lenrange[1] = slen;
+  else
+    {
+      get_range_strlen (src, lenrange);
+      slen = lenrange[0];
+    }
+
+  tree destsize = compute_dest_size (dest,
+				     warn_stringop_overflow - 1);
+
+  /* The number of bytes to write is LEN but check_sizes will also
+     check SLEN if LEN's value isn't known.  */
+  check_sizes (OPT_Wstringop_overflow_,
+	       exp, len, /*maxlen=*/NULL_TREE, slen, destsize);
+
+  return NULL_RTX;
+}
+
 /* Callback routine for store_by_pieces.  Read GET_MODE_BITSIZE (MODE)
    bytes from constant string DATA + OFFSET and return it as target
    constant.  */
@@ -3727,9 +3798,13 @@ check_strncat_sizes (tree exp, tree objsize)
   if (tree_fits_uhwi_p (maxlen) && tree_fits_uhwi_p (objsize)
       && tree_int_cst_equal (objsize, maxlen))
     {
-      warning_at (EXPR_LOCATION (exp), OPT_Wstringop_overflow_,
-		  "specified bound %wu "
+      location_t loc = tree_nonartificial_location (exp);
+      loc = expansion_point_location_if_in_system_header (loc);
+
+      warning_at (loc, OPT_Wstringop_overflow_,
+		  "%K%qD: specified bound %wu "
 		  "equals the size of the destination",
+		  exp, get_callee_fndecl (exp),
 		  tree_to_uhwi (maxlen));
 
       return false;
@@ -3791,9 +3866,13 @@ expand_builtin_strncat (tree exp, rtx)
   if (tree_fits_uhwi_p (maxlen) && tree_fits_uhwi_p (destsize)
       && tree_int_cst_equal (destsize, maxlen))
     {
-      warning_at (EXPR_LOCATION (exp), OPT_Wstringop_overflow_,
-		  "specified bound %wu "
+      location_t loc = tree_nonartificial_location (exp);
+      loc = expansion_point_location_if_in_system_header (loc);
+
+      warning_at (loc, OPT_Wstringop_overflow_,
+		  "%K%qD: specified bound %wu "
 		  "equals the size of the destination",
+		  exp, get_callee_fndecl (exp),
 		  tree_to_uhwi (maxlen));
 
       return NULL_RTX;
@@ -6709,6 +6788,12 @@ expand_builtin (tree exp, rtx target, rtx subtarge
 	return target;
       break;
 
+    case BUILT_IN_STPNCPY:
+      target = expand_builtin_stpncpy (exp, target);
+      if (target)
+	return target;
+      break;
+
     case BUILT_IN_MEMCPY:
       target = expand_builtin_memcpy (exp, target);
       if (target)
@@ -6715,6 +6800,12 @@ expand_builtin (tree exp, rtx target, rtx subtarge
 	return target;
       break;
 
+    case BUILT_IN_MEMMOVE:
+      target = expand_builtin_memmove (exp, target);
+      if (target)
+	return target;
+      break;
+
     case BUILT_IN_MEMPCPY:
       target = expand_builtin_mempcpy (exp, target, mode);
       if (target)
Index: gcc/testsuite/gcc.dg/pr79214.c
===================================================================
--- gcc/testsuite/gcc.dg/pr79214.c	(revision 0)
+++ gcc/testsuite/gcc.dg/pr79214.c	(working copy)
@@ -0,0 +1,88 @@
+/* PR preprocessor/79214 - -Wno-system-header defeats strncat buffer overflow
+   warnings
+   { dg-do compile }
+   { dg-options "-O2" } */
+
+#include "pr79214.h"
+
+typedef __SIZE_TYPE__ size_t;
+
+char d[3];
+char s[4];
+
+size_t range (void)
+{
+  extern size_t size ();
+  size_t n = size ();
+  if (n <= sizeof d)
+    return sizeof d + 1;
+
+  return n;
+}
+
+void test_bzero (void)
+{
+  bzero (d, range ());   /* { dg-warning ".__builtin_bzero. writing between 4 and \[0-9\]+ bytes into a region of size 3 overflows the destination" } */
+}
+
+void test_memcpy (void)
+{
+  memcpy (d, s, range ());   /* { dg-warning ".__builtin_memcpy. writing between 4 and \[0-9\]+ bytes into a region of size 3 overflows the destination" } */
+}
+
+void test_memmove (void)
+{
+  memmove (d, d + 1, range ());   /* { dg-warning ".__builtin_memmove. writing between 4 and \[0-9\]+ bytes into a region of size 3 overflows the destination" } */
+}
+
+void test_mempcpy (void)
+{
+  mempcpy (d, s, range ());   /* { dg-warning ".__builtin_mempcpy. writing between 4 and \[0-9\]+ bytes into a region of size 3 overflows the destination" } */
+}
+
+void test_memset (int n)
+{
+  memset (d, n, range ());   /* { dg-warning ".__builtin_memset. writing between 4 and \[0-9\]+ bytes into a region of size 3 overflows the destination" } */
+}
+
+void test_strcat (int i)
+{
+  const char *s = i < 0 ? "123" : "4567";
+
+  strcat (d, s);   /* { dg-warning ".__builtin_strcat. writing 4 bytes into a region of size 3 overflows the destination" } */
+}
+
+char* test_stpcpy (int i)
+{
+  const char *s = i < 0 ? "123" : "4567";
+
+  return stpcpy (d, s);   /* { dg-warning ".__builtin_stpcpy. writing 4 bytes into a region of size 3 overflows the destination" } */
+}
+
+char* test_stpncpy (int i)
+{
+  const char *s = i < 0 ? "123" : "4567";
+
+  return stpncpy (d, s, range ());   /* { dg-warning ".__builtin_stpncpy. writing between 4 and \[0-9\]+ bytes into a region of size 3 overflows the destination" } */
+}
+
+char* test_strcpy (int i)
+{
+  const char *s = i < 0 ? "123" : "4567";
+
+  return strcpy (d, s);   /* { dg-warning ".__builtin_strcpy. writing 4 bytes into a region of size 3 overflows the destination" } */
+}
+
+char* test_strncpy (int i)
+{
+  const char *s = i < 0 ? "123" : "4567";
+
+  return strncpy (d, s, range ());   /* { dg-warning ".__builtin_strncpy. writing between 4 and \[0-9\]+ bytes into a region of size 3 overflows the destination" } */
+}
+
+char* test_strncat (int i)
+{
+  const char *s = i < 0 ? "123" : "4567";
+
+  return strncat (d, s, range ());   /* { dg-warning ".__builtin_strncat.: specified bound between 4 and \[0-9\]+" } */
+}
Index: gcc/testsuite/gcc.dg/pr79214.h
===================================================================
--- gcc/testsuite/gcc.dg/pr79214.h	(revision 0)
+++ gcc/testsuite/gcc.dg/pr79214.h	(working copy)
@@ -0,0 +1,13 @@
+#pragma GCC system_header
+
+#define bzero    __builtin_bzero
+#define memcpy   __builtin_memcpy
+#define memmove  __builtin_memmove
+#define mempcpy  __builtin_mempcpy
+#define memset   __builtin_memset
+#define strcat   __builtin_strcat
+#define stpcpy   __builtin_stpcpy
+#define stpncpy  __builtin_stpncpy
+#define strcpy   __builtin_strcpy
+#define strncpy  __builtin_strncpy
+#define strncat  __builtin_strncat
Index: gcc/testsuite/gcc.dg/pr79222.c
===================================================================
--- gcc/testsuite/gcc.dg/pr79222.c	(revision 0)
+++ gcc/testsuite/gcc.dg/pr79222.c	(working copy)
@@ -0,0 +1,13 @@
+/* PR middle-end/79222 - missing -Wstringop-overflow= on a stpcpy overflow
+   { dg-do compile }
+   { dg-options "-O2" } */
+
+extern char* stpcpy (char*, const char*);
+
+char d[3];
+
+char* f (int i)
+{
+  const char *s = i < 0 ? "01234567" : "9876543210";
+  return stpcpy (d, s);   /* { dg-warning ".stpcpy. writing 9 bytes into a region of size 3 overflows the destination" } */
+}
Index: gcc/testsuite/gcc.dg/pr79223.c
===================================================================
--- gcc/testsuite/gcc.dg/pr79223.c	(revision 0)
+++ gcc/testsuite/gcc.dg/pr79223.c	(working copy)
@@ -0,0 +1,25 @@
+/* PR middle-end/79223 - missing -Wstringop-overflow on a memmove overflow
+   { dg-do compile }
+   { dg-additional-options "-O2 -Wall" }  */
+
+typedef __SIZE_TYPE__ size_t;
+
+extern void* memcpy (void*, const void*, size_t);
+
+char d[3];
+char s[4];
+
+size_t range (void)
+{
+  extern size_t size ();
+  size_t n = size ();
+  if (n <= sizeof d)
+    return sizeof d + 1;
+
+  return n;
+}
+
+void test_memcpy (void)
+{
+  memcpy (d, s, range ());   /* { dg-warning ".memcpy. writing between 4 and \[0-9\]+ bytes into a region of size 3 overflows the destination" } */
+}
Index: gcc/testsuite/gcc.dg/pr78138.c
===================================================================
--- gcc/testsuite/gcc.dg/pr78138.c	(revision 244844)
+++ gcc/testsuite/gcc.dg/pr78138.c	(working copy)
@@ -18,5 +18,5 @@ void g (void *p)
   extern unsigned n;
   if (n < 17 || 32 < n) n = 7;
 
-  memcpy (d, p, n);   /* { dg-warning ".memcpy.: writing between 7 and 32 bytes into a region of size 5" } */
+  memcpy (d, p, n);   /* { dg-warning ".memcpy. writing between 7 and 32 bytes into a region of size 5" } */
 };

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH] prevent -Wno-system-headers from suppressing -Wstringop-overflow (PR 79214)
@ 2017-05-05 21:22 David Edelsohn
  2017-05-05 21:27 ` Martin Sebor
  0 siblings, 1 reply; 13+ messages in thread
From: David Edelsohn @ 2017-05-05 21:22 UTC (permalink / raw)
  To: Martin Sebor; +Cc: Jeffrey Law, GCC Patches, Andreas Schwab

>>>>> Andreas Schwab wrote:

> I see this failure on aarch64 with -mabi=ilp32:

> FAIL: gfortran.dg/alloc_comp_auto_array_2.f90   -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  (test for excess errors)
> Excess errors:
> /opt/gcc/gcc-20170505/gcc/testsuite/gfortran.dg/alloc_comp_auto_array_2.f90:33:0: Warning: '__builtin_memcpy': specified size between 2147483648 and 4294967295 exceeds maximum object size 2147483647 [-Wstringop-overflow=]

I'm seeing a huge number of similar, new testsuite failures on AIX
that all match the testcases from the PR79214 commit:

pr79138.c
pr79214.c
pr79222.c
pr79223.c
unconstrained_commons.c

AIX defaults to 32 bit.  Do these testcases assume 64 bit types?  I
believe that all of the matching text of for the warning messages all
assume 64 bit sizes.

Thanks, David

^ permalink raw reply	[flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-05-10  8:26 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-25 21:43 [PATCH] prevent -Wno-system-headers from suppressing -Wstringop-overflow (PR 79214) Martin Sebor
2017-01-27 21:09 ` Jeff Law
2017-05-04 19:27 ` Jeff Law
2017-05-04 21:14   ` Martin Sebor
2017-05-05  2:25     ` Jeff Law
2017-05-05  7:10       ` Jakub Jelinek
2017-05-05 20:07 ` Andreas Schwab
2017-05-08  8:28 ` Andreas Schwab
2017-05-09 20:03   ` Rainer Orth
2017-05-09 20:49     ` Martin Sebor
2017-05-10  8:26       ` Rainer Orth
2017-05-05 21:22 David Edelsohn
2017-05-05 21:27 ` Martin Sebor

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).