analyzer: Standalone OOB-warning [PR109437, PR109439]

  From: Benjamin Priour <vultkayn@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.
---
 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
  

On Tue, 2023-06-06 at 13:48 +0200, priour.be@gmail.com wrote:
> From: Benjamin Priour <vultkayn@gcc.gnu.org>

Hi Benjamin, thanks for this patch.

Overall it look great.

You didn't say what testing you did on the patch.  Typically when
preparing a patch for the mailing list we do a bootstrap build of gcc
with the patch and run the regression test suite, and we state that we
did this, and which configuration we did it on.  Do you have access to
a machine where this process is fast enough to not be painful? (for
reference, it takes me 2 hours for one of my patches).  If not, we can
get you an account on the GCC Compile Farm.  For patches that just
touch the analyzer subdirectory we can probably get away with just
testing the C, C++ and Fortran frontends and testcases.

I have some minor formatting nitpicks:

[...snip...]
>  
> -/* 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.
> +*/

Normally we try to have the terminating "*/" on the final line of the
text of the comment, rather than starting a new line.  I sometimes
break this rule for ASCII art in comments, but that isn't the case
here.

[...snip...]

> @@ -800,6 +802,7 @@ region_model::check_symbolic_bounds (const region *base_reg,
>                                                               offset_tree,
>                                                               num_bytes_tree,
>                                                               capacity_tree));
> +    return false;

It's an unfortunate side-effect of our mixed spaces-and-tabs
indentation style that when a patch prefixes a line with "+" it's hard
for me to tell if the indentation is correct when reviewing a patch.

I'm going to assume that this is indented correctly, and it just looks
strange at review-time due to that.

Do you have "visual whitespace" turned on in your editor?

[...snip...]

>  
> -/* 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.
> +*/

Same nitpick as above about the "*/" comment terminator.

>  
> -void
> +bool
>  region_model::check_region_bounds (const region *reg,
>                                    enum access_direction dir,
>                                    region_model_context *ctxt) const

[...snip...]

> @@ -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;
>         }

As above; please doublecheck the indentation on the added lines on the
above.

[...snip...]

> 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 } */
>  }
> 

It could be argued for the above two cases that we should emit *both*
the uninit and the oob warnings, given that the part of the region
being read that's within bounds is uninitialized - but I can't see a
way to implement that without massively overcomplicating things, so
let's go with the approach in your patch.

Thanks again for the patch; sorry if this comes across as overly
nitpicky.

Dave
  

On Tue, Jun 6, 2023 at 8:37 PM David Malcolm <dmalcolm@redhat.com> wrote:
>
> On Tue, 2023-06-06 at 18:05 +0200, Benjamin Priour wrote:

[...]

> [Looks like you droppped the mailing list from the recipients; was that
> intentional?]
>

Not at all, just me missing the reply all button.

> >
> > I indeed bootstrapped and regtested on linux-x86_64, but it was last
> > week, since I'm still using my laptop, which is painfully slow  (1
> > night per step), my tests are always a few days old.
>
> Thanks.  The patch is OK for trunk once the minor formatting nits are
> fixed (you don't have to bother with a full test run for that).  We
> might want to backport it to gcc 13 as well, but let's let it "soak" in
> trunk for some time first.
>
> > We discussed it already but yes, in the end I believe an account on
> > the compile farm will be necessary for me.
>
> Let me know if you need any help with that.

I'm not certain about what to put under "Contributions" in the account
creation form.
I'm still green behind the ears, and wouldn't count my current count
of 2 patches
*not yet pushed to trunk* as anything remarkable.

> > I'll correct the formatting of the comments and resend it, and double
> > check the indentation.
>
> Thanks.

I said that but actually I am unsure about the indentation format.
Is it spaces up to 6 characters them morph them into tabs ?
It was looking like that in the code, although some portion were
breaking this rule.
I went with the same indentation rules as already shown within each function.

>
> >  I'm still writing custom formatting rules for
> > my gcc subfolders,
> > but the formatter is sometimes switching back to my default rules
> > instead of the workspace's.
>
> Which formatter are these rules for, BTW?
>

I'm using vscode default C/Cpp extension's formatter.

[...]

Thanks,
Benjamin
  

On Wed, 2023-06-07 at 13:38 +0200, Benjamin Priour wrote:
> On Tue, Jun 6, 2023 at 8:37 PM David Malcolm <dmalcolm@redhat.com>
> wrote:
> > 
> > On Tue, 2023-06-06 at 18:05 +0200, Benjamin Priour wrote:
> 
> [...]
> 
> > [Looks like you droppped the mailing list from the recipients; was
> > that
> > intentional?]
> > 
> 
> Not at all, just me missing the reply all button.
> 
> > > 
> > > I indeed bootstrapped and regtested on linux-x86_64, but it was
> > > last
> > > week, since I'm still using my laptop, which is painfully slow 
> > > (1
> > > night per step), my tests are always a few days old.
> > 
> > Thanks.  The patch is OK for trunk once the minor formatting nits
> > are
> > fixed (you don't have to bother with a full test run for that).  We
> > might want to backport it to gcc 13 as well, but let's let it
> > "soak" in
> > trunk for some time first.
> > 
> > > We discussed it already but yes, in the end I believe an account
> > > on
> > > the compile farm will be necessary for me.
> > 
> > Let me know if you need any help with that.
> 
> I'm not certain about what to put under "Contributions" in the
> account
> creation form.
> I'm still green behind the ears, and wouldn't count my current count
> of 2 patches
> *not yet pushed to trunk* as anything remarkable.

That's OK.  Let them know that you are a GSoC student working on GCC
this summer, and that I am sponsoring your request for usage of these
machines, as your GSoC mentor (you can link to this email in the
mailing list archives if need be).

> 
> > > I'll correct the formatting of the comments and resend it, and
> > > double
> > > check the indentation.
> > 
> > Thanks.
> 
> I said that but actually I am unsure about the indentation format.
> Is it spaces up to 6 characters them morph them into tabs ?
> It was looking like that in the code, although some portion were
> breaking this rule.
> I went with the same indentation rules as already shown within each
> function.

I believe it's 2 spaces for each indentation level, but every 8 spaces
becomes a tab, though from looking through
https://www.gnu.org/prep/standards/standards.html I couldn't find where
it specifies that.

> 
> > 
> > >  I'm still writing custom formatting rules for
> > > my gcc subfolders,
> > > but the formatter is sometimes switching back to my default rules
> > > instead of the workspace's.
> > 
> > Which formatter are these rules for, BTW?
> > 
> 
> I'm using vscode default C/Cpp extension's formatter.

(nods)

Thanks
Dave
  

> On Jun 6, 2023, at 15:48, Benjamin Priour via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> 
> From: Benjamin Priour <vultkayn@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
>
  

Hi,

Yes of course, I tested many days ago since regtesting takes several days
on my box, I should have retested !
But I got an account for the compile farm today, so I'm on it immediately,
I also see a divergence in the warnings on my box.

Thanks for the report !
Sincerely sorry,
Benjamin.

On Thu, Jun 8, 2023 at 7:53 PM Maxim Kuvyrkov <maxim.kuvyrkov@linaro.org>
wrote:

> > On Jun 6, 2023, at 15:48, Benjamin Priour via Gcc-patches <
> gcc-patches@gcc.gnu.org> wrote:
> >
> > From: Benjamin Priour <vultkayn@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
> >
>
>
  

Hi David,

So first real committed patch actually was a misstep. So I'm currently
fixing that.
The issue is that the original idea, to return a boolean and create a
unknown_svalue on OOB access to prevent further "use-of-uninitialized-value"
caused a loss of information on the location of the buffer. So now, when a
buffer is on the stack, we lose that information by returning an
unknown_svalue
from get_store_value. Therefore further checks from 'sm_malloc' won't be
able to detect erroneous operations expecting heap-allocated buffer, e.g. a
delete.
It does not trouble successive out_of_bounds checks, since the checks are
done on the boundaries of the initial buffer, that
The issue is from sm_state_map::get_state, since an unknown_svalue cannot
hold any state, then in the checker is misled.
I thought to artificially add a state, but since the unknown_svalue are
singleton per type, it is not right.
Therefore I'm considering something, to make it so
can_have_initial_svalue_p holds true for OOB heap access as it is for the
stack, instead of creating an unknown_svalue.
I'll do **PROPER** testing tomorrow, now that I have the compile farm, to
check if doing so won't introduce any further issue.
This way we would keep all the relevant information about the region,
without making it poisoned, and OOB checks are done with the initial byte
size of the buffer,
so this should not be disturbed.

I briefly tried the above as a proof of concept. Doing so fixed PR100244.C
mentioned by Maxim, while still passing my new test cases for PR109439.
I will regtest this configuration tomorrow morning on the farm, I am
getting sleepy, except if you can already see problems this would cause.

Sorry again I have somewhat managed to fail my first commit, and pushed it.

Thanks,
Benjamin.

---------- Forwarded message ---------
From: Benjamin Priour <priour.be@gmail.com>
Date: Thu, Jun 8, 2023 at 8:18 PM
Subject: Re: [PATCH] analyzer: Standalone OOB-warning [PR109437, PR109439]
To: Maxim Kuvyrkov <maxim.kuvyrkov@linaro.org>
Cc: <gcc-patches@gcc.gnu.org>, Benjamin Priour <vultkayn@gcc.gnu.org>, <
dmalcolm@redhat.com>


Hi,

Yes of course, I tested many days ago since regtesting takes several days
on my box, I should have retested !
But I got an account for the compile farm today, so I'm on it immediately,
I also see a divergence in the warnings on my box.

Thanks for the report !
Sincerely sorry,
Benjamin.

On Thu, Jun 8, 2023 at 7:53 PM Maxim Kuvyrkov <maxim.kuvyrkov@linaro.org>
wrote:

> > On Jun 6, 2023, at 15:48, Benjamin Priour via Gcc-patches <
> gcc-patches@gcc.gnu.org> wrote:
> >
> > From: Benjamin Priour <vultkayn@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
> >
>
>
  

Hi Maxim,

I managed to nail the bug on the failing test pr100244.C, as I did too
observe a divergence after my patch.
For pr101962.c, it was simply a dg-note I forgot to remove, that made it
fail, since the related warning is no longer relevant. The behavior
otherwise is as expected before and after the patch.
It is now fixed.

Thanks again a lot for you attention,
Benjamin.

On Fri, Jun 9, 2023 at 2:19 AM Benjamin Priour <priour.be@gmail.com> wrote:

> Hi David,
>
> So first real committed patch actually was a misstep. So I'm currently
> fixing that.
> The issue is that the original idea, to return a boolean and create a
> unknown_svalue on OOB access to prevent further "use-of-uninitialized-value"
> caused a loss of information on the location of the buffer. So now, when a
> buffer is on the stack, we lose that information by returning an
> unknown_svalue
> from get_store_value. Therefore further checks from 'sm_malloc' won't be
> able to detect erroneous operations expecting heap-allocated buffer, e.g. a
> delete.
> It does not trouble successive out_of_bounds checks, since the checks are
> done on the boundaries of the initial buffer, that
> The issue is from sm_state_map::get_state, since an unknown_svalue cannot
> hold any state, then in the checker is misled.
> I thought to artificially add a state, but since the unknown_svalue are
> singleton per type, it is not right.
> Therefore I'm considering something, to make it so
> can_have_initial_svalue_p holds true for OOB heap access as it is for the
> stack, instead of creating an unknown_svalue.
> I'll do **PROPER** testing tomorrow, now that I have the compile farm, to
> check if doing so won't introduce any further issue.
> This way we would keep all the relevant information about the region,
> without making it poisoned, and OOB checks are done with the initial byte
> size of the buffer,
> so this should not be disturbed.
>
> I briefly tried the above as a proof of concept. Doing so fixed PR100244.C
> mentioned by Maxim, while still passing my new test cases for PR109439.
> I will regtest this configuration tomorrow morning on the farm, I am
> getting sleepy, except if you can already see problems this would cause.
>
> Sorry again I have somewhat managed to fail my first commit, and pushed it.
>
> Thanks,
> Benjamin.
>
> ---------- Forwarded message ---------
> From: Benjamin Priour <priour.be@gmail.com>
> Date: Thu, Jun 8, 2023 at 8:18 PM
> Subject: Re: [PATCH] analyzer: Standalone OOB-warning [PR109437, PR109439]
> To: Maxim Kuvyrkov <maxim.kuvyrkov@linaro.org>
> Cc: <gcc-patches@gcc.gnu.org>, Benjamin Priour <vultkayn@gcc.gnu.org>, <
> dmalcolm@redhat.com>
>
>
> Hi,
>
> Yes of course, I tested many days ago since regtesting takes several days
> on my box, I should have retested !
> But I got an account for the compile farm today, so I'm on it immediately,
> I also see a divergence in the warnings on my box.
>
> Thanks for the report !
> Sincerely sorry,
> Benjamin.
>
> On Thu, Jun 8, 2023 at 7:53 PM Maxim Kuvyrkov <maxim.kuvyrkov@linaro.org>
> wrote:
>
>> > On Jun 6, 2023, at 15:48, Benjamin Priour via Gcc-patches <
>> gcc-patches@gcc.gnu.org> wrote:
>> >
>> > From: Benjamin Priour <vultkayn@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
>> >
>>
>>