public inbox for gcc-cvs@sourceware.org
help / color / mirror / Atom feed
* [gcc r12-7869] analyzer: ensure that we purge state when reusing a conjured_svalue [PR105087]
@ 2022-03-29 0:42 David Malcolm
0 siblings, 0 replies; only message in thread
From: David Malcolm @ 2022-03-29 0:42 UTC (permalink / raw)
To: gcc-cvs
https://gcc.gnu.org/g:3734527dfa0d10a50aee2f088d37320000fd65bf
commit r12-7869-g3734527dfa0d10a50aee2f088d37320000fd65bf
Author: David Malcolm <dmalcolm@redhat.com>
Date: Mon Mar 28 20:41:23 2022 -0400
analyzer: ensure that we purge state when reusing a conjured_svalue [PR105087]
PR analyzer/105087 describes a false positive from
-Wanalyzer-double-free in which the analyzer erroneously considers two
successive inlined vasprintf calls to have allocated the same pointer.
The root cause is that the result written back from vasprintf is a
conjured_svalue, and that we normally purge state when reusing a
conjured_svalue, but various places in the code were calling
region_model_manager::get_or_create_conjured_svalue but failing to
then call region_model::purge_state_involving on the result.
This patch fixes things by moving responsibility for calling
region_model::purge_state_involving into
region_model_manager::get_or_create_conjured_svalue, so that it is
always called when reusing a conjured_svalue, fixing the false positive.
gcc/analyzer/ChangeLog:
PR analyzer/105087
* analyzer.h (class conjured_purge): New forward decl.
* region-model-asm.cc (region_model::on_asm_stmt): Add
conjured_purge param to calls binding_cluster::on_asm and
region_model_manager::get_or_create_conjured_svalue.
* region-model-impl-calls.cc
(call_details::get_or_create_conjured_svalue): Likewise for call
to region_model_manager::get_or_create_conjured_svalue.
(region_model::impl_call_fgets): Remove call to
region_model::purge_state_involving, as this is now done
implicitly by call_details::get_or_create_conjured_svalue.
(region_model::impl_call_fread): Likewise.
(region_model::impl_call_strchr): Pass conjured_purge param to
call to region_model_manager::get_or_create_conjured_svalue.
* region-model-manager.cc (conjured_purge::purge): New.
(region_model_manager::get_or_create_conjured_svalue): Add
param "p". Use it to purge state when reusing an existing
conjured_svalue.
* region-model.cc (region_model::on_call_pre): Replace call to
region_model::purge_state_involving with passing conjured_purge
to region_model_manager::get_or_create_conjured_svalue.
(region_model::handle_unrecognized_call): Pass conjured_purge to
store::on_unknown_fncall.
* region-model.h
(region_model_manager::get_or_create_conjured_svalue): Add param
"p".
* store.cc (binding_cluster::on_unknown_fncall): Likewise. Pass
it on to region_model_manager::get_or_create_conjured_svalue.
(binding_cluster::on_asm): Likewise.
(store::on_unknown_fncall): Add param "p" and pass it on to
binding_cluster::on_unknown_fncall.
* store.h (binding_cluster::on_unknown_fncall): Add param p.
(binding_cluster::on_asm): Likewise.
(store::on_unknown_fncall): Likewise.
* svalue.h (class conjured_purge): New.
gcc/testsuite/ChangeLog:
* gcc.dg/analyzer/pr105087-1.c: New test.
* gcc.dg/analyzer/pr105087-2.c: New test.
* gcc.dg/analyzer/vasprintf-1.c: New test.
Signed-off-by: David Malcolm <dmalcolm@redhat.com>
Diff:
---
gcc/analyzer/analyzer.h | 1 +
gcc/analyzer/region-model-asm.cc | 8 ++--
gcc/analyzer/region-model-impl-calls.cc | 12 +++---
gcc/analyzer/region-model-manager.cc | 27 ++++++++++++--
gcc/analyzer/region-model.cc | 8 ++--
gcc/analyzer/region-model.h | 3 +-
gcc/analyzer/store.cc | 23 +++++++-----
gcc/analyzer/store.h | 9 +++--
gcc/analyzer/svalue.h | 21 +++++++++++
gcc/testsuite/gcc.dg/analyzer/pr105087-1.c | 18 +++++++++
gcc/testsuite/gcc.dg/analyzer/pr105087-2.c | 20 ++++++++++
gcc/testsuite/gcc.dg/analyzer/vasprintf-1.c | 57 +++++++++++++++++++++++++++++
12 files changed, 180 insertions(+), 27 deletions(-)
diff --git a/gcc/analyzer/analyzer.h b/gcc/analyzer/analyzer.h
index a8289d0a0e0..39934a302cb 100644
--- a/gcc/analyzer/analyzer.h
+++ b/gcc/analyzer/analyzer.h
@@ -70,6 +70,7 @@ class region;
class string_region;
class bit_range_region;
class region_model_manager;
+class conjured_purge;
struct model_merger;
class store_manager;
class store;
diff --git a/gcc/analyzer/region-model-asm.cc b/gcc/analyzer/region-model-asm.cc
index c7389f0e38d..bb73e6eed60 100644
--- a/gcc/analyzer/region-model-asm.cc
+++ b/gcc/analyzer/region-model-asm.cc
@@ -272,7 +272,8 @@ region_model::on_asm_stmt (const gasm *stmt, region_model_context *ctxt)
continue;
binding_cluster *cluster = m_store.get_or_create_cluster (base_reg);
- cluster->on_asm (stmt, m_mgr->get_store_manager ());
+ cluster->on_asm (stmt, m_mgr->get_store_manager (),
+ conjured_purge (this, ctxt));
}
/* Update the outputs. */
@@ -292,8 +293,9 @@ region_model::on_asm_stmt (const gasm *stmt, region_model_context *ctxt)
{
sval = m_mgr->get_or_create_conjured_svalue (TREE_TYPE (dst_expr),
stmt,
- dst_reg);
- purge_state_involving (sval, ctxt);
+ dst_reg,
+ conjured_purge (this,
+ ctxt));
}
set_value (dst_reg, sval, ctxt);
}
diff --git a/gcc/analyzer/region-model-impl-calls.cc b/gcc/analyzer/region-model-impl-calls.cc
index 65daa342824..621e7002ffb 100644
--- a/gcc/analyzer/region-model-impl-calls.cc
+++ b/gcc/analyzer/region-model-impl-calls.cc
@@ -212,13 +212,15 @@ call_details::dump (bool simple) const
pp_flush (&pp);
}
-/* Get a conjured_svalue for this call for REG. */
+/* Get a conjured_svalue for this call for REG,
+ and purge any state already relating to that conjured_svalue. */
const svalue *
call_details::get_or_create_conjured_svalue (const region *reg) const
{
region_model_manager *mgr = m_model->get_manager ();
- return mgr->get_or_create_conjured_svalue (reg->get_type (), m_call, reg);
+ return mgr->get_or_create_conjured_svalue (reg->get_type (), m_call, reg,
+ conjured_purge (m_model, m_ctxt));
}
/* Implementations of specific functions. */
@@ -434,7 +436,6 @@ region_model::impl_call_fgets (const call_details &cd)
{
const region *base_reg = reg->get_base_region ();
const svalue *new_sval = cd.get_or_create_conjured_svalue (base_reg);
- purge_state_involving (new_sval, cd.get_ctxt ());
set_value (base_reg, new_sval, cd.get_ctxt ());
}
}
@@ -449,7 +450,6 @@ region_model::impl_call_fread (const call_details &cd)
{
const region *base_reg = reg->get_base_region ();
const svalue *new_sval = cd.get_or_create_conjured_svalue (base_reg);
- purge_state_involving (new_sval, cd.get_ctxt ());
set_value (base_reg, new_sval, cd.get_ctxt ());
}
}
@@ -830,7 +830,9 @@ region_model::impl_call_strchr (const call_details &cd)
const svalue *offset
= mgr->get_or_create_conjured_svalue (size_type_node,
cd.get_call_stmt (),
- str_reg);
+ str_reg,
+ conjured_purge (model,
+ ctxt));
result = mgr->get_or_create_binop (lhs_type, POINTER_PLUS_EXPR,
str_sval, offset);
}
diff --git a/gcc/analyzer/region-model-manager.cc b/gcc/analyzer/region-model-manager.cc
index 61c7fb32628..5ca333a9ed6 100644
--- a/gcc/analyzer/region-model-manager.cc
+++ b/gcc/analyzer/region-model-manager.cc
@@ -1169,17 +1169,38 @@ region_model_manager::get_or_create_compound_svalue (tree type,
return compound_sval;
}
+/* class conjured_purge. */
+
+/* Purge state relating to SVAL. */
+
+void
+conjured_purge::purge (const conjured_svalue *sval) const
+{
+ m_model->purge_state_involving (sval, m_ctxt);
+}
+
/* Return the svalue * of type TYPE for the value conjured for ID_REG
- at STMT, creating it if necessary. */
+ at STMT, creating it if necessary.
+ Use P to purge existing state from the svalue, for the case where a
+ conjured_svalue would be reused along an execution path. */
const svalue *
region_model_manager::get_or_create_conjured_svalue (tree type,
const gimple *stmt,
- const region *id_reg)
+ const region *id_reg,
+ const conjured_purge &p)
{
conjured_svalue::key_t key (type, stmt, id_reg);
if (conjured_svalue **slot = m_conjured_values_map.get (key))
- return *slot;
+ {
+ const conjured_svalue *sval = *slot;
+ /* We're reusing an existing conjured_svalue, perhaps from a different
+ state within this analysis, or perhaps from an earlier state on this
+ execution path. For the latter, purge any state involving the "new"
+ svalue from the current program_state. */
+ p.purge (sval);
+ return sval;
+ }
conjured_svalue *conjured_sval
= new conjured_svalue (type, stmt, id_reg);
RETURN_UNKNOWN_IF_TOO_COMPLEX (conjured_sval);
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 44bd8026a35..816b4100f3a 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -1324,8 +1324,9 @@ region_model::on_call_pre (const gcall *call, region_model_context *ctxt,
use a conjured value, and purge any prior state involving that
value (in case this is in a loop). */
sval = m_mgr->get_or_create_conjured_svalue (TREE_TYPE (lhs), call,
- lhs_region);
- purge_state_involving (sval, ctxt);
+ lhs_region,
+ conjured_purge (this,
+ ctxt));
}
set_value (lhs_region, sval, ctxt);
}
@@ -1802,7 +1803,8 @@ region_model::handle_unrecognized_call (const gcall *call,
/* Update bindings for all clusters that have escaped, whether above,
or previously. */
- m_store.on_unknown_fncall (call, m_mgr->get_store_manager ());
+ m_store.on_unknown_fncall (call, m_mgr->get_store_manager (),
+ conjured_purge (this, ctxt));
/* Purge dynamic extents from any regions that have escaped mutably:
realloc could have been called on them. */
diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h
index 13a2ea94a3f..23841718b5c 100644
--- a/gcc/analyzer/region-model.h
+++ b/gcc/analyzer/region-model.h
@@ -277,7 +277,8 @@ public:
const svalue *get_or_create_compound_svalue (tree type,
const binding_map &map);
const svalue *get_or_create_conjured_svalue (tree type, const gimple *stmt,
- const region *id_reg);
+ const region *id_reg,
+ const conjured_purge &p);
const svalue *
get_or_create_asm_output_svalue (tree type,
const gasm *asm_stmt,
diff --git a/gcc/analyzer/store.cc b/gcc/analyzer/store.cc
index 9aa7d690b04..0014633c335 100644
--- a/gcc/analyzer/store.cc
+++ b/gcc/analyzer/store.cc
@@ -1843,12 +1843,14 @@ binding_cluster::mark_as_escaped ()
/* If this cluster has escaped (by this call, or by an earlier one, or
by being an external param), then unbind all values and mark it
- as "touched", so that it has an unknown value, rather than an
- initial_svalue. */
+ as "touched", so that it has a conjured value, rather than an
+ initial_svalue.
+ Use P to purge state involving conjured_svalues. */
void
binding_cluster::on_unknown_fncall (const gcall *call,
- store_manager *mgr)
+ store_manager *mgr,
+ const conjured_purge &p)
{
if (m_escaped)
{
@@ -1857,25 +1859,27 @@ binding_cluster::on_unknown_fncall (const gcall *call,
/* Bind it to a new "conjured" value using CALL. */
const svalue *sval
= mgr->get_svalue_manager ()->get_or_create_conjured_svalue
- (m_base_region->get_type (), call, m_base_region);
+ (m_base_region->get_type (), call, m_base_region, p);
bind (mgr, m_base_region, sval);
m_touched = true;
}
}
-/* Mark this cluster as having been clobbered by STMT. */
+/* Mark this cluster as having been clobbered by STMT.
+ Use P to purge state involving conjured_svalues. */
void
binding_cluster::on_asm (const gasm *stmt,
- store_manager *mgr)
+ store_manager *mgr,
+ const conjured_purge &p)
{
m_map.empty ();
/* Bind it to a new "conjured" value using CALL. */
const svalue *sval
= mgr->get_svalue_manager ()->get_or_create_conjured_svalue
- (m_base_region->get_type (), stmt, m_base_region);
+ (m_base_region->get_type (), stmt, m_base_region, p);
bind (mgr, m_base_region, sval);
m_touched = true;
@@ -2766,13 +2770,14 @@ store::mark_as_escaped (const region *base_reg)
(either in this fncall, or in a prior one). */
void
-store::on_unknown_fncall (const gcall *call, store_manager *mgr)
+store::on_unknown_fncall (const gcall *call, store_manager *mgr,
+ const conjured_purge &p)
{
m_called_unknown_fn = true;
for (cluster_map_t::iterator iter = m_cluster_map.begin ();
iter != m_cluster_map.end (); ++iter)
- (*iter).second->on_unknown_fncall (call, mgr);
+ (*iter).second->on_unknown_fncall (call, mgr, p);
}
/* Return true if a non-const pointer to BASE_REG (or something within it)
diff --git a/gcc/analyzer/store.h b/gcc/analyzer/store.h
index ee084dd81f3..89bb352f6a5 100644
--- a/gcc/analyzer/store.h
+++ b/gcc/analyzer/store.h
@@ -609,8 +609,10 @@ public:
store_manager *mgr);
void mark_as_escaped ();
- void on_unknown_fncall (const gcall *call, store_manager *mgr);
- void on_asm (const gasm *stmt, store_manager *mgr);
+ void on_unknown_fncall (const gcall *call, store_manager *mgr,
+ const conjured_purge &p);
+ void on_asm (const gasm *stmt, store_manager *mgr,
+ const conjured_purge &p);
bool escaped_p () const { return m_escaped; }
bool touched_p () const { return m_touched; }
@@ -735,7 +737,8 @@ public:
model_merger *merger);
void mark_as_escaped (const region *base_reg);
- void on_unknown_fncall (const gcall *call, store_manager *mgr);
+ void on_unknown_fncall (const gcall *call, store_manager *mgr,
+ const conjured_purge &p);
bool escaped_p (const region *reg) const;
void get_representative_path_vars (const region_model *model,
diff --git a/gcc/analyzer/svalue.h b/gcc/analyzer/svalue.h
index 8040712ec2e..4bbe8588b8d 100644
--- a/gcc/analyzer/svalue.h
+++ b/gcc/analyzer/svalue.h
@@ -1306,6 +1306,27 @@ template <> struct default_hash_traits<compound_svalue::key_t>
namespace ana {
+/* A bundle of state for purging information from a program_state about
+ a conjured_svalue. We pass this whenever calling
+ get_or_create_conjured_svalue, so that if the program_state already
+ has information about this conjured_svalue on an execution path, we
+ can purge that information, to avoid the analyzer confusing the two
+ values as being the same. */
+
+class conjured_purge
+{
+public:
+ conjured_purge (region_model *model, region_model_context *ctxt)
+ : m_model (model), m_ctxt (ctxt)
+ {
+ }
+ void purge (const conjured_svalue *sval) const;
+
+private:
+ region_model *m_model;
+ region_model_context *m_ctxt;
+};
+
/* A defined value arising from a statement, where we want to identify a
particular unknown value, rather than resorting to the unknown_value
singleton, so that the value can have sm-state.
diff --git a/gcc/testsuite/gcc.dg/analyzer/pr105087-1.c b/gcc/testsuite/gcc.dg/analyzer/pr105087-1.c
new file mode 100644
index 00000000000..c4acf429359
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/pr105087-1.c
@@ -0,0 +1,18 @@
+#include "analyzer-decls.h"
+
+extern void *inner_alloc (void);
+
+void * __attribute__((noinline))
+outer_alloc (void)
+{
+ return inner_alloc ();
+}
+
+void test_1 (void)
+{
+ void *p, *q;
+
+ p = outer_alloc ();
+ q = outer_alloc ();
+ __analyzer_eval (p == q); /* { dg-warning "UNKNOWN" } */
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/pr105087-2.c b/gcc/testsuite/gcc.dg/analyzer/pr105087-2.c
new file mode 100644
index 00000000000..7cd6591b820
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/pr105087-2.c
@@ -0,0 +1,20 @@
+#include "analyzer-decls.h"
+
+extern void inner_alloc (void **);
+
+void * __attribute__((noinline))
+outer_alloc (void)
+{
+ void *result;
+ inner_alloc (&result);
+ return result;
+}
+
+void test_1 (void)
+{
+ void *p, *q;
+
+ p = outer_alloc ();
+ q = outer_alloc ();
+ __analyzer_eval (p == q); /* { dg-warning "UNKNOWN" } */
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/vasprintf-1.c b/gcc/testsuite/gcc.dg/analyzer/vasprintf-1.c
new file mode 100644
index 00000000000..061cd008c13
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/vasprintf-1.c
@@ -0,0 +1,57 @@
+/* { dg-additional-options "-Wno-analyzer-too-complex" } */
+
+#define NULL ((void *)0)
+
+extern int printf (const char *__restrict __format, ...);
+extern int vasprintf (char **__restrict __ptr, const char *__restrict __f,
+ __builtin_va_list __arg)
+ __attribute__ ((__nothrow__, __format__ (__printf__, 2, 0))) ;
+extern void free (void *__ptr) __attribute__ ((__nothrow__ , __leaf__));
+
+static char * __attribute__ ((__format__ (__printf__, 1, 2)))
+zasprintf (const char *format, ...)
+{
+ char *resultp;
+ __builtin_va_list args;
+ __builtin_va_start (args, format);
+ int r = vasprintf (&resultp, format, args);
+ __builtin_va_end (args);
+ return r < 0 ? NULL : resultp;
+}
+
+int run_test() {
+ char *buf = NULL;
+ char *bar = NULL;
+ char *baz = NULL;
+ int i = 1232;
+
+ printf("static function check\n");
+
+ buf = zasprintf("i = %d", i);
+ if (buf) {
+ printf("buf = %s\nbuf = %p\n", buf, buf);
+ }
+
+ bar = zasprintf("i = %d - %d", i, i - 13);
+ if (bar) {
+ printf("bar = %s\nbar = %p\n", bar, bar);
+ printf("buf = %s\nbuf = %p\n", buf, buf);
+ }
+
+ baz = zasprintf("No i's here");
+ if (baz) {
+ printf("baz = %s\nbaz = %p\n", baz, baz);
+ printf("bar = %s\nbar = %p\n", bar, bar);
+ printf("buf = %s\nbuf = %p\n", buf, buf);
+ }
+
+ free(buf);
+ free(bar);
+ free(baz);
+
+ return 1;
+}
+
+int main(int argc, char **argv) {
+ return run_test();
+}
^ permalink raw reply [flat|nested] only message in thread
only message in thread, other threads:[~2022-03-29 0:42 UTC | newest]
Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-29 0:42 [gcc r12-7869] analyzer: ensure that we purge state when reusing a conjured_svalue [PR105087] David Malcolm
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).