From e43c5cdd05ef44480616f4bd39b105e8030a59b3 Mon Sep 17 00:00:00 2001 From: David Malcolm Date: Tue, 12 Mar 2019 16:33:04 -0400 Subject: [PATCH 098/138] FIXME: fix malloc-ipa-8.c --- gcc/analyzer/engine.cc | 85 +++++++++++++++++++++------- gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8.c | 47 +++++++++++---- notes.txt | 5 ++ 3 files changed, 106 insertions(+), 31 deletions(-) diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc index 4e1df7c..dae1465 100644 --- a/gcc/analyzer/engine.cc +++ b/gcc/analyzer/engine.cc @@ -63,6 +63,11 @@ struct var_state_t //typedef var_state_t value_type; static tree get_placeholder () { return error_mark_node; } + var_state_t () + : m_var (NULL_TREE), m_state (0) + { + } + var_state_t (tree var, state_machine::state_t state) : m_var (var), m_state (state) {} @@ -826,7 +831,9 @@ public: const sm_instance &sm_inst, const sm_instance &old_sm_inst); - void dfs_traverse (sm_instance sm_inst, backtrace b, + void dfs_traverse (sm_instance sm_inst, + sm_instance sm_inst_from_returning_call, + backtrace b, path_state ps, const supernode *node, const superedge *last_edge); void on_phi (const supernode *node, const gphi *phi, @@ -912,6 +919,25 @@ class set_of_vars public: typedef hash_set::iterator iterator; + set_of_vars () : m_set () {} + set_of_vars (const set_of_vars &other) : m_set () + { + for (iterator i = other.begin (); i != other.end (); ++i) + { + m_set.add (*i); + } + } + + set_of_vars& operator= (const set_of_vars &other) + { + m_set.empty (); + for (iterator i = other.begin (); i != other.end (); ++i) + { + m_set.add (*i); + } + return *this; + } + var_state_t *has_expr (tree expr) const { for (hash_set::iterator iter = m_set.begin (); @@ -1278,7 +1304,9 @@ engine::add_blk_edges_to_cache (const supernode *node, /* FIXME. */ void -engine::dfs_traverse (sm_instance sm_inst, backtrace b, +engine::dfs_traverse (sm_instance sm_inst, + sm_instance sm_inst_from_returning_call, + backtrace b, path_state ps, const supernode *node, const superedge *last_edge) { @@ -1289,6 +1317,8 @@ engine::dfs_traverse (sm_instance sm_inst, backtrace b, if (get_logger ()) { log ("sm_inst: %s", sm_inst.to_str (m_sm).c_str ()); + log ("sm_inst_from_returning_call: %s", + sm_inst_from_returning_call.to_str (m_sm).c_str ()); log ("backtrace: %s", b.to_str ().c_str ()); log ("backtrace.m_path: %s", b.m_path.to_str ().c_str ()); log ("path_state: %s", ps.to_str ().c_str ()); @@ -1296,6 +1326,20 @@ engine::dfs_traverse (sm_instance sm_inst, backtrace b, add_events_for_node (&b.m_path, node, b.m_path.m_stack_depth); + sm_instance per_block_old_sm_inst = sm_inst; + // FIXME: ^^^ does copy-ctor for hash_set do a deep copy? + // Looks like it might be a shallow copy + + /* FIXME: returning call: the 2nd half. */ + if (node->m_returning_call) + { + // FIXME: assert no phi nodes + /* This ensures that transitions for the effect of the call + show up, in this node. */ + sm_inst = sm_inst_from_returning_call; + // TODO: prune to cache! + } + /* Look up to see if we've visited this node in these states before. */ prune_active_vars_to_cache_misses (&sm_inst, node); if (get_logger ()) @@ -1308,10 +1352,6 @@ engine::dfs_traverse (sm_instance sm_inst, backtrace b, return; } - sm_instance per_block_old_sm_inst = sm_inst; - // FIXME: ^^^ does copy-ctor for hash_set do a deep copy? - // Looks like it might be a shallow copy - /* Phi nodes. */ // FIXME: do these have to somehow happen simultaneously? for (gphi_iterator gpi = const_cast(node)->start_phis (); @@ -1337,6 +1377,9 @@ engine::dfs_traverse (sm_instance sm_inst, backtrace b, { if (cgraph_edge *edge = supergraph_call_edge (node->m_fun, call)) { +#if 1 + add_blk_edges_to_cache (node, sm_inst, per_block_old_sm_inst); +#endif follow_call (sm_inst, b, ps, node, edge); // TODO return; } @@ -1417,7 +1460,8 @@ engine::dfs_traverse (sm_instance sm_inst, backtrace b, per_block_old_sm_inst); #endif - dfs_traverse (per_edge_new_sm_inst, new_b, new_ps, succ->m_dest, + dfs_traverse (per_edge_new_sm_inst, per_edge_new_sm_inst, + new_b, new_ps, succ->m_dest, succ); } } @@ -1839,6 +1883,7 @@ engine::restore_after_call (sm_instance sm_inst_in_callee, cgraph_edge *edge, } sm_instance sm_inst_in_caller;// (sm_inst); + sm_inst_in_caller.add_placeholder (); tree callee = edge->callee->get_fun ()->decl; tree result = DECL_RESULT (callee); @@ -1940,7 +1985,7 @@ engine::follow_call (sm_instance sm_inst, backtrace b, path_state ps, } b_in_callee.m_path.m_stack_depth++; - dfs_traverse (callee_sm_inst, b_in_callee, ps, callee_node, + dfs_traverse (callee_sm_inst, callee_sm_inst, b_in_callee, ps, callee_node, NULL /* FIXME */); log ("building summary edges from callee function summary"); @@ -2030,7 +2075,7 @@ engine::follow_call (sm_instance sm_inst, backtrace b, path_state ps, = restore_after_call (callee_summary_sm_inst, edge, rest_of_caller_node, new_b); log ("rest of call is in SN: %i", rest_of_caller_node->m_index); - dfs_traverse (restored_sm, new_b, ps, rest_of_caller_node, + dfs_traverse (callee_sm_inst, restored_sm, new_b, ps, rest_of_caller_node, NULL /* FIXME */); } } @@ -2662,22 +2707,22 @@ engine::dump_analyzer_summary (const supernode *node, tree var) const ++iter) { if ((*iter).m_start.m_varstate.m_var == var) - inform (node->get_start_location (), - "summary: add: %qE: %qs -> %qs", - var, - m_sm.get_state_name ((*iter).m_start.m_varstate.m_state), - m_sm.get_state_name ((*iter).m_end.m_varstate.m_state)); + warning_at (node->get_start_location (), 0, + "summary: add: %qE: %qs -> %qs", + var, + m_sm.get_state_name ((*iter).m_start.m_varstate.m_state), + m_sm.get_state_name ((*iter).m_end.m_varstate.m_state)); } for (set_of_summary_edges::iterator iter = sfx_transition.begin (); iter != sfx_transition.end (); ++iter) { if ((*iter).m_start.m_varstate.m_var == var) - inform (node->get_start_location (), - "summary: transition: %qE: %qs -> %qs", - var, - m_sm.get_state_name ((*iter).m_start.m_varstate.m_state), - m_sm.get_state_name ((*iter).m_end.m_varstate.m_state)); + warning_at (node->get_start_location (), 0, + "summary: transition: %qE: %qs -> %qs", + var, + m_sm.get_state_name ((*iter).m_start.m_varstate.m_state), + m_sm.get_state_name ((*iter).m_end.m_varstate.m_state)); } } @@ -2872,7 +2917,7 @@ run_one_checker (const supergraph &sg, const state_machine &sm, logger *logger) sm_instance sm_inst; sm_inst.add_placeholder (); - e.dfs_traverse (sm_inst, b, ps, + e.dfs_traverse (sm_inst, sm_inst, b, ps, sg.get_node_for_function_entry (fun), NULL); e.emit_candidate_diagnostics (); diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8.c b/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8.c index d630138..2c09d76 100644 --- a/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8.c +++ b/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8.c @@ -1,15 +1,27 @@ +/* Example of a multilevel wrapper around malloc/free, with a double-free. */ + /* { dg-options "--analyzer -fdump-analyzer-summaries -fanalyzer-checker=malloc" } */ #include -void *wrapped_malloc (size_t size) /* { dg-message "summary: add: '': 'start' -> 'unchecked'" } */ +void *wrapped_malloc (size_t size) /* { dg-warning "summary: add: '': 'start' -> 'unchecked'" } */ +/* { dg-message "\\(4\\) entry to 'wrapped_malloc'" "" { target *-*-* } .-1 } */ { return malloc (size); } -void wrapped_free (void *ptr) /* { dg-message "summary: add: 'ptr': 'start' -> 'free'" } */ +void wrapped_free (void *ptr) /* { dg-warning "summary: add: 'ptr': 'start' -> 'free'" } */ +/* { dg-warning "summary: transition: 'ptr': 'unchecked' -> 'free'" "" { target *-*-* } .-1 } */ +/* { dg-warning "summary: transition: 'ptr': 'free' -> 'stop'" "" { target *-*-* } .-2 } */ +/* { dg-message "\\(10\\) entry to 'wrapped_free'" "" { target *-*-* } .-3 } */ +/* { dg-message "\\(17\\) entry to 'wrapped_free'" "" { target *-*-* } .-4 } */ { - free (ptr); + free (ptr); /* { dg-warning "double-free of 'ptr' \\\[CWE-415\\]" } */ + /* { dg-message "\\(11\\) state of 'ptr': 'start' -> 'free'" "" { target *-*-* } .-1 } */ + /* { dg-message "\\(12\\) state of 'ptr': 'unchecked' -> 'free'" "" { target *-*-* } .-2 } */ + /* FIXME: unchecked -> free is correct, but start -> free should be pruned + from the path. */ + /* { dg-message "\\(18\\) here \\('ptr' is in state 'free'\\)" "" { target *-*-* } .-5 } */ } typedef struct boxed_int @@ -18,7 +30,8 @@ typedef struct boxed_int } boxed_int; boxed_int * -make_boxed_int (int i) +make_boxed_int (int i) /* { dg-warning "summary: add: '': 'start' -> 'unchecked'" } */ +/* { dg-message "\\(3\\) entry to 'make_boxed_int'" "" { target *-*-* } .-1 } */ { boxed_int *bi = (boxed_int *)wrapped_malloc (sizeof (boxed_int)); bi->i = i; @@ -26,15 +39,27 @@ make_boxed_int (int i) } void -free_boxed_int (boxed_int *bi) +free_boxed_int (boxed_int *bi) /* { dg-warning "summary: add: 'bi': 'start' -> 'free'" } */ +/* { dg-warning "summary: add: 'bi': 'start' -> 'stop'" "" { target *-*-* } .-1 } */ +/* { dg-message "\\(7\\) entry to 'free_boxed_int'" "" { target *-*-* } .-2 } */ +/* { dg-message "\\(15\\) entry to 'free_boxed_int'" "" { target *-*-* } .-3 } */ { - wrapped_free (bi); + wrapped_free (bi); /* { dg-message "\\(8\\) state of 'ptr': 'start' -> 'unchecked'" } */ + /* { dg-message "\\(9\\) state of 'ptr': 'start' -> 'free'" "" { target *-*-* } .-1 } */ + /* FIXME: these are wrong; they refer to ptr, not bi. + Also, they should be just start->free, or be skipped. */ + /* { dg-message "\\(16\\) calling 'wrapped_free' from 'free_boxed_int'" "" { target *-*-* } .-4 } */ } -void test (int i) +void test (int i) /* { dg-message "\\(1\\) entry to 'test'" } */ { - boxed_int *bi = make_boxed_int (i); - free_boxed_int (bi); - free_boxed_int (bi); /* BUG: double-free. */ + boxed_int *bi = make_boxed_int (i); /* { dg-message "\\(2\\) calling 'make_boxed_int' from 'test'" } */ + /* { dg-message "\\(5\\) returning to 'test' from 'make_boxed_int'" "" { target *-*-* } .-1 } */ + + free_boxed_int (bi); /* { dg-message "\\(6\\) calling 'free_boxed_int' from 'test'" } */ + /* { dg-message "\\(13\\) returning to 'test' from 'free_boxed_int'" "" { target *-*-* } .-1 } */ + + free_boxed_int (bi); /* { dg-message "\\(14\\) calling 'free_boxed_int' from 'test'" } */ } -// TODO: issue a warning at the multi-level double-free + +/* TODO: various strange entries in the path. */ diff --git a/notes.txt b/notes.txt index ee62683..9f95f0f 100644 --- a/notes.txt +++ b/notes.txt @@ -1,3 +1,8 @@ We need a path: we need to explain to the user what the problem is, and to persuade them that there really is a problem. Hence having a path isn't just an incidental detail of the analyzer; it's required. + +Paths ought to be: +* interprocedurally-valid +* feasible +...but it's not clear to what extent we can enforce this. -- 1.8.5.3