From e0139b2a912585496f23c352f0e2c56895f78fbf Mon Sep 17 00:00:00 2001 From: David Malcolm Date: Wed, 17 Feb 2021 10:37:16 -0500 Subject: [PATCH] analyzer: fix false leak involving params [PR98969] This patch updates the svalue liveness code so that the initial value of parameters at top-level functions to the analysis are treated as live (since the values are presumably still live within the outside-of-the-analysis calling code). This fixes the false leak in PR analyzer/98969 seen on: void test (long int i) { struct foo *f = (struct foo *)i; f->expr = __builtin_malloc (1024); } since the calling code can presumably still access the allocated buffer via: ((struct foo *)i)->expr The patch also removes the expected leak warnings from g++.dg/analyzer/pr99064.C and gcc.dg/analyzer/pr96841.c, which now appear to me to be false positives. gcc/analyzer/ChangeLog: PR analyzer/98969 * constraint-manager.cc (dead_svalue_purger::should_purge_p): Update for change to svalue::live_p. * program-state.cc (sm_state_map::on_liveness_change): Likewise. (program_state::detect_leaks): Likewise. * region-model-reachability.cc (reachable_regions::init_cluster): When dealing with a symbolic region, if the underlying pointer is implicitly live, add the region to the reachable regions. * region-model.cc (region_model::compare_initial_and_pointer): Move logic for detecting initial values of params to initial_svalue::initial_value_of_param_p. * svalue.cc (svalue::live_p): Convert "live_svalues" from a reference to a pointer; support it being NULL. (svalue::implicitly_live_p): Convert first param from a refererence to a pointer. (region_svalue::implicitly_live_p): Likewise. (constant_svalue::implicitly_live_p): Likewise. (initial_svalue::implicitly_live_p): Likewise. Treat the initial values of params for the top level frame as still live. (initial_svalue::initial_value_of_param_p): New function, taken from a test in region_model::compare_initial_and_pointer. (unaryop_svalue::implicitly_live_p): Convert first param from a refererence to a pointer. (binop_svalue::implicitly_live_p): Likewise. (sub_svalue::implicitly_live_p): Likewise. (unmergeable_svalue::implicitly_live_p): Likewise. * svalue.h (svalue::live_p): Likewise. (svalue::implicitly_live_p): Likewise. (region_svalue::implicitly_live_p): Likewise. (constant_svalue::implicitly_live_p): Likewise. (initial_svalue::implicitly_live_p): Likewise. (initial_svalue::initial_value_of_param_p): New decl. (unaryop_svalue::implicitly_live_p): Convert first param from a refererence to a pointer. (binop_svalue::implicitly_live_p): Likewise. (sub_svalue::implicitly_live_p): Likewise. (unmergeable_svalue::implicitly_live_p): Likewise. gcc/testsuite/ChangeLog: PR analyzer/98969 * g++.dg/analyzer/pr99064.C: Convert dg-bogus to dg-warning. * gcc.dg/analyzer/pr96841.c: Add -Wno-analyzer-too-complex to options. Remove false leak directive. * gcc.dg/analyzer/pr98969.c (test_1): Remove xfail from leak false positive. (test_3): New. --- gcc/analyzer/constraint-manager.cc | 2 +- gcc/analyzer/program-state.cc | 4 +-- gcc/analyzer/region-model-reachability.cc | 2 ++ gcc/analyzer/region-model.cc | 14 ++------- gcc/analyzer/svalue.cc | 52 ++++++++++++++++++++++++------- gcc/analyzer/svalue.h | 20 ++++++------ gcc/testsuite/g++.dg/analyzer/pr99064.C | 2 +- gcc/testsuite/gcc.dg/analyzer/pr96841.c | 4 +-- gcc/testsuite/gcc.dg/analyzer/pr98969.c | 9 +++++- 9 files changed, 69 insertions(+), 40 deletions(-) diff --git a/gcc/analyzer/constraint-manager.cc b/gcc/analyzer/constraint-manager.cc index 2342108..4dadd20 100644 --- a/gcc/analyzer/constraint-manager.cc +++ b/gcc/analyzer/constraint-manager.cc @@ -1643,7 +1643,7 @@ public: bool should_purge_p (const svalue *sval) const { - return !sval->live_p (m_live_svalues, m_model); + return !sval->live_p (&m_live_svalues, m_model); } private: diff --git a/gcc/analyzer/program-state.cc b/gcc/analyzer/program-state.cc index 60fed56..e427fff 100644 --- a/gcc/analyzer/program-state.cc +++ b/gcc/analyzer/program-state.cc @@ -523,7 +523,7 @@ sm_state_map::on_liveness_change (const svalue_set &live_svalues, ++iter) { const svalue *iter_sval = (*iter).first; - if (!iter_sval->live_p (live_svalues, model)) + if (!iter_sval->live_p (&live_svalues, model)) { svals_to_unset.add (iter_sval); entry_t e = (*iter).second; @@ -1201,7 +1201,7 @@ program_state::detect_leaks (const program_state &src_state, live in DEST_STATE: either explicitly reachable, or implicitly live based on the set of explicitly reachable svalues. Record those that have ceased to be live. */ - if (!sval->live_p (dest_svalues, dest_state.m_region_model)) + if (!sval->live_p (&dest_svalues, dest_state.m_region_model)) dead_svals.quick_push (sval); } diff --git a/gcc/analyzer/region-model-reachability.cc b/gcc/analyzer/region-model-reachability.cc index a988ffc..087185b 100644 --- a/gcc/analyzer/region-model-reachability.cc +++ b/gcc/analyzer/region-model-reachability.cc @@ -91,6 +91,8 @@ reachable_regions::init_cluster (const region *base_reg) if (const symbolic_region *sym_reg = base_reg->dyn_cast_symbolic_region ()) { const svalue *ptr = sym_reg->get_pointer (); + if (ptr->implicitly_live_p (NULL, m_model)) + add (base_reg, true); switch (ptr->get_kind ()) { default: diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index 2fc07c4..159b0f18 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -1980,18 +1980,8 @@ region_model::compare_initial_and_pointer (const initial_svalue *init, /* If we have a pointer to something within a stack frame, it can't be the initial value of a param. */ if (pointee->maybe_get_frame_region ()) - { - const region *reg = init->get_region (); - if (tree reg_decl = reg->maybe_get_decl ()) - if (TREE_CODE (reg_decl) == SSA_NAME) - { - tree ssa_name = reg_decl; - if (SSA_NAME_IS_DEFAULT_DEF (ssa_name) - && SSA_NAME_VAR (ssa_name) - && TREE_CODE (SSA_NAME_VAR (ssa_name)) == PARM_DECL) - return tristate::TS_FALSE; - } - } + if (init->initial_value_of_param_p ()) + return tristate::TS_FALSE; return tristate::TS_UNKNOWN; } diff --git a/gcc/analyzer/svalue.cc b/gcc/analyzer/svalue.cc index 5bbd05e..d6305a3 100644 --- a/gcc/analyzer/svalue.cc +++ b/gcc/analyzer/svalue.cc @@ -246,15 +246,18 @@ svalue::can_merge_p (const svalue *other, } /* Determine if this svalue is either within LIVE_SVALUES, or is implicitly - live with respect to LIVE_SVALUES and MODEL. */ + live with respect to LIVE_SVALUES and MODEL. + LIVE_SVALUES can be NULL, in which case determine if this svalue is + intrinsically live. */ bool -svalue::live_p (const svalue_set &live_svalues, +svalue::live_p (const svalue_set *live_svalues, const region_model *model) const { /* Determine if SVAL is explicitly live. */ - if (const_cast (live_svalues).contains (this)) - return true; + if (live_svalues) + if (const_cast (live_svalues)->contains (this)) + return true; /* Otherwise, determine if SVAL is implicitly live due to being made of other live svalues. */ @@ -264,7 +267,7 @@ svalue::live_p (const svalue_set &live_svalues, /* Base implementation of svalue::implicitly_live_p. */ bool -svalue::implicitly_live_p (const svalue_set &, const region_model *) const +svalue::implicitly_live_p (const svalue_set *, const region_model *) const { return false; } @@ -512,7 +515,7 @@ region_svalue::accept (visitor *v) const /* Implementation of svalue::implicitly_live_p vfunc for region_svalue. */ bool -region_svalue::implicitly_live_p (const svalue_set &, +region_svalue::implicitly_live_p (const svalue_set *, const region_model *model) const { /* Pointers into clusters that have escaped should be treated as live. */ @@ -609,7 +612,7 @@ constant_svalue::accept (visitor *v) const Constants are implicitly live. */ bool -constant_svalue::implicitly_live_p (const svalue_set &, +constant_svalue::implicitly_live_p (const svalue_set *, const region_model *) const { return true; @@ -749,7 +752,7 @@ initial_svalue::accept (visitor *v) const /* Implementation of svalue::implicitly_live_p vfunc for initial_svalue. */ bool -initial_svalue::implicitly_live_p (const svalue_set &, +initial_svalue::implicitly_live_p (const svalue_set *, const region_model *model) const { /* This svalue may be implicitly live if the region still implicitly @@ -765,6 +768,31 @@ initial_svalue::implicitly_live_p (const svalue_set &, return true; } + /* Assume that the initial values of params for the top level frame + are still live, because (presumably) they're still + live in the external caller. */ + if (initial_value_of_param_p ()) + if (const frame_region *frame_reg = m_reg->maybe_get_frame_region ()) + if (frame_reg->get_calling_frame () == NULL) + return true; + + return false; +} + +/* Return true if this is the initial value of a function parameter. */ + +bool +initial_svalue::initial_value_of_param_p () const +{ + if (tree reg_decl = m_reg->maybe_get_decl ()) + if (TREE_CODE (reg_decl) == SSA_NAME) + { + tree ssa_name = reg_decl; + if (SSA_NAME_IS_DEFAULT_DEF (ssa_name) + && SSA_NAME_VAR (ssa_name) + && TREE_CODE (SSA_NAME_VAR (ssa_name)) == PARM_DECL) + return true; + } return false; } @@ -816,7 +844,7 @@ unaryop_svalue::accept (visitor *v) const /* Implementation of svalue::implicitly_live_p vfunc for unaryop_svalue. */ bool -unaryop_svalue::implicitly_live_p (const svalue_set &live_svalues, +unaryop_svalue::implicitly_live_p (const svalue_set *live_svalues, const region_model *model) const { return get_arg ()->live_p (live_svalues, model); @@ -862,7 +890,7 @@ binop_svalue::accept (visitor *v) const /* Implementation of svalue::implicitly_live_p vfunc for binop_svalue. */ bool -binop_svalue::implicitly_live_p (const svalue_set &live_svalues, +binop_svalue::implicitly_live_p (const svalue_set *live_svalues, const region_model *model) const { return (get_arg0 ()->live_p (live_svalues, model) @@ -919,7 +947,7 @@ sub_svalue::accept (visitor *v) const /* Implementation of svalue::implicitly_live_p vfunc for sub_svalue. */ bool -sub_svalue::implicitly_live_p (const svalue_set &live_svalues, +sub_svalue::implicitly_live_p (const svalue_set *live_svalues, const region_model *model) const { return get_parent ()->live_p (live_svalues, model); @@ -1136,7 +1164,7 @@ unmergeable_svalue::accept (visitor *v) const /* Implementation of svalue::implicitly_live_p vfunc for unmergeable_svalue. */ bool -unmergeable_svalue::implicitly_live_p (const svalue_set &live_svalues, +unmergeable_svalue::implicitly_live_p (const svalue_set *live_svalues, const region_model *model) const { return get_arg ()->live_p (live_svalues, model); diff --git a/gcc/analyzer/svalue.h b/gcc/analyzer/svalue.h index 0703cac..672a89c 100644 --- a/gcc/analyzer/svalue.h +++ b/gcc/analyzer/svalue.h @@ -128,9 +128,9 @@ public: virtual void accept (visitor *v) const = 0; - bool live_p (const svalue_set &live_svalues, + bool live_p (const svalue_set *live_svalues, const region_model *model) const; - virtual bool implicitly_live_p (const svalue_set &live_svalues, + virtual bool implicitly_live_p (const svalue_set *live_svalues, const region_model *model) const; static int cmp_ptr (const svalue *, const svalue *); @@ -194,7 +194,7 @@ public: void dump_to_pp (pretty_printer *pp, bool simple) const FINAL OVERRIDE; void accept (visitor *v) const FINAL OVERRIDE; - bool implicitly_live_p (const svalue_set &, + bool implicitly_live_p (const svalue_set *, const region_model *) const FINAL OVERRIDE; const region * get_pointee () const { return m_reg; } @@ -243,7 +243,7 @@ public: void dump_to_pp (pretty_printer *pp, bool simple) const FINAL OVERRIDE; void accept (visitor *v) const FINAL OVERRIDE; - bool implicitly_live_p (const svalue_set &, + bool implicitly_live_p (const svalue_set *, const region_model *) const FINAL OVERRIDE; tree get_constant () const { return m_cst_expr; } @@ -493,9 +493,11 @@ public: void dump_to_pp (pretty_printer *pp, bool simple) const FINAL OVERRIDE; void accept (visitor *v) const FINAL OVERRIDE; - bool implicitly_live_p (const svalue_set &, + bool implicitly_live_p (const svalue_set *, const region_model *) const FINAL OVERRIDE; + bool initial_value_of_param_p () const; + const region *get_region () const { return m_reg; } private: @@ -564,7 +566,7 @@ public: void dump_to_pp (pretty_printer *pp, bool simple) const FINAL OVERRIDE; void accept (visitor *v) const FINAL OVERRIDE; - bool implicitly_live_p (const svalue_set &, + bool implicitly_live_p (const svalue_set *, const region_model *) const FINAL OVERRIDE; enum tree_code get_op () const { return m_op; } @@ -653,7 +655,7 @@ public: void dump_to_pp (pretty_printer *pp, bool simple) const FINAL OVERRIDE; void accept (visitor *v) const FINAL OVERRIDE; - bool implicitly_live_p (const svalue_set &, + bool implicitly_live_p (const svalue_set *, const region_model *) const FINAL OVERRIDE; enum tree_code get_op () const { return m_op; } @@ -734,7 +736,7 @@ public: void dump_to_pp (pretty_printer *pp, bool simple) const FINAL OVERRIDE; void accept (visitor *v) const FINAL OVERRIDE; - bool implicitly_live_p (const svalue_set &, + bool implicitly_live_p (const svalue_set *, const region_model *) const FINAL OVERRIDE; const svalue *get_parent () const { return m_parent_svalue; } @@ -788,7 +790,7 @@ public: void dump_to_pp (pretty_printer *pp, bool simple) const FINAL OVERRIDE; void accept (visitor *v) const FINAL OVERRIDE; - bool implicitly_live_p (const svalue_set &, + bool implicitly_live_p (const svalue_set *, const region_model *) const FINAL OVERRIDE; const svalue *get_arg () const { return m_arg; } diff --git a/gcc/testsuite/g++.dg/analyzer/pr99064.C b/gcc/testsuite/g++.dg/analyzer/pr99064.C index a002219..9fa54f5 100644 --- a/gcc/testsuite/g++.dg/analyzer/pr99064.C +++ b/gcc/testsuite/g++.dg/analyzer/pr99064.C @@ -34,6 +34,6 @@ struct TPkcs11Token { void list_tokens() { for (__normal_iterator base = list_tokens_token_list.begin();;) { int *add_info = new int; - (*base).add_info = add_info; // { dg-bogus "leak" "PR analyzer/98969" { xfail *-*-* } } + (*base).add_info = add_info; // { dg-warning "leak" } } } diff --git a/gcc/testsuite/gcc.dg/analyzer/pr96841.c b/gcc/testsuite/gcc.dg/analyzer/pr96841.c index d9d35f3..8546661 100644 --- a/gcc/testsuite/gcc.dg/analyzer/pr96841.c +++ b/gcc/testsuite/gcc.dg/analyzer/pr96841.c @@ -1,4 +1,4 @@ -/* { dg-additional-options "-O1 -Wno-builtin-declaration-mismatch" } */ +/* { dg-additional-options "-Wno-analyzer-too-complex -O1 -Wno-builtin-declaration-mismatch" } */ int l8 (void); @@ -18,6 +18,6 @@ bv (__SIZE_TYPE__ ny) { *mf = 0; (*mf)[ny] = (int *) malloc (sizeof (int)); - th ((*mf)[ny]); /* { dg-warning "leak" } */ + th ((*mf)[ny]); } } diff --git a/gcc/testsuite/gcc.dg/analyzer/pr98969.c b/gcc/testsuite/gcc.dg/analyzer/pr98969.c index 8298f26..7e1587d 100644 --- a/gcc/testsuite/gcc.dg/analyzer/pr98969.c +++ b/gcc/testsuite/gcc.dg/analyzer/pr98969.c @@ -8,7 +8,7 @@ test_1 (long int i) { struct foo *f = (struct foo *)i; f->expr = __builtin_malloc (1024); -} /* { dg-bogus "leak" "PR analyzer/98969" { xfail *-*-* } } */ +} /* { dg-bogus "leak" } */ void test_2 (long int i) @@ -16,3 +16,10 @@ test_2 (long int i) __builtin_free (((struct foo *)i)->expr); __builtin_free (((struct foo *)i)->expr); /* { dg-warning "double-'free' of '\\*\\(\\(struct foo \\*\\)i\\)\\.expr'" } */ } + +void +test_3 (void *p) +{ + void **q = (void **)p; + *q = __builtin_malloc (1024); +} -- 2.7.4