diff mbox series

avoid a spurious -Wstringop-overflow due to multiple MEM_REFs (PR 92014)

Message ID 71b5c989-dd1f-d753-505a-30618448a6df@gmail.com
State New
Headers show
Series avoid a spurious -Wstringop-overflow due to multiple MEM_REFs (PR 92014) | expand

Commit Message

Martin Sebor Oct. 8, 2019, 12:58 a.m. UTC 
Last week's enhancement to detect one-byte buffer overflows exposed
a bug that let the warning use the size of a prior MEM_REF access
and "override" the size of the actual store to the character array.
When the store was smaller than the prior access (e.g., one byte,
vs an 8-byte null pointer read such as in a PHI), this would lead
to a false positive.

The attached patch has the function fail after it has determined
the size of the store from a MEM_REF if one of its recursive
invocations finds another MEM_REF.

Tested on x86_64-linux.  Since the bug is causing trouble in Glibc
builds I will plan on committing the fix tomorrow.

Martin

Comments

Martin Sebor Oct. 8, 2019, 7:50 p.m. UTC | #1 
On 10/7/19 6:58 PM, Martin Sebor wrote:
> Last week's enhancement to detect one-byte buffer overflows exposed
> a bug that let the warning use the size of a prior MEM_REF access
> and "override" the size of the actual store to the character array.
> When the store was smaller than the prior access (e.g., one byte,
> vs an 8-byte null pointer read such as in a PHI), this would lead
> to a false positive.
> 
> The attached patch has the function fail after it has determined
> the size of the store from a MEM_REF if one of its recursive
> invocations finds another MEM_REF.
> 
> Tested on x86_64-linux.  Since the bug is causing trouble in Glibc
> builds I will plan on committing the fix tomorrow.

I have committed this patch in r276711 along with an additional
minor tweak to take care of bug 92026 that was raised overnight
for test suite failures on a few targets.

Martin
diff mbox series

Patch

PR middle-end/92014 - bogus warning: writing 8 bytes into a region of size 1 in timezone/zic.c

gcc/ChangeLog:

	PR middle-end/92014
	* tree-ssa-strlen.c (count_nonzero_bytes): Avoid recursing for MEM_REF
	again once nbytes has been set .

gcc/testsuite/ChangeLog:

	PR middle-end/92014
	* gcc.dg/Wstringop-overflow-19.c: New test.

Index: gcc/testsuite/gcc.dg/Wstringop-overflow-19.c
===================================================================
--- gcc/testsuite/gcc.dg/Wstringop-overflow-19.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/Wstringop-overflow-19.c	(working copy)
@@ -0,0 +1,27 @@ 
+/* PR middle-end/92014 - bogus warning: writing 8 bytes into a region
+   of size 1 in timezone/zic.c
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+struct
+{
+  char *s1, *s2;
+  char c;
+} z;
+
+
+void f (char **a, int i, int j)
+{
+  char * cp = __builtin_strchr (a[i], '%');
+
+  if (cp && *++cp != 's')
+    return;
+
+  z.s1 = __builtin_strdup (a[i]);
+  if (!z.s1) __builtin_abort ();
+
+  z.s2 = __builtin_strdup (a[j]);
+  if (!z.s2) __builtin_abort ();
+
+  z.c = cp ? *cp : '\0';    // { dg-bogus "\\\[-Wstringop-overflow" }
+}
Index: gcc/tree-ssa-strlen.c
===================================================================
--- gcc/tree-ssa-strlen.c	(revision 276657)
+++ gcc/tree-ssa-strlen.c	(working copy)
@@ -3741,13 +3741,16 @@  int ssa_name_limit_t::next_ssa_name (tree ssa_name
   return 0;
 }
 
-/* Determine the minimum and maximum number of leading non-zero bytes
+/* Determines the minimum and maximum number of leading non-zero bytes
    in the representation of EXP and set LENRANGE[0] and LENRANGE[1]
-   to each.  Set LENRANGE[2] to the total number of bytes in
-   the representation.  Set *NULTREM if the representation contains
-   a zero byte, and set *ALLNUL if all the bytes are zero.  Avoid
-   recursing deeper than the limits in SNLIM allow.  Return true
-   on success and false otherwise.  */
+   to each.  Sets LENRANGE[2] to the total number of bytes in
+   the representation.  Sets *NULTREM if the representation contains
+   a zero byte, and sets *ALLNUL if all the bytes are zero.
+   OFFSET and NBYTES are the offset into the representation and
+   the size of the access to it determined from a MEM_REF or zero
+   for other expressions.
+   Avoid recursing deeper than the limits in SNLIM allow.
+   Returns true on success and false otherwise.  */
 
 static bool
 count_nonzero_bytes (tree exp, unsigned HOST_WIDE_INT offset,
@@ -3843,6 +3846,9 @@  count_nonzero_bytes (tree exp, unsigned HOST_WIDE_
 
   if (TREE_CODE (exp) == MEM_REF)
     {
+      if (nbytes)
+	return false;
+
       tree arg = TREE_OPERAND (exp, 0);
       tree off = TREE_OPERAND (exp, 1);
 
@@ -3910,8 +3916,10 @@  count_nonzero_bytes (tree exp, unsigned HOST_WIDE_
 	  lenrange[0] = 0;
 	  prep = NULL;
 	}
-      else
+      else if (!nbytes)
 	nbytes = repsize;
+      else if (nbytes < repsize)
+	return false;
     }
 
   if (!nbytes)