diff mbox series

use get_size_range to get allocated size (PR 92942)

Message ID 6d21c9c1-c770-ac6a-5af5-926acdadc898@gmail.com
State New
Headers show
Series use get_size_range to get allocated size (PR 92942) | expand

Commit Message

Martin Sebor Aug. 28, 2020, 5:12 p.m. UTC
The gimple_call_alloc_size() function that determines the range
of sizes of allocated objects and constrains the bounds in calls
to functions like memcpy calls get_range() instead of
get_size_range() to obtain its result.  The latter is the right
function to call because it has the necessary logic to constrain
the range to just the values that are valid for object sizes.
This is especially useful when the range is the result of
a conversion from a signed to a wider unsigned integer where
the upper subrange is excessive and can be eliminated such as in:

   char* f (int n)
   {
     if (n > 8)
       n = 8;
     char *p = malloc (n);
     strcpy (p, "0123456789");   // buffer overflow
     ...
   }

Attached is a fix that lets -Wstringop-overflow diagnose the buffer
overflow above.  Besides with GCC I have also tested the change by
building Binutils/GDB and Glibc and verifying that it doesn't
introduce any false positives.

Martin

Comments

Martin Sebor Sept. 9, 2020, 9:42 p.m. UTC | #1
Ping: https://gcc.gnu.org/pipermail/gcc-patches/2020-August/552903.html

On 8/28/20 11:12 AM, Martin Sebor wrote:
> The gimple_call_alloc_size() function that determines the range
> of sizes of allocated objects and constrains the bounds in calls
> to functions like memcpy calls get_range() instead of
> get_size_range() to obtain its result.  The latter is the right
> function to call because it has the necessary logic to constrain
> the range to just the values that are valid for object sizes.
> This is especially useful when the range is the result of
> a conversion from a signed to a wider unsigned integer where
> the upper subrange is excessive and can be eliminated such as in:
> 
>    char* f (int n)
>    {
>      if (n > 8)
>        n = 8;
>      char *p = malloc (n);
>      strcpy (p, "0123456789");   // buffer overflow
>      ...
>    }
> 
> Attached is a fix that lets -Wstringop-overflow diagnose the buffer
> overflow above.  Besides with GCC I have also tested the change by
> building Binutils/GDB and Glibc and verifying that it doesn't
> introduce any false positives.
> 
> Martin
Martin Sebor Sept. 30, 2020, 10:25 p.m. UTC | #2
Ping: https://gcc.gnu.org/pipermail/gcc-patches/2020-August/552903.html

(I lost track of this patch.)

On 9/9/20 3:42 PM, Martin Sebor wrote:
> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2020-August/552903.html
> 
> On 8/28/20 11:12 AM, Martin Sebor wrote:
>> The gimple_call_alloc_size() function that determines the range
>> of sizes of allocated objects and constrains the bounds in calls
>> to functions like memcpy calls get_range() instead of
>> get_size_range() to obtain its result.  The latter is the right
>> function to call because it has the necessary logic to constrain
>> the range to just the values that are valid for object sizes.
>> This is especially useful when the range is the result of
>> a conversion from a signed to a wider unsigned integer where
>> the upper subrange is excessive and can be eliminated such as in:
>>
>>    char* f (int n)
>>    {
>>      if (n > 8)
>>        n = 8;
>>      char *p = malloc (n);
>>      strcpy (p, "0123456789");   // buffer overflow
>>      ...
>>    }
>>
>> Attached is a fix that lets -Wstringop-overflow diagnose the buffer
>> overflow above.  Besides with GCC I have also tested the change by
>> building Binutils/GDB and Glibc and verifying that it doesn't
>> introduce any false positives.
>>
>> Martin
>
Martin Sebor Oct. 27, 2020, 2:38 p.m. UTC | #3
This patch was never reviewed and I forgot all about it but I wound
up reimplementing the same solution in r11-3827, so I just committed
the tests from this one in r11-4441.

On 9/30/20 4:25 PM, Martin Sebor wrote:
> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2020-August/552903.html
> 
> (I lost track of this patch.)
> 
> On 9/9/20 3:42 PM, Martin Sebor wrote:
>> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2020-August/552903.html
>>
>> On 8/28/20 11:12 AM, Martin Sebor wrote:
>>> The gimple_call_alloc_size() function that determines the range
>>> of sizes of allocated objects and constrains the bounds in calls
>>> to functions like memcpy calls get_range() instead of
>>> get_size_range() to obtain its result.  The latter is the right
>>> function to call because it has the necessary logic to constrain
>>> the range to just the values that are valid for object sizes.
>>> This is especially useful when the range is the result of
>>> a conversion from a signed to a wider unsigned integer where
>>> the upper subrange is excessive and can be eliminated such as in:
>>>
>>>    char* f (int n)
>>>    {
>>>      if (n > 8)
>>>        n = 8;
>>>      char *p = malloc (n);
>>>      strcpy (p, "0123456789");   // buffer overflow
>>>      ...
>>>    }
>>>
>>> Attached is a fix that lets -Wstringop-overflow diagnose the buffer
>>> overflow above.  Besides with GCC I have also tested the change by
>>> building Binutils/GDB and Glibc and verifying that it doesn't
>>> introduce any false positives.
>>>
>>> Martin
>>
>
Jeff Law Nov. 6, 2020, 4:35 p.m. UTC | #4
On 8/28/20 11:12 AM, Martin Sebor via Gcc-patches wrote:
> The gimple_call_alloc_size() function that determines the range
> of sizes of allocated objects and constrains the bounds in calls
> to functions like memcpy calls get_range() instead of
> get_size_range() to obtain its result.  The latter is the right
> function to call because it has the necessary logic to constrain
> the range to just the values that are valid for object sizes.
> This is especially useful when the range is the result of
> a conversion from a signed to a wider unsigned integer where
> the upper subrange is excessive and can be eliminated such as in:
>
>   char* f (int n)
>   {
>     if (n > 8)
>       n = 8;
>     char *p = malloc (n);
>     strcpy (p, "0123456789");   // buffer overflow
>     ...
>   }
>
> Attached is a fix that lets -Wstringop-overflow diagnose the buffer
> overflow above.  Besides with GCC I have also tested the change by
> building Binutils/GDB and Glibc and verifying that it doesn't
> introduce any false positives.
>
> Martin
>
> gcc-92942.diff
>
> PR middle-end/92942 - missing -Wstringop-overflow for allocations with a negative lower bound size
>
> gcc/ChangeLog:
>
> 	PR middle-end/92942
> 	* builtins.c (gimple_call_alloc_size): Call get_size_range instead
> 	of get_range.
> 	* calls.c (get_size_range): Define new overload.  Handle anti-ranges
> 	  whose upper part is with the valid size range.
> 	* calls.h (get_size_range): Declare new overload.
>
> gcc/testsuite/ChangeLog:
>
> 	PR middle-end/92942
> 	* gcc.dg/Wstringop-overflow-40.c: New test.
> 	* gcc.dg/Wstringop-overflow-41.c: New test.
> 	* gcc.dg/attr-alloc_size-10.c: Disable macro tracking.

Please re-rest and once re-validated, this is fine for the trunk.


jeff
diff mbox series

Patch

PR middle-end/92942 - missing -Wstringop-overflow for allocations with a negative lower bound size

gcc/ChangeLog:

	PR middle-end/92942
	* builtins.c (gimple_call_alloc_size): Call get_size_range instead
	of get_range.
	* calls.c (get_size_range): Define new overload.  Handle anti-ranges
	  whose upper part is with the valid size range.
	* calls.h (get_size_range): Declare new overload.

gcc/testsuite/ChangeLog:

	PR middle-end/92942
	* gcc.dg/Wstringop-overflow-40.c: New test.
	* gcc.dg/Wstringop-overflow-41.c: New test.
	* gcc.dg/attr-alloc_size-10.c: Disable macro tracking.

diff --git a/gcc/builtins.c b/gcc/builtins.c
index 8845816aebd..4caa02f9a3b 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -3884,7 +3884,7 @@  check_access (tree exp, tree, tree, tree dstwrite,
 
 tree
 gimple_call_alloc_size (gimple *stmt, wide_int rng1[2] /* = NULL */,
-			const vr_values *rvals /* = NULL */)
+			const vr_values * /* = NULL */)
 {
   if (!stmt)
     return NULL_TREE;
@@ -3936,7 +3936,7 @@  gimple_call_alloc_size (gimple *stmt, wide_int rng1[2] /* = NULL */,
   if (!rng1)
     rng1 = rng1_buf;
 
-  if (!get_range (size, rng1, rvals))
+  if (!get_size_range (size, rng1))
     return NULL_TREE;
 
   if (argidx2 > nargs && TREE_CODE (size) == INTEGER_CST)
@@ -3946,7 +3946,7 @@  gimple_call_alloc_size (gimple *stmt, wide_int rng1[2] /* = NULL */,
      of the upper bounds as a constant.  Ignore anti-ranges.  */
   tree n = argidx2 < nargs ? gimple_call_arg (stmt, argidx2) : integer_one_node;
   wide_int rng2[2];
-  if (!get_range (n, rng2, rvals))
+  if (!get_size_range (n, rng2))
     return NULL_TREE;
 
   /* Extend to the maximum precision to avoid overflow.  */
diff --git a/gcc/calls.c b/gcc/calls.c
index a11da66492d..6c59b82dd78 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -1238,17 +1238,17 @@  alloc_max_size (void)
 }
 
 /* Return true when EXP's range can be determined and set RANGE[] to it
-   after adjusting it if necessary to make EXP a represents a valid size
-   of object, or a valid size argument to an allocation function declared
-   with attribute alloc_size (whose argument may be signed), or to a string
-   manipulation function like memset.  When ALLOW_ZERO is true, allow
-   returning a range of [0, 0] for a size in an anti-range [1, N] where
-   N > PTRDIFF_MAX.  A zero range is a (nearly) invalid argument to
-   allocation functions like malloc but it is a valid argument to
-   functions like memset.  */
+   after adjusting it if necessary to make EXP represent a valid size
+   of an object, or a valid size argument to an allocation function
+   declared with attribute alloc_size (whose argument may be signed),
+   or to a string manipulation function like memset.  When ALLOW_ZERO
+   is true, allow returning a range of [0, 0] for a size in an anti-range
+   [1, N] where N >= PTRDIFF_MAX.  A zero range is a (nearly) invalid
+   argument to allocation functions like malloc but it is a valid argument
+   to functions like memset.  */
 
 bool
-get_size_range (tree exp, tree range[2], bool allow_zero /* = false */)
+get_size_range (tree exp, wide_int range[2], bool allow_zero /* = false */)
 {
   if (!exp)
     return false;
@@ -1256,7 +1256,7 @@  get_size_range (tree exp, tree range[2], bool allow_zero /* = false */)
   if (tree_fits_uhwi_p (exp))
     {
       /* EXP is a constant.  */
-      range[0] = range[1] = exp;
+      range[0] = range[1] = wi::to_wide (exp);
       return true;
     }
 
@@ -1277,13 +1277,11 @@  get_size_range (tree exp, tree range[2], bool allow_zero /* = false */)
 	{
 	  /* Use the full range of the type of the expression when
 	     no value range information is available.  */
-	  range[0] = TYPE_MIN_VALUE (exptype);
-	  range[1] = TYPE_MAX_VALUE (exptype);
+	  range[0] = wi::to_wide (TYPE_MIN_VALUE (exptype));
+	  range[1] = wi::to_wide (TYPE_MAX_VALUE (exptype));
 	  return true;
 	}
 
-      range[0] = NULL_TREE;
-      range[1] = NULL_TREE;
       return false;
     }
 
@@ -1337,14 +1335,46 @@  get_size_range (tree exp, tree range[2], bool allow_zero /* = false */)
 	}
       else
 	{
-	  max = min - 1;
-	  min = wi::zero (expprec);
+	  /* Make sure both operands have the same precision to keep
+	     wide_int from helpfully aborting.  */
+	  wide_int maxsz = wi::to_wide (max_object_size ());
+	  max = wide_int::from (max, maxsz.get_precision (), UNSIGNED);
+	  if (wi::geu_p (max, maxsz))
+	    {
+	      /* EXP is unsigned and the upper part of the range is
+		 in excess of maximum object size.  */
+	      max = min - 1;
+	      min = wi::zero (expprec);
+	    }
+	  else
+	    {
+	      min = wi::zero (expprec);
+	      max = wi::to_wide (TYPE_MAX_VALUE (exptype));
+	    }
 	}
     }
 
-  range[0] = wide_int_to_tree (exptype, min);
-  range[1] = wide_int_to_tree (exptype, max);
+  range[0] = min;
+  range[1] = max;
+  return true;
+}
+
+
+/* Convenience overload for the get_size_range above.  */
+
+bool
+get_size_range (tree exp, tree range[2], bool allow_zero /* = false */)
+{
+  wide_int rng[2];
+  if (!get_size_range (exp, rng, allow_zero))
+    {
+      range[0] = range[1] = NULL_TREE;
+      return false;
+    }
 
+  tree type = TREE_TYPE (exp);
+  range[0] = wide_int_to_tree (type, rng[0]);
+  range[1] = wide_int_to_tree (type, rng[1]);
   return true;
 }
 
diff --git a/gcc/calls.h b/gcc/calls.h
index 4ee49360777..913f879bb42 100644
--- a/gcc/calls.h
+++ b/gcc/calls.h
@@ -133,7 +133,10 @@  extern bool reference_callee_copied (CUMULATIVE_ARGS *,
 extern void maybe_warn_alloc_args_overflow (tree, tree, tree[2], int[2]);
 extern tree get_attr_nonstring_decl (tree, tree * = NULL);
 extern void maybe_warn_nonstring_arg (tree, tree);
-extern bool get_size_range (tree, tree[2], bool = false);
+extern bool get_size_range (tree, wide_int[2], bool = false)
+  ATTRIBUTE_NONNULL (2);
+extern bool get_size_range (tree, tree[2], bool = false)
+  ATTRIBUTE_NONNULL (2);
 extern rtx rtx_for_static_chain (const_tree, bool);
 extern bool cxx17_empty_base_field_p (const_tree);
 
diff --git a/gcc/testsuite/gcc.dg/Wstringop-overflow-40.c b/gcc/testsuite/gcc.dg/Wstringop-overflow-40.c
new file mode 100644
index 00000000000..b3e598ca30e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wstringop-overflow-40.c
@@ -0,0 +1,163 @@ 
+/* PR middle-end/92942 - missing -Wstringop-overflow for allocations with
+   a negative lower bound size
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+#define SIZE_MAX        __SIZE_MAX__
+#define UINT8_MAX       __UINT8_MAX__
+#define UINT16_MAX      __UINT16_MAX__
+
+typedef __SIZE_TYPE__   size_t;
+typedef __UINT8_TYPE__  uint8_t;
+typedef __UINT16_TYPE__ uint16_t;
+
+void* usr_alloc1 (size_t) __attribute__ ((alloc_size (1)));
+void* usr_alloc2 (size_t, size_t)  __attribute__ ((alloc_size (1, 2)));
+
+void* malloc (size_t);
+void* memcpy (void*, const void*, size_t);
+void* memset (void*, int, size_t);
+char* strcpy (char*, const char*);
+
+void sink (void*);
+
+void malloc_uint_range_strcpy (unsigned n)
+{
+  void *p = malloc (5 < n ? 5 : n);
+
+  strcpy (p, "01234");              // { dg-warning "\\\[-Wstringop-overflow" }
+  sink (p);
+
+  strcpy (p, "0123");
+  sink (p);
+}
+
+void malloc_uint16_anti_range_memset (uint16_t n)
+{
+  if (5 <= n && n <= 9) return;
+  void *p = malloc (n);
+
+  if (UINT16_MAX < SIZE_MAX)
+    {
+      size_t sz = (uint16_t)-1 + (size_t)1;
+      memset (p, 0, sz);            // { dg-warning "\\\[-Wstringop-overflow" }
+      sink (p);
+    }
+
+  memset (p, 0, 1);
+  sink (p);
+  memset (p, 0, 5);
+  sink (p);
+  memset (p, 0, 6);
+  sink (p);
+  memset (p, 0, UINT16_MAX - 1);
+  sink (p);
+  memset (p, 0, UINT16_MAX);
+  sink (p);
+}
+
+void malloc_int_strcpy (int n)
+{
+  void *p = malloc (7 < n ? 7 : n);
+
+  strcpy (p, "0123456");            // { dg-warning "\\\[-Wstringop-overflow" }
+  sink (p);
+
+  strcpy (p, "012345");
+  sink (p);
+}
+
+void vla_int_strcpy (int n)
+{
+  char a[9 < n ? 9 : n];
+
+  strcpy (a, "012345678");          // { dg-warning "\\\[-Wstringop-overflow" }
+  sink (a);
+
+  strcpy (a, "01234567");
+  sink (a);
+}
+
+void usr_alloc1_int_strcpy (int n)
+{
+  void *p = usr_alloc1 (7 < n ? 7 : n);
+
+  strcpy (p, "0123456");            // { dg-warning "\\\[-Wstringop-overflow" }
+  sink (p);
+
+  strcpy (p, "012345");
+  sink (p);
+}
+
+void usr_alloc2_cst_ir_strcpy (int n)
+{
+  void *p = usr_alloc2 (1, 5 < n ? 5 : n);
+
+  strcpy (p, "01234");              // { dg-warning "\\\[-Wstringop-overflow" }
+  sink (p);
+
+  strcpy (p, "0123");
+  sink (p);
+}
+
+void usr_alloc2_ir_ir_strcpy (int m, int n)
+{
+  void *p = usr_alloc2 (3 < n ? 3 : n, 5 < n ? 5 : n);
+
+  strcpy (p, "0123456789abcde");    // { dg-warning "\\\[-Wstringop-overflow" }
+  sink (p);
+
+  strcpy (p, "0123456789abcd");
+  sink (p);
+}
+
+void usr_alloc2_uint8_memset (uint8_t m, uint8_t n)
+{
+  if (3 <= m && m <= 7) return;
+  if (5 <= n && n <= 9) return;
+  void *p = usr_alloc2 (m, n);
+
+  size_t sz = UINT8_MAX * UINT8_MAX + 1;
+  memset (p, 0, sz);               // { dg-warning "\\\[-Wstringop-overflow" "" { xfail *-*-* } }
+                                   // { dg-warning "\\\[-Warray-bounds" "pr?????" { target *-*-* } .-1 }
+  sink (p);
+
+  memset (p, 0, sz - 1);
+  sink (p);
+  memset (p, 0, 64);
+  sink (p);
+  memset (p, 0, 63);
+  sink (p);
+  memset (p, 0, 16);
+  sink (p);
+  memset (p, 0, 15);
+  sink (p);
+  memset (p, 0, 14);
+  sink (p);
+  memset (p, 0, 3);
+  sink (p);
+}
+
+
+
+void malloc_int_memset (int n)
+{
+  void *p = malloc (11 < n ? 11 : n);
+
+  memset (p, 0, 12);                // { dg-warning "\\\[-Wstringop-overflow" }
+  sink (p);
+
+  memset (p, 0, 11);
+  sink (p);
+}
+
+void vla_int_memset (int n)
+{
+  char a[13 < n ? 13 : n];
+
+  memset (a, 0, 14);                // { dg-warning "\\\[-Wstringop-overflow" }
+  sink (a);
+
+  memset (a, 0, 13);
+  sink (a);
+}
diff --git a/gcc/testsuite/gcc.dg/Wstringop-overflow-41.c b/gcc/testsuite/gcc.dg/Wstringop-overflow-41.c
new file mode 100644
index 00000000000..173aa164646
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wstringop-overflow-41.c
@@ -0,0 +1,91 @@ 
+/* Verify that an anti-range ~[A, B] with small positive A and B
+   is handled correctly and doesn't trigger warnings.
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+typedef __typeof__ (sizeof 0) size_t;
+
+int f (void*, size_t);
+int g (void*);
+
+// Test case distilled from gcc/cp/semantics.c
+
+int omp_reduction_id (int i, int j, const char *mm)
+{
+  const char *p = 0;
+  const char *m = 0;
+
+  switch (i)
+    {
+    case 1:
+      p = "min";
+      break;
+    case 2:
+      p = "max";
+      break;
+    default:
+      break;
+    }
+
+  if (j)
+    m = mm;
+
+  const char prefix[] = "omp declare reduction ";
+  size_t lenp = sizeof (prefix);
+
+  if (__builtin_strncmp (p, prefix, lenp - 1) == 0)
+    lenp = 1;
+
+  size_t len = __builtin_strlen (p);
+  size_t lenm = m ? __builtin_strlen (m) + 1 : 0;
+  char *name = ((char *) __builtin_alloca(lenp + len + lenm));
+
+  if (lenp > 1)
+    __builtin_memcpy (name, prefix, lenp - 1);
+
+  __builtin_memcpy (name + lenp - 1, p, len + 1);
+  if (m)
+    {
+      name[lenp + len - 1] = '~';
+      __builtin_memcpy (name + lenp + len, m, lenm);
+    }
+  return (__builtin_constant_p (name)
+	  ? f (name, __builtin_strlen (name)) : g (name));
+}
+
+// Test case derived from gcc/d/dmd/root/filename.c.
+
+const char *ext (const char *str)
+{
+  size_t len = __builtin_strlen(str);
+
+  const char *e = str + len;
+  for (;;)
+    {
+      switch (*e)
+        {
+	case '.': return e + 1;
+	case '/': break;
+	default:
+	  if (e == str)
+	    break;
+	  e--;
+	  continue;
+        }
+      return 0;
+    }
+}
+
+const char *removeExt (const char *str)
+{
+  const char *e = ext (str);
+  if (e)
+    {
+      size_t len = (e - str) - 1;
+      char *n = (char *)__builtin_malloc (len + 1);
+      __builtin_memcpy(n, str, len);
+      n[len] = 0;
+      return n;
+    }
+  return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/attr-alloc_size-10.c b/gcc/testsuite/gcc.dg/attr-alloc_size-10.c
index 071c6aa1e3b..dac6ca1e147 100644
--- a/gcc/testsuite/gcc.dg/attr-alloc_size-10.c
+++ b/gcc/testsuite/gcc.dg/attr-alloc_size-10.c
@@ -4,7 +4,7 @@ 
    range.
 
    { dg-do compile }
-   { dg-options "-O2 -Walloc-size-larger-than=12" } 
+   { dg-options "-O2 -Walloc-size-larger-than=12 -ftrack-macro-expansion=0" }
    { dg-options "-Wno-overflow" { target { ! int32plus } } } */
 
 #define SCHAR_MAX __SCHAR_MAX__