[COMMITTED] analyzer: Standalone OOB-warning, formatting fixed [PR109437, PR109439]

Message ID 20230608093048.1677718-1-vultkayn@gcc.gnu.org
State Accepted
Headers
Series [COMMITTED] analyzer: Standalone OOB-warning, formatting fixed [PR109437, PR109439] |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Benjamin Priour June 8, 2023, 9:30 a.m. UTC
  From: Benjamin Priour <vultkayn@gcc.gnu.org>

For the record, below is the previous patch I submitted, with the
little formatting issues fixed - multiline docstring no ends on a newline.
It was otherwise validated by David Malcolm, so I already committed it.

This patch enhances -Wanalyzer-out-of-bounds that is no longer paired
with a -Wanalyzer-use-of-uninitialized-value on out-of-bounds-read.

This also fixes PR analyzer/109437.
Before there could always be at most one OOB-read warning per frame because
-Wanalyzer-use-of-uninitialized-value always terminates the analysis
path.

	PR analyzer/109439

gcc/analyzer/ChangeLog:

	* bounds-checking.cc (region_model::check_symbolic_bounds):
          Returns whether the BASE_REG region access was OOB.
	(region_model::check_region_bounds): Likewise.
	* region-model.cc (region_model::get_store_value): Creates an
          unknown svalue on OOB-read access to REG.
	(region_model::check_region_access): Returns whether an
unknown svalue needs be created.
	(region_model::check_region_for_read): Passes
check_region_access return value.
	* region-model.h: Update prior function definitions.

gcc/testsuite/ChangeLog:

	* gcc.dg/analyzer/out-of-bounds-2.c: Cleaned test for
          uninitialized-value warning.
	* gcc.dg/analyzer/out-of-bounds-5.c: Likewise.
	* gcc.dg/analyzer/pr101962.c: Likewise.
	* gcc.dg/analyzer/realloc-5.c: Likewise.
	* gcc.dg/analyzer/pr109439.c: New test.
---
 gcc/analyzer/bounds-checking.cc               | 28 +++++++++++++------
 gcc/analyzer/region-model.cc                  | 22 +++++++++------
 gcc/analyzer/region-model.h                   |  8 +++---
 .../gcc.dg/analyzer/out-of-bounds-2.c         |  1 -
 .../gcc.dg/analyzer/out-of-bounds-5.c         |  2 --
 gcc/testsuite/gcc.dg/analyzer/pr101962.c      |  1 -
 gcc/testsuite/gcc.dg/analyzer/pr109439.c      | 12 ++++++++
 gcc/testsuite/gcc.dg/analyzer/realloc-5.c     |  1 -
 8 files changed, 49 insertions(+), 26 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr109439.c
  

Patch

diff --git a/gcc/analyzer/bounds-checking.cc b/gcc/analyzer/bounds-checking.cc
index 3bf542a8eba..a5692cf9319 100644
--- a/gcc/analyzer/bounds-checking.cc
+++ b/gcc/analyzer/bounds-checking.cc
@@ -767,9 +767,10 @@  public:
   }
 };
 
-/* Check whether an access is past the end of the BASE_REG.  */
+/* Check whether an access is past the end of the BASE_REG.
+  Return TRUE if the access was valid, FALSE otherwise.  */
 
-void
+bool
 region_model::check_symbolic_bounds (const region *base_reg,
 				     const svalue *sym_byte_offset,
 				     const svalue *num_bytes_sval,
@@ -800,6 +801,7 @@  region_model::check_symbolic_bounds (const region *base_reg,
 							      offset_tree,
 							      num_bytes_tree,
 							      capacity_tree));
+	  return false;
 	  break;
 	case DIR_WRITE:
 	  ctxt->warn (make_unique<symbolic_buffer_overflow> (base_reg,
@@ -807,9 +809,11 @@  region_model::check_symbolic_bounds (const region *base_reg,
 							     offset_tree,
 							     num_bytes_tree,
 							     capacity_tree));
+	  return false;
 	  break;
 	}
     }
+  return true;
 }
 
 static tree
@@ -822,9 +826,10 @@  maybe_get_integer_cst_tree (const svalue *sval)
   return NULL_TREE;
 }
 
-/* May complain when the access on REG is out-of-bounds.  */
+/* May complain when the access on REG is out-of-bounds.
+   Return TRUE if the access was valid, FALSE otherwise.  */
 
-void
+bool
 region_model::check_region_bounds (const region *reg,
 				   enum access_direction dir,
 				   region_model_context *ctxt) const
@@ -839,14 +844,14 @@  region_model::check_region_bounds (const region *reg,
      (e.g. because the analyzer did not see previous offsets on the latter,
      it might think that a negative access is before the buffer).  */
   if (base_reg->symbolic_p ())
-    return;
+	  return true;
 
   /* Find out how many bytes were accessed.  */
   const svalue *num_bytes_sval = reg->get_byte_size_sval (m_mgr);
   tree num_bytes_tree = maybe_get_integer_cst_tree (num_bytes_sval);
   /* Bail out if 0 bytes are accessed.  */
   if (num_bytes_tree && zerop (num_bytes_tree))
-    return;
+	  return true;
 
   /* Get the capacity of the buffer.  */
   const svalue *capacity = get_capacity (base_reg);
@@ -877,13 +882,13 @@  region_model::check_region_bounds (const region *reg,
 	}
       else
 	byte_offset_sval = reg_offset.get_symbolic_byte_offset ();
-      check_symbolic_bounds (base_reg, byte_offset_sval, num_bytes_sval,
+		  return check_symbolic_bounds (base_reg, byte_offset_sval, num_bytes_sval,
 			     capacity, dir, ctxt);
-      return;
     }
 
   /* Otherwise continue to check with concrete values.  */
   byte_range out (0, 0);
+  bool oob_safe = true;
   /* NUM_BYTES_TREE should always be interpreted as unsigned.  */
   byte_offset_t num_bytes_unsigned = wi::to_offset (num_bytes_tree);
   byte_range read_bytes (offset, num_bytes_unsigned);
@@ -899,10 +904,12 @@  region_model::check_region_bounds (const region *reg,
 	case DIR_READ:
 	  ctxt->warn (make_unique<concrete_buffer_under_read> (reg, diag_arg,
 							       out));
+	  oob_safe = false;
 	  break;
 	case DIR_WRITE:
 	  ctxt->warn (make_unique<concrete_buffer_underwrite> (reg, diag_arg,
 							       out));
+	  oob_safe = false;
 	  break;
 	}
     }
@@ -911,7 +918,7 @@  region_model::check_region_bounds (const region *reg,
      do a symbolic check here because the inequality check does not reason
      whether constants are greater than symbolic values.  */
   if (!cst_capacity_tree)
-    return;
+	  return oob_safe;
 
   byte_range buffer (0, wi::to_offset (cst_capacity_tree));
   /* If READ_BYTES exceeds BUFFER, we do have an overflow.  */
@@ -929,13 +936,16 @@  region_model::check_region_bounds (const region *reg,
 	case DIR_READ:
 	  ctxt->warn (make_unique<concrete_buffer_over_read> (reg, diag_arg,
 							      out, byte_bound));
+	  oob_safe = false;
 	  break;
 	case DIR_WRITE:
 	  ctxt->warn (make_unique<concrete_buffer_overflow> (reg, diag_arg,
 							     out, byte_bound));
+	  oob_safe = false;
 	  break;
 	}
     }
+  return oob_safe;
 }
 
 } // namespace ana
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 3bb3df2f063..fb96cd54940 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -2396,7 +2396,8 @@  region_model::get_store_value (const region *reg,
   if (reg->empty_p ())
     return m_mgr->get_or_create_unknown_svalue (reg->get_type ());
 
-  check_region_for_read (reg, ctxt);
+  if (check_region_for_read (reg, ctxt))
+    return m_mgr->get_or_create_unknown_svalue(reg->get_type());
 
   /* Special-case: handle var_decls in the constant pool.  */
   if (const decl_region *decl_reg = reg->dyn_cast_decl_region ())
@@ -2802,19 +2803,22 @@  region_model::get_string_size (const region *reg) const
 }
 
 /* If CTXT is non-NULL, use it to warn about any problems accessing REG,
-   using DIR to determine if this access is a read or write.  */
+   using DIR to determine if this access is a read or write.
+   Return TRUE if an UNKNOWN_SVALUE needs be created.  */
 
-void
+bool
 region_model::check_region_access (const region *reg,
 				   enum access_direction dir,
 				   region_model_context *ctxt) const
 {
   /* Fail gracefully if CTXT is NULL.  */
   if (!ctxt)
-    return;
+    return false;
 
+  bool need_unknown_sval = false;
   check_region_for_taint (reg, dir, ctxt);
-  check_region_bounds (reg, dir, ctxt);
+  if (!check_region_bounds (reg, dir, ctxt))
+    need_unknown_sval = true;
 
   switch (dir)
     {
@@ -2827,6 +2831,7 @@  region_model::check_region_access (const region *reg,
       check_for_writable_region (reg, ctxt);
       break;
     }
+  return need_unknown_sval;
 }
 
 /* If CTXT is non-NULL, use it to warn about any problems writing to REG.  */
@@ -2838,13 +2843,14 @@  region_model::check_region_for_write (const region *dest_reg,
   check_region_access (dest_reg, DIR_WRITE, ctxt);
 }
 
-/* If CTXT is non-NULL, use it to warn about any problems reading from REG.  */
+/* If CTXT is non-NULL, use it to warn about any problems reading from REG.
+  Returns TRUE if an unknown svalue needs be created.  */
 
-void
+bool
 region_model::check_region_for_read (const region *src_reg,
 				     region_model_context *ctxt) const
 {
-  check_region_access (src_reg, DIR_READ, ctxt);
+  return check_region_access (src_reg, DIR_READ, ctxt);
 }
 
 /* Concrete subclass for casts of pointers that lead to trailing bytes.  */
diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h
index fe3db0b0c98..12f84b20463 100644
--- a/gcc/analyzer/region-model.h
+++ b/gcc/analyzer/region-model.h
@@ -553,22 +553,22 @@  private:
 
   void check_for_writable_region (const region* dest_reg,
 				  region_model_context *ctxt) const;
-  void check_region_access (const region *reg,
+  bool check_region_access (const region *reg,
 			    enum access_direction dir,
 			    region_model_context *ctxt) const;
-  void check_region_for_read (const region *src_reg,
+  bool check_region_for_read (const region *src_reg,
 			      region_model_context *ctxt) const;
   void check_region_size (const region *lhs_reg, const svalue *rhs_sval,
 			  region_model_context *ctxt) const;
 
   /* Implemented in bounds-checking.cc  */
-  void check_symbolic_bounds (const region *base_reg,
+  bool check_symbolic_bounds (const region *base_reg,
 			      const svalue *sym_byte_offset,
 			      const svalue *num_bytes_sval,
 			      const svalue *capacity,
 			      enum access_direction dir,
 			      region_model_context *ctxt) const;
-  void check_region_bounds (const region *reg, enum access_direction dir,
+  bool check_region_bounds (const region *reg, enum access_direction dir,
 			    region_model_context *ctxt) const;
 
   void check_call_args (const call_details &cd) const;
diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-2.c b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-2.c
index 1330090f348..336f624441c 100644
--- a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-2.c
+++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-2.c
@@ -82,5 +82,4 @@  void test5 (void)
   /* { dg-warning "heap-based buffer over-read" "bounds warning" { target *-*-* } test5 } */
   /* { dg-message "read of 4 bytes from after the end of the region" "num bad bytes note" { target *-*-* } test5 } */
 
-  /* { dg-warning "use of uninitialized value" "uninit warning" { target *-*-* } test5 } */
 }
diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-5.c b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-5.c
index 2a61d8ca236..568f9cad199 100644
--- a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-5.c
+++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-5.c
@@ -68,7 +68,6 @@  void test8 (size_t size, size_t offset)
   char dst[size];
   memcpy (dst, src, size + offset); /* { dg-line test8 } */
   /* { dg-warning "over-read" "warning" { target *-*-* } test8 } */
-  /* { dg-warning "use of uninitialized value" "warning" { target *-*-* } test8 } */
   /* { dg-warning "overflow" "warning" { target *-*-* } test8 } */
 }
 
@@ -78,7 +77,6 @@  void test9 (size_t size, size_t offset)
   int32_t dst[size];
   memcpy (dst, src, 4 * size + 1); /* { dg-line test9 } */
   /* { dg-warning "over-read" "warning" { target *-*-* } test9 } */
-  /* { dg-warning "use of uninitialized value" "warning" { target *-*-* } test9 } */
   /* { dg-warning "overflow" "warning" { target *-*-* } test9 } */
 }
 
diff --git a/gcc/testsuite/gcc.dg/analyzer/pr101962.c b/gcc/testsuite/gcc.dg/analyzer/pr101962.c
index 08c0aba5cbf..b878aad9cf1 100644
--- a/gcc/testsuite/gcc.dg/analyzer/pr101962.c
+++ b/gcc/testsuite/gcc.dg/analyzer/pr101962.c
@@ -24,7 +24,6 @@  test_1 (void)
   __analyzer_eval (a != NULL); /* { dg-warning "TRUE" } */
   return *a; /* { dg-line test_1 } */
 
-  /* { dg-warning "use of uninitialized value '\\*a'" "warning" { target *-*-* } test_1 } */
   /* { dg-warning "stack-based buffer over-read" "warning" { target *-*-* } test_1 } */
 }
 
diff --git a/gcc/testsuite/gcc.dg/analyzer/pr109439.c b/gcc/testsuite/gcc.dg/analyzer/pr109439.c
new file mode 100644
index 00000000000..01c87cf171c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/pr109439.c
@@ -0,0 +1,12 @@ 
+int would_like_only_oob (int i)
+{
+    int arr[] = {1,2,3,4,5,6,7};
+    arr[10] = 9; /* { dg-warning "stack-based buffer overflow" } */
+    arr[11] = 15; /* { dg-warning "stack-based buffer overflow" } */
+    int y1 = arr[9]; /* { dg-warning "stack-based buffer over-read" } */
+            /* { dg-bogus "use of uninitialized value" "" { target *-*-* } .-1 } */
+
+    arr[18] = 15; /* { dg-warning "stack-based buffer overflow" } */
+    
+    return y1;
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/realloc-5.c b/gcc/testsuite/gcc.dg/analyzer/realloc-5.c
index 137e05b87aa..f65f2c6ca25 100644
--- a/gcc/testsuite/gcc.dg/analyzer/realloc-5.c
+++ b/gcc/testsuite/gcc.dg/analyzer/realloc-5.c
@@ -40,7 +40,6 @@  void test_1 ()
     
       /* { dg-warning "UNKNOWN" "warning" { target *-*-* } eval } */
       /* { dg-warning "heap-based buffer over-read" "warning" { target *-*-* } eval } */
-      /* { dg-warning "use of uninitialized value" "warning" { target *-*-* } eval } */
     }
 
   free (q);