analyzer: Fix regression bug after r14-1632-g9589a46ddadc8b [pr110198]
Checks
Commit Message
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(-)
Comments
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
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
@@ -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 ());
@@ -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);
@@ -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,