diff mbox series

avoid folding of invalid indices to compound literals (PR 92341)

Message ID ee065823-9bb6-f9af-94d7-db2b8dbc827d@gmail.com
State New
Headers show
Series avoid folding of invalid indices to compound literals (PR 92341) | expand

Commit Message

Martin Sebor Nov. 4, 2019, 10:05 p.m. UTC 
While testing some other changes I noticed that -Warray-bounds
fails to detect out-of-bounds indices to compound literals such
as in:

   int *p = (int[]){ 1, 2, 3 };
   // ...
   p[3] = 7;

This is because SRA transforms such references into accesses to
uninitialized scalar variables and also sets the TREE_NO_WARNING
bit for the replacement variables.  This prevents -Wuninitialized
from detecting such bugs, although that wouldn't be the right
warning to issue in these cases).

The attached patch tweaks SRA to avoid this transformation when
the access is out of the bounds of the referenced variable.  That
in turn lets -Warray-bounds diagnose these invalid accesses.

The patch also adjusts -Warray-bounds to reference to correct
index and message and issue the warning even for zero-length
compound literal arrays.  This was exposed and the fix is relied
on by the test I wrote for the compound literals.

Finally, the change also corrects an oversight of mine from some
time ago in failing to handle out-of-bounds indices relative to
addresses of function parameters.  This is a trivial one-line
tweak that could be submitted separately but it doesn't seem
worth the overhead.

Tested on x86_64-linux.

Martin

Comments

Jeff Law Nov. 4, 2019, 10:27 p.m. UTC | #1 
On 11/4/19 3:05 PM, Martin Sebor wrote:
> While testing some other changes I noticed that -Warray-bounds
> fails to detect out-of-bounds indices to compound literals such
> as in:
> 
>   int *p = (int[]){ 1, 2, 3 };
>   // ...
>   p[3] = 7;
> 
> This is because SRA transforms such references into accesses to
> uninitialized scalar variables and also sets the TREE_NO_WARNING
> bit for the replacement variables.  This prevents -Wuninitialized
> from detecting such bugs, although that wouldn't be the right
> warning to issue in these cases).
> 
> The attached patch tweaks SRA to avoid this transformation when
> the access is out of the bounds of the referenced variable.  That
> in turn lets -Warray-bounds diagnose these invalid accesses.
> 
> The patch also adjusts -Warray-bounds to reference to correct
> index and message and issue the warning even for zero-length
> compound literal arrays.  This was exposed and the fix is relied
> on by the test I wrote for the compound literals.
> 
> Finally, the change also corrects an oversight of mine from some
> time ago in failing to handle out-of-bounds indices relative to
> addresses of function parameters.  This is a trivial one-line
> tweak that could be submitted separately but it doesn't seem
> worth the overhead.
> 
> Tested on x86_64-linux.
> 
> Martin
> 
> gcc-92341.diff
> 
> PR middle-end/92341 - missing -Warray-bounds indexing past the end of a compound literal
> PR middle-end/82612 - missing -Warray-bounds on a non-zero offset from the address of a non-array object
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR middle-end/92341
> 	PR middle-end/82612
> 	* gcc.dg/Warray-bounds-50.c: New test.
> 	* gcc.dg/Warray-bounds-51.c: New test.
> 
> gcc/ChangeLog:
> 
> 	PR middle-end/92341
> 	PR middle-end/82612
> 	* tree-sra.c (get_access_for_expr): Fail for out-of-bounds offsets.
> 	* tree-vrp.c (vrp_prop::check_array_ref): Correct index and text
> 	of message printed in a warning for empty arrays.
> 	(vrp_prop::check_mem_ref): Also handle function parameters and
> 	empty arrays.
OK
jeff
diff mbox series

Patch

PR middle-end/92341 - missing -Warray-bounds indexing past the end of a compound literal
PR middle-end/82612 - missing -Warray-bounds on a non-zero offset from the address of a non-array object

gcc/testsuite/ChangeLog:

	PR middle-end/92341
	PR middle-end/82612
	* gcc.dg/Warray-bounds-50.c: New test.
	* gcc.dg/Warray-bounds-51.c: New test.

gcc/ChangeLog:

	PR middle-end/92341
	PR middle-end/82612
	* tree-sra.c (get_access_for_expr): Fail for out-of-bounds offsets.
	* tree-vrp.c (vrp_prop::check_array_ref): Correct index and text
	of message printed in a warning for empty arrays.
	(vrp_prop::check_mem_ref): Also handle function parameters and
	empty arrays.

diff --git a/gcc/testsuite/gcc.dg/Warray-bounds-50.c b/gcc/testsuite/gcc.dg/Warray-bounds-50.c
new file mode 100644
index 00000000000..c27e6a494f3
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Warray-bounds-50.c
@@ -0,0 +1,96 @@ 
+/* PR middle-end/92341 - missing -Warray-bounds indexing past the end
+   of a compound literal
+   { dg-do compile }
+   { dg-options "-O2 -Wall -ftrack-macro-expansion=0" } */
+
+#include "range.h"
+
+#define INT_MAX    __INT_MAX__
+#define INT_MIN    (-__INT_MAX__ - 1)
+
+void sink (int, ...);
+
+
+#define T(...) sink (__LINE__, (__VA_ARGS__))
+
+
+void direct_idx_cst (void)
+{
+  T ((int[]){ }[-1]);           // { dg-warning "array subscript -1 is outside array bounds of 'int\\\[0]'" }
+  T ((int[]){ }[0]);            // { dg-warning "array subscript 0 is outside array bounds of 'int\\\[0]'" }
+  T ((int[]){ }[1]);            // { dg-warning "array subscript 1 is outside array bounds of 'int\\\[0]'" }
+
+  T ((int[]){ 1 }[-1]);         // { dg-warning "array subscript -1 is below array bounds of 'int\\\[1]'" }
+  T ((int[]){ 1 }[0]);
+  T ((int[]){ 1 }[1]);          // { dg-warning "array subscript 1 is above array bounds of 'int\\\[1]'" }
+  T ((int[]){ 1 }[INT_MIN]);    // { dg-warning "array subscript -\[0-9\]+ is below array bounds of 'int\\\[1]'" }
+  T ((int[]){ 1 }[INT_MAX]);    // { dg-warning "array subscript \[0-9\]+ is above array bounds of 'int\\\[1]'" }
+  T ((int[]){ 1 }[SIZE_MAX]);   // { dg-warning "array subscript \[0-9\]+ is above array bounds of 'int\\\[1]'" }
+}
+
+
+void direct_idx_var (int i)
+{
+  T ((char[]){ }[i]);           // { dg-warning "array subscript i is outside array bounds of 'char\\\[0]'" }
+  T ((int[]){ }[i]);            // { dg-warning "array subscript i is outside array bounds of 'int\\\[0]'" }
+}
+
+
+void direct_idx_range (void)
+{
+  ptrdiff_t i = SR (-2, -1);
+
+  T ((int[]){ 1 }[i]);          // { dg-warning "array subscript \[ \n\r]+ is outside array bounds of 'int\\\[0]'" "pr?????" { xfail *-*-* } }
+}
+
+
+#undef T
+#define T(idx, ...) do {			\
+    int *p = (__VA_ARGS__);			\
+    sink (p[idx]);				\
+  } while (0)
+
+void ptr_idx_cst (void)
+{
+  T (-1, (int[]){ });           // { dg-warning "array subscript -1 is outside array bounds of 'int\\\[0]'" }
+  T ( 0, (int[]){ });           // { dg-warning "array subscript 0 is outside array bounds of 'int\\\[0]'" }
+  T (+1, (int[]){ });           // { dg-warning "array subscript 1 is outside array bounds of 'int\\\[0]'" }
+
+  T (-1, (int[]){ 1 });         // { dg-warning "array subscript -1 is outside array bounds of 'int\\\[1]'" }
+  T ( 0, (int[]){ 1 });
+  T (+1, (int[]){ 1 });         // { dg-warning "array subscript 1 is outside array bounds of 'int\\\[1]'" }
+  T (INT_MIN, (int[]){ 1 });    // { dg-warning "array subscript -\[0-9\]+ is outside array bounds of 'int\\\[1]'" }
+  T (INT_MAX, (int[]){ 1 });    // { dg-warning "array subscript \[0-9\]+ is outside array bounds of 'int\\\[1]'" }
+  T (SIZE_MAX, (int[]){ 1 });   // { dg-warning "array subscript -?\[0-9\]+ is outside array bounds of 'int\\\[1]'" }
+}
+
+
+void ptr_idx_var (int i)
+{
+  T (i, (int[]){ });            // { dg-warning "array subscript \[^\n\r\]+ is outside array bounds of 'int\\\[0]'" }
+  T (i, (int[]){ 1 });
+  T (i, (int[]){ i, 1 });
+}
+
+void ptr_idx_range (void)
+{
+  ptrdiff_t i = SR (-2, -1);
+
+  T (i, (int[]){ });            // { dg-warning "array subscript \\\[-2, -1] is outside array bounds of 'int\\\[0]'" }
+  T (i, (int[]){ 1 });          // { dg-warning "array subscript \\\[-2, -1] is outside array bounds of 'int\\\[1]'" }
+  T (i, (int[]){ i });          // { dg-warning "array subscript \\\[-2, -1] is outside array bounds of 'int\\\[1]'" }
+
+  i = SR (0, 1);
+
+  T (i, (int[]){ });            // { dg-warning "array subscript \\\[0, 1] is outside array bounds of 'int\\\[0]'" }
+  T (i, (int[]){ 1 });
+
+  i = SR (1, 2);
+  T (i, (int[]){ 1 });          // { dg-warning "array subscript \\\[1, 2] is outside array bounds of 'int\\\[1]'" }
+
+  i = SR (2, 3);
+  T (i, (int[]){ 1, 2, 3 });
+
+  i = SR (3, 4);
+  T (i, (int[]){ 2, 3, 4 });          // { dg-warning "array subscript \\\[3, 4] is outside array bounds of 'int\\\[3]'" }
+}
diff --git a/gcc/testsuite/gcc.dg/Warray-bounds-51.c b/gcc/testsuite/gcc.dg/Warray-bounds-51.c
new file mode 100644
index 00000000000..644fcd02cb6
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Warray-bounds-51.c
@@ -0,0 +1,24 @@ 
+/* PR middle-end/82612 - missing -Warray-bounds on a non-zero offset
+   from the address of a non-array object
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+int i;
+int f0 (void)
+{
+  int *p = &i;
+  return p[2];      // { dg-warning "-Warray-bounds" }
+}
+
+int f1 (void)
+{
+  int i;
+  int *p = &i;
+  return p[2];      // { dg-warning "-Warray-bounds" }
+}
+
+int f2 (int i)
+{
+  int *p = &i;
+  return p[2];      // { dg-warning "-Warray-bounds" }
+}
diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
index 3f104238d93..44862690559 100644
--- a/gcc/tree-sra.c
+++ b/gcc/tree-sra.c
@@ -3068,6 +3068,13 @@  get_access_for_expr (tree expr)
       || !DECL_P (base))
     return NULL;
 
+  if (tree basesize = DECL_SIZE (base))
+    {
+      poly_int64 sz = tree_to_poly_int64 (basesize);
+      if (offset < 0 || known_le (sz, offset))
+	return NULL;
+    }
+
   if (!bitmap_bit_p (candidate_bitmap, DECL_UID (base)))
     return NULL;
 
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index 31258615247..536fb96bf80 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -4122,18 +4122,18 @@  bool
 vrp_prop::check_array_ref (location_t location, tree ref,
 			   bool ignore_off_by_one)
 {
-  tree low_sub, up_sub;
-  tree low_bound, up_bound, up_bound_p1;
-
   if (TREE_NO_WARNING (ref))
     return false;
 
-  low_sub = up_sub = TREE_OPERAND (ref, 1);
-  up_bound = array_ref_up_bound (ref);
+  tree low_sub = TREE_OPERAND (ref, 1);
+  tree up_sub = low_sub;
+  tree up_bound = array_ref_up_bound (ref);
 
   /* Set for accesses to interior zero-length arrays.  */
   bool interior_zero_len = false;
 
+  tree up_bound_p1;
+
   if (!up_bound
       || TREE_CODE (up_bound) != INTEGER_CST
       || (warn_array_bounds < 2
@@ -4186,7 +4186,7 @@  vrp_prop::check_array_ref (location_t location, tree ref,
     up_bound_p1 = int_const_binop (PLUS_EXPR, up_bound,
 				   build_int_cst (TREE_TYPE (up_bound), 1));
 
-  low_bound = array_ref_low_bound (ref);
+  tree low_bound = array_ref_low_bound (ref);
 
   tree artype = TREE_TYPE (TREE_OPERAND (ref, 0));
 
@@ -4195,8 +4195,8 @@  vrp_prop::check_array_ref (location_t location, tree ref,
   /* Empty array.  */
   if (up_bound && tree_int_cst_equal (low_bound, up_bound_p1))
     warned = warning_at (location, OPT_Warray_bounds,
-			 "array subscript %E is above array bounds of %qT",
-			 low_bound, artype);
+			 "array subscript %E is outside array bounds of %qT",
+			 low_sub, artype);
 
   const value_range *vr = NULL;
   if (TREE_CODE (low_sub) == SSA_NAME)
@@ -4410,6 +4410,7 @@  vrp_prop::check_mem_ref (location_t location, tree ref,
     {
       arg = TREE_OPERAND (arg, 0);
       if (TREE_CODE (arg) != STRING_CST
+	  && TREE_CODE (arg) != PARM_DECL
 	  && TREE_CODE (arg) != VAR_DECL)
 	return false;
     }
@@ -4493,7 +4494,9 @@  vrp_prop::check_mem_ref (location_t location, tree ref,
   if (ignore_off_by_one)
     ubound += 1;
 
-  if (offrange[0] >= ubound || offrange[1] < arrbounds[0])
+  if (arrbounds[0] == arrbounds[1]
+      || offrange[0] >= ubound
+      || offrange[1] < arrbounds[0])
     {
       /* Treat a reference to a non-array object as one to an array
 	 of a single element.  */