From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 2209) id 0513E3858C51; Tue, 29 Mar 2022 00:42:24 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 0513E3858C51 MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="utf-8" From: David Malcolm To: gcc-cvs@gcc.gnu.org Subject: [gcc r12-7869] analyzer: ensure that we purge state when reusing a conjured_svalue [PR105087] X-Act-Checkin: gcc X-Git-Author: David Malcolm X-Git-Refname: refs/heads/master X-Git-Oldrev: 1203e8f7880c9751ece5f5302e413b20f4608a00 X-Git-Newrev: 3734527dfa0d10a50aee2f088d37320000fd65bf Message-Id: <20220329004224.0513E3858C51@sourceware.org> Date: Tue, 29 Mar 2022 00:42:24 +0000 (GMT) X-BeenThere: gcc-cvs@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-cvs mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 29 Mar 2022 00:42:24 -0000 https://gcc.gnu.org/g:3734527dfa0d10a50aee2f088d37320000fd65bf commit r12-7869-g3734527dfa0d10a50aee2f088d37320000fd65bf Author: David Malcolm 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 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 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(); +}