> On Jun 6, 2023, at 15:48, Benjamin Priour via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: > > From: Benjamin Priour <vultk...@gcc.gnu.org> > > This patch enchances -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. > ---
Hi Benjamin, This patch makes two tests fail on arm-linux-gnueabihf. Probably, they need to be updated as well. Would you please investigate? Let me know if it doesn't easily reproduce for you, and I'll help. === g++ tests === Running g++:g++.dg/analyzer/analyzer.exp ... FAIL: g++.dg/analyzer/pr100244.C -std=c++14 (test for warnings, line 17) FAIL: g++.dg/analyzer/pr100244.C -std=c++17 (test for warnings, line 17) FAIL: g++.dg/analyzer/pr100244.C -std=c++20 (test for warnings, line 17) === gcc tests === Running gcc:gcc.dg/analyzer/analyzer.exp ... FAIL: gcc.dg/analyzer/pr101962.c (test for warnings, line 19) Thanks, -- Maxim Kuvyrkov https://www.linaro.org > gcc/analyzer/bounds-checking.cc | 30 +++++++++++++------ > 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, 51 insertions(+), 26 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr109439.c > > diff --git a/gcc/analyzer/bounds-checking.cc b/gcc/analyzer/bounds-checking.cc > index 3bf542a8eba..479b8e4b88d 100644 > --- a/gcc/analyzer/bounds-checking.cc > +++ b/gcc/analyzer/bounds-checking.cc > @@ -767,9 +767,11 @@ 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 +802,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 +810,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 +827,11 @@ 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 +846,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 +884,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 +906,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 +920,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 +938,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); > -- > 2.34.1 >
