From 71fc4655ab86ab66b40165b2cb49c1395ca82a9a Mon Sep 17 00:00:00 2001 From: David Malcolm Date: Wed, 24 Mar 2021 20:47:57 -0400 Subject: [PATCH] analyzer; reset sm-state for SSA names at def-stmts [PR93695,PR99044,PR99716] Various false positives from -fanalyzer involve SSA names in loops, where sm-state associated with an SSA name from one iteration is erroneously reused in a subsequent iteration. For example, PR analyzer/99716 describes a false "double 'fclose' of FILE 'fp'" on: for (i = 0; i < 2; ++i) { FILE *fp = fopen ("/tmp/test", "w"); fprintf (fp, "hello"); fclose (fp); } where the gimple of the loop body is: fp_7 = fopen ("/tmp/test", "w"); __builtin_fwrite ("hello", 1, 5, fp_7); fclose (fp_7); i_10 = i_1 + 1; where fp_7 transitions to "closed" at the fclose, but is not reset at the subsequent fopen, leading to the false positive when the fclose is re-reached. The fix is to reset sm-state for svalues that involve an SSA name at the SSA name's def-stmt, since the def-stmt effectively changes the meaning of those related svalues. gcc/analyzer/ChangeLog: PR analyzer/93695 PR analyzer/99044 PR analyzer/99716 * engine.cc (exploded_node::on_stmt): Clear sm-state involving an SSA name at the def-stmt of that SSA name. * program-state.cc (sm_state_map::purge_state_involving): New. * program-state.h (sm_state_map::purge_state_involving): New decl. * region-model.cc (selftest::test_involves_p): New. (selftest::analyzer_region_model_cc_tests): Call it. * svalue.cc (class involvement_visitor): New class (svalue::involves_p): New. * svalue.h (svalue::involves_p): New decl. gcc/testsuite/ChangeLog: PR analyzer/93695 PR analyzer/99044 PR analyzer/99716 * gcc.dg/analyzer/attr-malloc-CVE-2019-19078-usb-leak.c: Remove xfail. * gcc.dg/analyzer/pr93695-1.c: New test. * gcc.dg/analyzer/pr99044-1.c: New test. * gcc.dg/analyzer/pr99044-2.c: New test. * gcc.dg/analyzer/pr99716-1.c: New test. * gcc.dg/analyzer/pr99716-2.c: New test. * gcc.dg/analyzer/pr99716-3.c: New test. --- gcc/analyzer/engine.cc | 25 ++++++++ gcc/analyzer/program-state.cc | 30 ++++++++++ gcc/analyzer/program-state.h | 3 + gcc/analyzer/region-model.cc | 32 ++++++++++ gcc/analyzer/svalue.cc | 34 +++++++++++ gcc/analyzer/svalue.h | 2 + .../attr-malloc-CVE-2019-19078-usb-leak.c | 4 +- gcc/testsuite/gcc.dg/analyzer/pr93695-1.c | 53 ++++++++++++++++ gcc/testsuite/gcc.dg/analyzer/pr99044-1.c | 60 +++++++++++++++++++ gcc/testsuite/gcc.dg/analyzer/pr99044-2.c | 42 +++++++++++++ gcc/testsuite/gcc.dg/analyzer/pr99716-1.c | 40 +++++++++++++ gcc/testsuite/gcc.dg/analyzer/pr99716-2.c | 34 +++++++++++ gcc/testsuite/gcc.dg/analyzer/pr99716-3.c | 16 +++++ 13 files changed, 372 insertions(+), 3 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr93695-1.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr99044-1.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr99044-2.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr99716-1.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr99716-2.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr99716-3.c diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc index 5792c1428477..d7866b5598b4 100644 --- a/gcc/analyzer/engine.cc +++ b/gcc/analyzer/engine.cc @@ -1244,6 +1244,31 @@ exploded_node::on_stmt (exploded_graph &eg, sm_state_map *new_smap = state->m_checker_states[sm_idx]; impl_sm_context sm_ctxt (eg, sm_idx, sm, this, &old_state, state, old_smap, new_smap); + + /* If we're at the def-stmt of an SSA name, then potentially purge + any sm-state for svalues that involve that SSA name. This avoids + false positives in loops, since a symbolic value referring to the + SSA name will be referring to the previous value of that SSA name. + For example, in: + while ((e = hashmap_iter_next(&iter))) { + struct oid2strbuf *e_strbuf = (struct oid2strbuf *)e; + free (e_strbuf->value); + } + at the def-stmt of e_8: + e_8 = hashmap_iter_next (&iter); + we should purge the "freed" state of: + INIT_VAL(CAST_REG(‘struct oid2strbuf’, (*INIT_VAL(e_8))).value) + which is the "e_strbuf->value" value from the previous iteration, + or we will erroneously report a double-free - the "e_8" within it + refers to the previous value. */ + if (tree lhs = gimple_get_lhs (stmt)) + if (TREE_CODE (lhs) == SSA_NAME) + { + const svalue *sval + = old_state.m_region_model->get_rvalue (lhs, &ctxt); + new_smap->purge_state_involving (sval, eg.get_ext_state ()); + } + /* Allow the state_machine to handle the stmt. */ if (sm.on_stmt (&sm_ctxt, snode, stmt)) unknown_side_effects = false; diff --git a/gcc/analyzer/program-state.cc b/gcc/analyzer/program-state.cc index e427fff59d67..fcea5ce436d2 100644 --- a/gcc/analyzer/program-state.cc +++ b/gcc/analyzer/program-state.cc @@ -586,6 +586,36 @@ sm_state_map::on_unknown_change (const svalue *sval, impl_set_state (*iter, (state_machine::state_t)0, NULL, ext_state); } +/* Purge state for things involving SVAL. + For use when SVAL changes meaning, at the def_stmt on an SSA_NAME. */ + +void +sm_state_map::purge_state_involving (const svalue *sval, + const extrinsic_state &ext_state) +{ + /* Currently svalue::involves_p requires this. */ + if (sval->get_kind () != SK_INITIAL) + return; + + svalue_set svals_to_unset; + + for (map_t::iterator iter = m_map.begin (); + iter != m_map.end (); + ++iter) + { + const svalue *key = (*iter).first; + entry_t e = (*iter).second; + if (!m_sm.can_purge_p (e.m_state)) + continue; + if (key->involves_p (sval)) + svals_to_unset.add (key); + } + + for (svalue_set::iterator iter = svals_to_unset.begin (); + iter != svals_to_unset.end (); ++iter) + impl_set_state (*iter, (state_machine::state_t)0, NULL, ext_state); +} + /* Comparator for imposing an order on sm_state_map instances. */ int diff --git a/gcc/analyzer/program-state.h b/gcc/analyzer/program-state.h index d72945d74498..54fdb5b167c0 100644 --- a/gcc/analyzer/program-state.h +++ b/gcc/analyzer/program-state.h @@ -157,6 +157,9 @@ public: bool is_mutable, const extrinsic_state &ext_state); + void purge_state_involving (const svalue *sval, + const extrinsic_state &ext_state); + iterator_t begin () const { return m_map.begin (); } iterator_t end () const { return m_map.end (); } size_t elements () const { return m_map.elements (); } diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index 96ed549adf63..fb5dc39fc66f 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -5114,6 +5114,37 @@ test_alloca () ASSERT_EQ (model.get_rvalue (p, &ctxt)->get_kind (), SK_POISONED); } +/* Verify that svalue::involves_p works. */ + +static void +test_involves_p () +{ + region_model_manager mgr; + tree int_star = build_pointer_type (integer_type_node); + tree p = build_global_decl ("p", int_star); + tree q = build_global_decl ("q", int_star); + + test_region_model_context ctxt; + region_model model (&mgr); + const svalue *p_init = model.get_rvalue (p, &ctxt); + const svalue *q_init = model.get_rvalue (q, &ctxt); + + ASSERT_TRUE (p_init->involves_p (p_init)); + ASSERT_FALSE (p_init->involves_p (q_init)); + + const region *star_p_reg = mgr.get_symbolic_region (p_init); + const region *star_q_reg = mgr.get_symbolic_region (q_init); + + const svalue *init_star_p = mgr.get_or_create_initial_value (star_p_reg); + const svalue *init_star_q = mgr.get_or_create_initial_value (star_q_reg); + + ASSERT_TRUE (init_star_p->involves_p (p_init)); + ASSERT_FALSE (p_init->involves_p (init_star_p)); + ASSERT_FALSE (init_star_p->involves_p (q_init)); + ASSERT_TRUE (init_star_q->involves_p (q_init)); + ASSERT_FALSE (init_star_q->involves_p (p_init)); +} + /* Run all of the selftests within this file. */ void @@ -5150,6 +5181,7 @@ analyzer_region_model_cc_tests () test_POINTER_PLUS_EXPR_then_MEM_REF (); test_malloc (); test_alloca (); + test_involves_p (); } } // namespace selftest diff --git a/gcc/analyzer/svalue.cc b/gcc/analyzer/svalue.cc index d6305a37b9a6..897e84e8464d 100644 --- a/gcc/analyzer/svalue.cc +++ b/gcc/analyzer/svalue.cc @@ -481,6 +481,40 @@ svalue::cmp_ptr_ptr (const void *p1, const void *p2) return cmp_ptr (sval1, sval2); } +/* Subclass of visitor for use in implementing svalue::involves_p. */ + +class involvement_visitor : public visitor +{ +public: + involvement_visitor (const svalue *needle) + : m_needle (needle), m_found (false) {} + + void visit_initial_svalue (const initial_svalue *candidate) + { + if (candidate == m_needle) + m_found = true; + } + + bool found_p () const { return m_found; } + +private: + const svalue *m_needle; + bool m_found; +}; + +/* Return true iff this svalue is defined in terms of OTHER. */ + +bool +svalue::involves_p (const svalue *other) const +{ + /* Currently only implemented for initial_svalue. */ + gcc_assert (other->get_kind () == SK_INITIAL); + + involvement_visitor v (other); + accept (&v); + return v.found_p (); +} + /* class region_svalue : public svalue. */ /* Implementation of svalue::dump_to_pp vfunc for region_svalue. */ diff --git a/gcc/analyzer/svalue.h b/gcc/analyzer/svalue.h index 672a89cccff7..7fe0ba3a603f 100644 --- a/gcc/analyzer/svalue.h +++ b/gcc/analyzer/svalue.h @@ -136,6 +136,8 @@ public: static int cmp_ptr (const svalue *, const svalue *); static int cmp_ptr_ptr (const void *, const void *); + bool involves_p (const svalue *other) const; + protected: svalue (complexity c, tree type) : m_complexity (c), m_type (type) diff --git a/gcc/testsuite/gcc.dg/analyzer/attr-malloc-CVE-2019-19078-usb-leak.c b/gcc/testsuite/gcc.dg/analyzer/attr-malloc-CVE-2019-19078-usb-leak.c index 905d50ec3f95..e086843c42b6 100644 --- a/gcc/testsuite/gcc.dg/analyzer/attr-malloc-CVE-2019-19078-usb-leak.c +++ b/gcc/testsuite/gcc.dg/analyzer/attr-malloc-CVE-2019-19078-usb-leak.c @@ -192,9 +192,7 @@ static int ath10k_usb_hif_tx_sg(struct ath10k *ar, u8 pipe_id, goto err_free_urb_to_pipe; } - /* TODO: the loop confuses the double-free checker (another - instance of PR analyzer/93695). */ - usb_free_urb(urb); /* { dg-bogus "double-'usb_free_urb' of 'urb'" "" { xfail *-*-* } } */ + usb_free_urb(urb); /* { dg-bogus "double-'usb_free_urb' of 'urb'" } */ } return 0; diff --git a/gcc/testsuite/gcc.dg/analyzer/pr93695-1.c b/gcc/testsuite/gcc.dg/analyzer/pr93695-1.c new file mode 100644 index 000000000000..e0500c49be72 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/pr93695-1.c @@ -0,0 +1,53 @@ +/* { dg-additional-options "-Wno-analyzer-too-complex" } */ +/* TODO: remove the need for this option (PR analyzer/93695). */ + +#define NELEMS 10 +#define ARRAY_SIZE(a) (sizeof (a) / sizeof (a[0])) + +void +test_1 (void) +{ + int *p[NELEMS]; + int i; + + for (i = 0; i < ARRAY_SIZE (p); ++i) + p[i] = __builtin_malloc (sizeof (i)); + + for (i = 0; i < ARRAY_SIZE (p); ++i) + __builtin_free (p [i]); +} + +void +test_2 (int n) +{ + int **p; + int i; + + p = (int **)__builtin_malloc (sizeof (int *) * n); + if (!p) + return; + + for (i = 0; i < n; ++i) + p[i] = __builtin_malloc (sizeof (i)); + + for (i = 0; i < n; ++i) + __builtin_free (p [i]); + + __builtin_free (p); +} + +void +test_3 (int **p, int n) +{ + int i; + for (i = 0; i < n; ++i) + p[i] = __builtin_malloc (sizeof (i)); +} + +void +test_4 (void **p, int n) +{ + int i; + for (i = 0; i < n; ++i) + __builtin_free (p[i]); +} diff --git a/gcc/testsuite/gcc.dg/analyzer/pr99044-1.c b/gcc/testsuite/gcc.dg/analyzer/pr99044-1.c new file mode 100644 index 000000000000..6b5d9010fd96 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/pr99044-1.c @@ -0,0 +1,60 @@ +#include + +struct hashmap_entry { + struct hashmap_entry *next; + unsigned int hash; +}; + +struct strbuf { + size_t alloc; + size_t len; + char *buf; +}; + +struct oid2strbuf { + struct hashmap_entry ent; /* must be the first member! */ + unsigned char key[21]; + struct strbuf *value; +}; + + +struct hashmap_iter { + struct hashmap *map; + struct hashmap_entry *next; + unsigned int tablepos; +}; + +struct hashmap { + struct hashmap_entry **table; + // hashmap_cmp_fn cmpfn; + unsigned int size, tablesize, grow_at, shrink_at; + unsigned disallow_rehash : 1; +}; +void strbuf_init(struct strbuf *, size_t); +void *hashmap_iter_next(struct hashmap_iter *iter); +void hashmap_free(struct hashmap *map, int free_entries); +void hashmap_iter_init(struct hashmap *map, struct hashmap_iter *iter); + +void strbuf_release(struct strbuf *sb) +{ + if (sb->alloc) { /* { dg-bogus "use after 'free'" } */ + free(sb->buf); + strbuf_init(sb, 0); + } +} + +void oid2strbuf_free(struct hashmap *map) { + struct hashmap_iter iter; + struct hashmap_entry *e; + + hashmap_iter_init(map, &iter); + while ((e = hashmap_iter_next(&iter))) { + struct oid2strbuf *e_strbuf = (struct oid2strbuf *)e; + strbuf_release(e_strbuf->value); /* { dg-bogus "use after 'free'" } */ + free(e_strbuf->value); + free(e); + } + + hashmap_free(map, 0); +} + diff --git a/gcc/testsuite/gcc.dg/analyzer/pr99044-2.c b/gcc/testsuite/gcc.dg/analyzer/pr99044-2.c new file mode 100644 index 000000000000..fd71d35d7e4e --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/pr99044-2.c @@ -0,0 +1,42 @@ +struct node +{ + struct node *next; +}; + +void test_1 (struct node *n) +{ + while (n) + { + struct node *next = n->next; + __builtin_free (n); + n = next; + } +} + +extern void *get_ptr (void); + +void test_2 (void) +{ + void *p; + while (p = get_ptr ()) + __builtin_free (p); /* { dg-bogus "double-'free' of 'p'" } */ +} + +extern void **get_ptr_ptr (void); + +void test_3 (void) +{ + void **p; + while (p = get_ptr_ptr ()) + __builtin_free (*p); /* { dg-bogus "double-'free'" } */ +} + +void test_4 (void) +{ + void *p = (void *)0; + while (1) + { + __builtin_free (p); /* { dg-bogus "double-'free' of 'p'" } */ + p = get_ptr (); + } +} diff --git a/gcc/testsuite/gcc.dg/analyzer/pr99716-1.c b/gcc/testsuite/gcc.dg/analyzer/pr99716-1.c new file mode 100644 index 000000000000..6720c3c198b9 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/pr99716-1.c @@ -0,0 +1,40 @@ +#include +#include + +void +test_1 (void) +{ + int i; + + for (i = 0; i < 2; ++i) { + FILE *fp = fopen ("/tmp/test", "w"); + fprintf (fp, "hello:%s ", "world"); + fclose (fp); /* { dg-bogus "double 'fclose'" } */ + } +} + +void +test_2 (void) +{ + int i; + + for (i = 0; i < 2; ++i) { + FILE *fp = fopen ("/tmp/test", "w"); + fprintf (fp, "hello"); + } +} /* { dg-warning "leak of FILE 'fp'" } */ + +FILE *fp3; + +void +test_3 (FILE **fpp) +{ + int i; + + for (i = 0; i < 2; ++i) { + *fpp = fopen ("/tmp/test", "w"); + fprintf (*fpp, "hello"); + fclose (*fpp); /* { dg-bogus "double 'fclose'" } */ + *fpp = NULL; + } +} diff --git a/gcc/testsuite/gcc.dg/analyzer/pr99716-2.c b/gcc/testsuite/gcc.dg/analyzer/pr99716-2.c new file mode 100644 index 000000000000..7c9881c61ff9 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/pr99716-2.c @@ -0,0 +1,34 @@ +/* Reduced from + https://github.com/libguestfs/libguestfs/blob/e0a11061035d47b118c95706240bcc17fd576edc/tests/mount-local/test-parallel-mount-local.c#L299-L335 + which is GPLv2 or later. */ + +#include +#include + +extern int foo (void); + +void +test_mountpoint (const char *mp) +{ + const int nr_passes = 5 + (random () & 31); + int pass; + int ret = 1; + FILE *fp; + + for (pass = 0; pass < nr_passes; ++pass) { + if (foo ()) { + goto error; + } + fp = fopen ("file", "w"); + if (fp == NULL) { + goto error; + } + fprintf (fp, "hello world\n"); + fclose (fp); /* { dg-bogus "double 'fclose'" } */ + } + + ret = 0; + + error: + exit (ret); +} diff --git a/gcc/testsuite/gcc.dg/analyzer/pr99716-3.c b/gcc/testsuite/gcc.dg/analyzer/pr99716-3.c new file mode 100644 index 000000000000..77d450ea3be0 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/pr99716-3.c @@ -0,0 +1,16 @@ +#include + +extern void foo (void *); + +void +test_1 (int nr_passes) +{ + int pass; + void *p; + + for (pass = 0; pass < nr_passes; ++pass) { + p = malloc (1024); + foo (p); + free (p); + } +}