[PATCH] analyzer: Fix regression bug after r14-1632-g9589a46ddadc8b [pr110198]

[PATCH] analyzer: Fix regression bug after r14-1632-g9589a46ddadc8b [pr110198]

[priour.be]

From: benjamin priour <priour.be@gmail.com>

Resend with proper subject line ...

Hi,

Below is the fix to regression bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110198
Was bootstrapped and regtested successfully on x86_64-linux-gnu
Considering mishap from last patch, I'd would appreciate if you could
also regtest it, to be sure :)

Thanks,
Benjamin.


g++.dg/analyzer/pr100244.C was failing after a patch of PR109439.
The reason was a spurious preemptive return of get_store_value upon out-of-bounds read that
was preventing further checks. Now instead, a boolean value check_poisoned goes to false when
a OOB is detected, and is later on given to get_or_create_initial_value.

gcc/analyzer/ChangeLog:

	* region-model-manager.cc (region_model_manager::get_or_create_initial_value): Take an
		optional boolean value to bypass poisoning checks
	* region-model-manager.h: Update declaration of the above function.
	* region-model.cc (region_model::get_store_value): No longer
		returns on OOB, but rather gives a boolean to get_or_create_initial_value.
	(region_model::check_region_access): Update docstring.
	(region_model::check_region_for_write): Update docstring.

Signed-off-by: benjamin priour <priour.be@gmail.com>
---
 gcc/analyzer/region-model-manager.cc |  5 +++--
 gcc/analyzer/region-model-manager.h  |  3 ++-
 gcc/analyzer/region-model.cc         | 15 ++++++++-------
 3 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/gcc/analyzer/region-model-manager.cc b/gcc/analyzer/region-model-manager.cc
index 1453acf7bc9..4f11ef4bd29 100644
--- a/gcc/analyzer/region-model-manager.cc
+++ b/gcc/analyzer/region-model-manager.cc
@@ -293,9 +293,10 @@ region_model_manager::create_unique_svalue (tree type)
    necessary.  */
 
 const svalue *
-region_model_manager::get_or_create_initial_value (const region *reg)
+region_model_manager::get_or_create_initial_value (const region *reg,
+						   bool check_poisoned)
 {
-  if (!reg->can_have_initial_svalue_p ())
+  if (!reg->can_have_initial_svalue_p () && check_poisoned)
     return get_or_create_poisoned_svalue (POISON_KIND_UNINIT,
 					  reg->get_type ());
 
diff --git a/gcc/analyzer/region-model-manager.h b/gcc/analyzer/region-model-manager.h
index 3340c3ebd1e..ff5333bf07c 100644
--- a/gcc/analyzer/region-model-manager.h
+++ b/gcc/analyzer/region-model-manager.h
@@ -49,7 +49,8 @@ public:
 					     tree type);
   const svalue *get_or_create_poisoned_svalue (enum poison_kind kind,
 					       tree type);
-  const svalue *get_or_create_initial_value (const region *reg);
+  const svalue *get_or_create_initial_value (const region *reg,
+					     bool check_poisoned = true);
   const svalue *get_ptr_svalue (tree ptr_type, const region *pointee);
   const svalue *get_or_create_unaryop (tree type, enum tree_code op,
 				       const svalue *arg);
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 6bc60f89f3d..187013a37cc 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -2373,8 +2373,9 @@ region_model::get_store_value (const region *reg,
   if (reg->empty_p ())
     return m_mgr->get_or_create_unknown_svalue (reg->get_type ());
 
+  bool check_poisoned = true;
   if (check_region_for_read (reg, ctxt))
-    return m_mgr->get_or_create_unknown_svalue(reg->get_type());
+    check_poisoned = false;
 
   /* Special-case: handle var_decls in the constant pool.  */
   if (const decl_region *decl_reg = reg->dyn_cast_decl_region ())
@@ -2427,7 +2428,7 @@ region_model::get_store_value (const region *reg,
       == RK_GLOBALS)
     return get_initial_value_for_global (reg);
 
-  return m_mgr->get_or_create_initial_value (reg);
+  return m_mgr->get_or_create_initial_value (reg, check_poisoned);
 }
 
 /* Return false if REG does not exist, true if it may do.
@@ -2790,7 +2791,7 @@ 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.
-   Return TRUE if an UNKNOWN_SVALUE needs be created.
+   Return TRUE if an OOB access was detected.
    If SVAL_HINT is non-NULL, use it as a hint in diagnostics
    about the value that would be written to REG.  */
 
@@ -2804,10 +2805,10 @@ region_model::check_region_access (const region *reg,
   if (!ctxt)
     return false;
 
-  bool need_unknown_sval = false;
+  bool oob_access_detected = false;
   check_region_for_taint (reg, dir, ctxt);
   if (!check_region_bounds (reg, dir, sval_hint, ctxt))
-    need_unknown_sval = true;
+    oob_access_detected = true;
 
   switch (dir)
     {
@@ -2820,7 +2821,7 @@ region_model::check_region_access (const region *reg,
       check_for_writable_region (reg, ctxt);
       break;
     }
-  return need_unknown_sval;
+  return oob_access_detected;
 }
 
 /* If CTXT is non-NULL, use it to warn about any problems writing to REG.  */
@@ -2834,7 +2835,7 @@ region_model::check_region_for_write (const region *dest_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.  */
+  Returns TRUE if an OOB read was detected.  */
 
 bool
 region_model::check_region_for_read (const region *src_reg,
-- 
2.34.1

PING: Re: [PATCH] analyzer: Fix regression bug after r14-1632-g9589a46ddadc8b [pr110198]

[Benjamin Priour]

[-- Attachment #1: Type: text/plain, Size: 6508 bytes --]

Hi,
Pinging that regression fix.
Is everything OK for trunk ?

Thanks,
Benjamin

On Thu, Jun 22, 2023 at 9:57 PM <priour.be@gmail.com> wrote:

    From: benjamin priour <priour.be@gmail.com>

    Resend with proper subject line ...

    Hi,

    Below is the fix to regression bug
    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110198
    Was bootstrapped and regtested successfully on x86_64-linux-gnu
    Considering mishap from last patch, I would appreciate if you could
    also regtest it, to be sure :)

    Thanks,
    Benjamin.


    g++.dg/analyzer/pr100244.C was failing after a patch of PR109439.
    The reason was a spurious preemptive return of get_store_value upon
    out-of-bounds read that
    was preventing further checks. Now instead, a boolean value
    check_poisoned goes to false when
    a OOB is detected, and is later on given to get_or_create_initial_value.

    gcc/analyzer/ChangeLog:

             * region-model-manager.cc
    (region_model_manager::get_or_create_initial_value): Take an
                     optional boolean value to bypass poisoning checks
             * region-model-manager.h: Update declaration of the above
    function.
             * region-model.cc (region_model::get_store_value): No longer
                     returns on OOB, but rather gives a boolean to
    get_or_create_initial_value.
             (region_model::check_region_access): Update docstring.
             (region_model::check_region_for_write): Update docstring.

    Signed-off-by: benjamin priour <priour.be@gmail.com>
    ---
      gcc/analyzer/region-model-manager.cc |  5 +++--
      gcc/analyzer/region-model-manager.h  |  3 ++-
      gcc/analyzer/region-model.cc         | 15 ++++++++-------
      3 files changed, 13 insertions(+), 10 deletions(-)

    diff --git a/gcc/analyzer/region-model-manager.cc
    b/gcc/analyzer/region-model-manager.cc
    index 1453acf7bc9..4f11ef4bd29 100644
    --- a/gcc/analyzer/region-model-manager.cc
    +++ b/gcc/analyzer/region-model-manager.cc
    @@ -293,9 +293,10 @@ region_model_manager::create_unique_svalue
    (tree type)
         necessary.  */

      const svalue *
    -region_model_manager::get_or_create_initial_value (const region *reg)
    +region_model_manager::get_or_create_initial_value (const region *reg,
    +                                                  bool check_poisoned)
      {
    -  if (!reg->can_have_initial_svalue_p ())
    +  if (!reg->can_have_initial_svalue_p () && check_poisoned)
          return get_or_create_poisoned_svalue (POISON_KIND_UNINIT,
                                               reg->get_type ());

    diff --git a/gcc/analyzer/region-model-manager.h
    b/gcc/analyzer/region-model-manager.h
    index 3340c3ebd1e..ff5333bf07c 100644
    --- a/gcc/analyzer/region-model-manager.h
    +++ b/gcc/analyzer/region-model-manager.h
    @@ -49,7 +49,8 @@ public:
                                                  tree type);
        const svalue *get_or_create_poisoned_svalue (enum poison_kind kind,
                                                    tree type);
    -  const svalue *get_or_create_initial_value (const region *reg);
    +  const svalue *get_or_create_initial_value (const region *reg,
    +                                            bool check_poisoned =
    true);
        const svalue *get_ptr_svalue (tree ptr_type, const region *pointee);
        const svalue *get_or_create_unaryop (tree type, enum tree_code op,
                                            const svalue *arg);
    diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
    index 6bc60f89f3d..187013a37cc 100644
    --- a/gcc/analyzer/region-model.cc
    +++ b/gcc/analyzer/region-model.cc
    @@ -2373,8 +2373,9 @@ region_model::get_store_value (const region *reg,
        if (reg->empty_p ())
          return m_mgr->get_or_create_unknown_svalue (reg->get_type ());

    +  bool check_poisoned = true;
        if (check_region_for_read (reg, ctxt))
    -    return m_mgr->get_or_create_unknown_svalue(reg->get_type());
    +    check_poisoned = false;

        /* Special-case: handle var_decls in the constant pool.  */
        if (const decl_region *decl_reg = reg->dyn_cast_decl_region ())
    @@ -2427,7 +2428,7 @@ region_model::get_store_value (const region *reg,
            == RK_GLOBALS)
          return get_initial_value_for_global (reg);

    -  return m_mgr->get_or_create_initial_value (reg);
    +  return m_mgr->get_or_create_initial_value (reg, check_poisoned);
      }

      /* Return false if REG does not exist, true if it may do.
    @@ -2790,7 +2791,7 @@ 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.
    -   Return TRUE if an UNKNOWN_SVALUE needs be created.
    +   Return TRUE if an OOB access was detected.
         If SVAL_HINT is non-NULL, use it as a hint in diagnostics
         about the value that would be written to REG.  */

    @@ -2804,10 +2805,10 @@ region_model::check_region_access (const
    region *reg,
        if (!ctxt)
          return false;

    -  bool need_unknown_sval = false;
    +  bool oob_access_detected = false;
        check_region_for_taint (reg, dir, ctxt);
        if (!check_region_bounds (reg, dir, sval_hint, ctxt))
    -    need_unknown_sval = true;
    +    oob_access_detected = true;

        switch (dir)
          {
    @@ -2820,7 +2821,7 @@ region_model::check_region_access (const
    region *reg,
            check_for_writable_region (reg, ctxt);
            break;
          }
    -  return need_unknown_sval;
    +  return oob_access_detected;
      }

      /* If CTXT is non-NULL, use it to warn about any problems writing
    to REG.  */
    @@ -2834,7 +2835,7 @@ region_model::check_region_for_write (const
    region *dest_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.  */
    +  Returns TRUE if an OOB read was detected.  */

      bool
      region_model::check_region_for_read (const region *src_reg,
    -- 
    2.34.1

Re: [PATCH] analyzer: Fix regression bug after r14-1632-g9589a46ddadc8b [pr110198]

[David Malcolm]

On Thu, 2023-06-22 at 21:55 +0200, priour.be@gmail.com wrote:
> From: benjamin priour <priour.be@gmail.com>
> 
> Resend with proper subject line ...
> 
> Hi,

Hi Benjamin

> 
> Below is the fix to regression bug
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110198
> Was bootstrapped and regtested successfully on x86_64-linux-gnu
> Considering mishap from last patch, I'd would appreciate if you could
> also regtest it, to be sure :)

I tried this, but it didn't apply cleanly to my working copy.  Which
version of master was this against / when did you last rebase this?  I
see in comment #5 of PR 110198 that the results have been changing.

[...snip...]

> g++.dg/analyzer/pr100244.C was failing after a patch of PR109439.
> The reason was a spurious preemptive return of get_store_value upon out-of-bounds read that
> was preventing further checks. Now instead, a boolean value check_poisoned goes to false when
> a OOB is detected, and is later on given to get_or_create_initial_value.
> 
> gcc/analyzer/ChangeLog:
> 
>         * region-model-manager.cc (region_model_manager::get_or_create_initial_value): Take an
>                 optional boolean value to bypass poisoning checks
>         * region-model-manager.h: Update declaration of the above function.
>         * region-model.cc (region_model::get_store_value): No longer
>                 returns on OOB, but rather gives a boolean to get_or_create_initial_value.
>         (region_model::check_region_access): Update docstring.
>         (region_model::check_region_for_write): Update docstring.

Something's gone a bit wrong with the formatting of the ChangeLog
entries.  Ideally they shouldn't go wider than 74 columns, so they need
a few newlines.  Also, some of the lines have too many leading tabs.

[...snip...]

The content of the patch itself looks reasonable.

Thanks

Dave