diff options
author | David Malcolm <dmalcolm@redhat.com> | 2021-07-15 15:07:07 -0400 |
---|---|---|
committer | David Malcolm <dmalcolm@redhat.com> | 2021-07-15 15:07:07 -0400 |
commit | 33255ad3ac14e3953750fe0f2d82b901c2852ff6 (patch) | |
tree | 24cff3eaf638921b62d7c0586b180af1633b7c3e /gcc/analyzer | |
parent | 98cd4d123aa14598b1f0d54c22663c8200a96d9c (diff) | |
download | gcc-33255ad3ac14e3953750fe0f2d82b901c2852ff6.zip gcc-33255ad3ac14e3953750fe0f2d82b901c2852ff6.tar.gz gcc-33255ad3ac14e3953750fe0f2d82b901c2852ff6.tar.bz2 |
analyzer: reimplement -Wanalyzer-use-of-uninitialized-value [PR95006 et al]
The initial gcc 10 era commit of the analyzer (in
757bf1dff5e8cee34c0a75d06140ca972bfecfa7) had an implementation of
-Wanalyzer-use-of-uninitialized-value, but was sufficiently buggy
that I removed it in 78b9783774bfd3540f38f5b1e3c7fc9f719653d7 before
the release of gcc 10.1
This patch reintroduces the warning, heavily rewritten, with (I hope)
a less buggy implementation this time, for GCC 12.
gcc/analyzer/ChangeLog:
PR analyzer/95006
PR analyzer/94713
PR analyzer/94714
* analyzer.cc (maybe_reconstruct_from_def_stmt): Split out
GIMPLE_ASSIGN case into...
(get_diagnostic_tree_for_gassign_1): New.
(get_diagnostic_tree_for_gassign): New.
* analyzer.h (get_diagnostic_tree_for_gassign): New decl.
* analyzer.opt (Wanalyzer-write-to-string-literal): New.
* constraint-manager.cc (class svalue_purger): New.
(constraint_manager::purge_state_involving): New.
* constraint-manager.h
(constraint_manager::purge_state_involving): New.
* diagnostic-manager.cc (saved_diagnostic::supercedes_p): New.
(dedupe_winners::handle_interactions): New.
(diagnostic_manager::emit_saved_diagnostics): Call it.
* diagnostic-manager.h (saved_diagnostic::supercedes_p): New decl.
* engine.cc (impl_region_model_context::warn): Convert return type
to bool. Return false if the diagnostic isn't saved.
(impl_region_model_context::purge_state_involving): New.
(impl_sm_context::get_state): Use NULL ctxt when querying old
rvalue.
(impl_sm_context::set_next_state): Use new sval when querying old
state.
(class dump_path_diagnostic): Move to region-model.cc
(exploded_node::on_stmt): Move to on_stmt_pre and on_stmt_post.
Remove call to purge_state_involving.
(exploded_node::on_stmt_pre): New, based on the above. Move most
of it to region_model::on_stmt_pre.
(exploded_node::on_stmt_post): Likewise, moving to
region_model::on_stmt_post.
(class stale_jmp_buf): Fix parent class to use curiously recurring
template pattern.
(feasibility_state::maybe_update_for_edge): Call on_call_pre and
on_call_post on gcalls.
* exploded-graph.h (impl_region_model_context::warn): Return bool.
(impl_region_model_context::purge_state_involving): New decl.
(exploded_node::on_stmt_pre): New decl.
(exploded_node::on_stmt_post): New decl.
* pending-diagnostic.h (pending_diagnostic::use_of_uninit_p): New.
(pending_diagnostic::supercedes_p): New.
* program-state.cc (sm_state_map::get_state): Inherit state for
conjured_svalue as well as initial_svalue.
(sm_state_map::purge_state_involving): Also support SK_CONJURED.
* region-model-impl-calls.cc (call_details::get_uncertainty):
Handle m_ctxt being NULL.
(call_details::get_or_create_conjured_svalue): New.
(region_model::impl_call_fgets): New.
(region_model::impl_call_fread): New.
* region-model-manager.cc
(region_model_manager::get_or_create_initial_value): Return an
uninitialized poisoned value for regions that can't have initial
values.
* region-model-reachability.cc
(reachable_regions::mark_escaped_clusters): Handle ctxt being
NULL.
* region-model.cc (region_to_value_map::purge_state_involving): New.
(poisoned_value_diagnostic::use_of_uninit_p): New.
(poisoned_value_diagnostic::emit): Handle POISON_KIND_UNINIT.
(poisoned_value_diagnostic::describe_final_event): Likewise.
(region_model::check_for_poison): New.
(region_model::on_assignment): Call it.
(class dump_path_diagnostic): Move here from engine.cc.
(region_model::on_stmt_pre): New, based on exploded_node::on_stmt.
(region_model::on_call_pre): Move the setting of the LHS to a
conjured svalue to before the checks for specific functions.
Handle "fgets", "fgets_unlocked", and "fread".
(region_model::purge_state_involving): New.
(region_model::handle_unrecognized_call): Handle ctxt being NULL.
(region_model::get_rvalue): Call check_for_poison.
(selftest::test_stack_frames): Use NULL for context when getting
uninitialized rvalue.
(selftest::test_alloca): Likewise.
* region-model.h (region_to_value_map::purge_state_involving): New
decl.
(call_details::get_or_create_conjured_svalue): New decl.
(region_model::on_stmt_pre): New decl.
(region_model::purge_state_involving): New decl.
(region_model::impl_call_fgets): New decl.
(region_model::impl_call_fread): New decl.
(region_model::check_for_poison): New decl.
(region_model_context::warn): Return bool.
(region_model_context::purge_state_involving): New.
(noop_region_model_context::warn): Return bool.
(noop_region_model_context::purge_state_involving): New.
(test_region_model_context:: warn): Return bool.
* region.cc (region::get_memory_space): New.
(region::can_have_initial_svalue_p): New.
(region::involves_p): New.
* region.h (enum memory_space): New.
(region::get_memory_space): New decl.
(region::can_have_initial_svalue_p): New decl.
(region::involves_p): New decl.
* sm-malloc.cc (use_after_free::supercedes_p): New.
* store.cc (binding_cluster::purge_state_involving): New.
(store::purge_state_involving): New.
* store.h (class symbolic_binding): New forward decl.
(binding_key::dyn_cast_symbolic_binding): New.
(symbolic_binding::dyn_cast_symbolic_binding): New.
(binding_cluster::purge_state_involving): New.
(store::purge_state_involving): New.
* svalue.cc (svalue::can_merge_p): Reject attempts to merge
poisoned svalues with other svalues, so that we identify
paths in which a variable is conditionally uninitialized.
(involvement_visitor::visit_conjured_svalue): New.
(svalue::involves_p): Also handle SK_CONJURED.
(poison_kind_to_str): Handle POISON_KIND_UNINIT.
(poisoned_svalue::maybe_fold_bits_within): New.
* svalue.h (enum poison_kind): Add POISON_KIND_UNINIT.
(poisoned_svalue::maybe_fold_bits_within): New decl.
gcc/ChangeLog:
PR analyzer/95006
PR analyzer/94713
PR analyzer/94714
* doc/invoke.texi: Add -Wanalyzer-use-of-uninitialized-value.
gcc/testsuite/ChangeLog:
PR analyzer/95006
PR analyzer/94713
PR analyzer/94714
* g++.dg/analyzer/pr93212.C: Update location of warning.
* g++.dg/analyzer/pr94011.C: Add
-Wno-analyzer-use-of-uninitialized-value.
* g++.dg/analyzer/pr94503.C: Likewise.
* gcc.dg/analyzer/clobbers-1.c: Convert "f" from a local to a
param to avoid uninitialized warning.
* gcc.dg/analyzer/data-model-1.c (test_12): Add test for
uninitialized value on result of alloca.
(test_12a): Add expected warning.
(test_12c): Likewise.
(test_19): Likewise.
(test_29b): Likewise.
(test_29c): Likewise.
(test_37): Remove xfail.
(test_37a): Likewise.
* gcc.dg/analyzer/data-model-20.c: Add warning about leak.
* gcc.dg/analyzer/explode-2.c: Remove params; add
-Wno-analyzer-too-complex, -Wno-analyzer-malloc-leak, and xfails.
Initialize the locals.
* gcc.dg/analyzer/explode-2a.c: Initialize the locals. Add
expected leak.
* gcc.dg/analyzer/fgets-1.c: New test.
* gcc.dg/analyzer/fread-1.c: New test.
* gcc.dg/analyzer/malloc-1.c (test_16): Add expected warning.
(test_40): Likewise.
* gcc.dg/analyzer/memset-CVE-2017-18549-1.c: Check for
uninitialized padding.
* gcc.dg/analyzer/pr93355-localealias-feasibility.c (fread): New
decl.
(read_alias_file): Call it.
* gcc.dg/analyzer/pr94047.c: Add expected warnings.
* gcc.dg/analyzer/pr94851-2.c: Likewise.
* gcc.dg/analyzer/pr96841.c: Convert local to a param.
* gcc.dg/analyzer/pr98628.c: Likewise.
* gcc.dg/analyzer/pr99042.c: Updated expected location of leak
diagnostics.
* gcc.dg/analyzer/symbolic-1.c: Add expected warnings.
* gcc.dg/analyzer/symbolic-7.c: Likewise.
* gcc.dg/analyzer/torture/pr93649.c: Add expected warning. Skip
with -fno-fat-lto-objects.
* gcc.dg/analyzer/uninit-1.c: New test.
* gcc.dg/analyzer/uninit-2.c: New test.
* gcc.dg/analyzer/uninit-3.c: New test.
* gcc.dg/analyzer/uninit-4.c: New test.
* gcc.dg/analyzer/uninit-pr94713.c: New test.
* gcc.dg/analyzer/uninit-pr94714.c: New test.
* gcc.dg/analyzer/use-after-free-2.c: New test.
* gcc.dg/analyzer/use-after-free-3.c: New test.
* gcc.dg/analyzer/zlib-3.c: Add expected warning.
* gcc.dg/analyzer/zlib-6.c: Convert locals to params to avoid
uninitialized warnings. Remove xfail.
* gcc.dg/analyzer/zlib-6a.c: New test, based on the old version
of the above.
* gfortran.dg/analyzer/pr97668.f: Add
-Wno-analyzer-use-of-uninitialized-value and
-Wno-analyzer-too-complex.
Signed-off-by: David Malcolm <dmalcolm@redhat.com>
Diffstat (limited to 'gcc/analyzer')
-rw-r--r-- | gcc/analyzer/analyzer.cc | 95 | ||||
-rw-r--r-- | gcc/analyzer/analyzer.h | 1 | ||||
-rw-r--r-- | gcc/analyzer/analyzer.opt | 4 | ||||
-rw-r--r-- | gcc/analyzer/constraint-manager.cc | 23 | ||||
-rw-r--r-- | gcc/analyzer/constraint-manager.h | 1 | ||||
-rw-r--r-- | gcc/analyzer/diagnostic-manager.cc | 46 | ||||
-rw-r--r-- | gcc/analyzer/diagnostic-manager.h | 2 | ||||
-rw-r--r-- | gcc/analyzer/engine.cc | 250 | ||||
-rw-r--r-- | gcc/analyzer/exploded-graph.h | 15 | ||||
-rw-r--r-- | gcc/analyzer/pending-diagnostic.h | 13 | ||||
-rw-r--r-- | gcc/analyzer/program-state.cc | 43 | ||||
-rw-r--r-- | gcc/analyzer/region-model-impl-calls.cc | 50 | ||||
-rw-r--r-- | gcc/analyzer/region-model-manager.cc | 4 | ||||
-rw-r--r-- | gcc/analyzer/region-model-reachability.cc | 16 | ||||
-rw-r--r-- | gcc/analyzer/region-model.cc | 261 | ||||
-rw-r--r-- | gcc/analyzer/region-model.h | 32 | ||||
-rw-r--r-- | gcc/analyzer/region.cc | 117 | ||||
-rw-r--r-- | gcc/analyzer/region.h | 16 | ||||
-rw-r--r-- | gcc/analyzer/sm-malloc.cc | 19 | ||||
-rw-r--r-- | gcc/analyzer/store.cc | 55 | ||||
-rw-r--r-- | gcc/analyzer/store.h | 10 | ||||
-rw-r--r-- | gcc/analyzer/svalue.cc | 32 | ||||
-rw-r--r-- | gcc/analyzer/svalue.h | 8 |
23 files changed, 880 insertions, 233 deletions
diff --git a/gcc/analyzer/analyzer.cc b/gcc/analyzer/analyzer.cc index a8ee1a1..ddace9a 100644 --- a/gcc/analyzer/analyzer.cc +++ b/gcc/analyzer/analyzer.cc @@ -63,6 +63,51 @@ get_stmt_location (const gimple *stmt, function *fun) static tree fixup_tree_for_diagnostic_1 (tree expr, hash_set<tree> *visited); +/* Attemp to generate a tree for the LHS of ASSIGN_STMT. + VISITED must be non-NULL; it is used to ensure termination. */ + +static tree +get_diagnostic_tree_for_gassign_1 (const gassign *assign_stmt, + hash_set<tree> *visited) +{ + enum tree_code code = gimple_assign_rhs_code (assign_stmt); + + /* Reverse the effect of extract_ops_from_tree during + gimplification. */ + switch (get_gimple_rhs_class (code)) + { + default: + case GIMPLE_INVALID_RHS: + gcc_unreachable (); + case GIMPLE_TERNARY_RHS: + case GIMPLE_BINARY_RHS: + case GIMPLE_UNARY_RHS: + { + tree t = make_node (code); + TREE_TYPE (t) = TREE_TYPE (gimple_assign_lhs (assign_stmt)); + unsigned num_rhs_args = gimple_num_ops (assign_stmt) - 1; + for (unsigned i = 0; i < num_rhs_args; i++) + { + tree op = gimple_op (assign_stmt, i + 1); + if (op) + { + op = fixup_tree_for_diagnostic_1 (op, visited); + if (op == NULL_TREE) + return NULL_TREE; + } + TREE_OPERAND (t, i) = op; + } + return t; + } + case GIMPLE_SINGLE_RHS: + { + tree op = gimple_op (assign_stmt, 1); + op = fixup_tree_for_diagnostic_1 (op, visited); + return op; + } + } +} + /* Subroutine of fixup_tree_for_diagnostic_1, called on SSA names. Attempt to reconstruct a a tree expression for SSA_NAME based on its def-stmt. @@ -91,45 +136,8 @@ maybe_reconstruct_from_def_stmt (tree ssa_name, /* Can't handle these. */ return NULL_TREE; case GIMPLE_ASSIGN: - { - enum tree_code code = gimple_assign_rhs_code (def_stmt); - - /* Reverse the effect of extract_ops_from_tree during - gimplification. */ - switch (get_gimple_rhs_class (code)) - { - default: - case GIMPLE_INVALID_RHS: - gcc_unreachable (); - case GIMPLE_TERNARY_RHS: - case GIMPLE_BINARY_RHS: - case GIMPLE_UNARY_RHS: - { - tree t = make_node (code); - TREE_TYPE (t) = TREE_TYPE (ssa_name); - unsigned num_rhs_args = gimple_num_ops (def_stmt) - 1; - for (unsigned i = 0; i < num_rhs_args; i++) - { - tree op = gimple_op (def_stmt, i + 1); - if (op) - { - op = fixup_tree_for_diagnostic_1 (op, visited); - if (op == NULL_TREE) - return NULL_TREE; - } - TREE_OPERAND (t, i) = op; - } - return t; - } - case GIMPLE_SINGLE_RHS: - { - tree op = gimple_op (def_stmt, 1); - op = fixup_tree_for_diagnostic_1 (op, visited); - return op; - } - } - } - break; + return get_diagnostic_tree_for_gassign_1 + (as_a <const gassign *> (def_stmt), visited); case GIMPLE_CALL: { gcall *call_stmt = as_a <gcall *> (def_stmt); @@ -193,6 +201,15 @@ fixup_tree_for_diagnostic (tree expr) return fixup_tree_for_diagnostic_1 (expr, &visited); } +/* Attempt to generate a tree for the LHS of ASSIGN_STMT. */ + +tree +get_diagnostic_tree_for_gassign (const gassign *assign_stmt) +{ + hash_set<tree> visited; + return get_diagnostic_tree_for_gassign_1 (assign_stmt, &visited); +} + } // namespace ana /* Helper function for checkers. Is the CALL to the given function name, diff --git a/gcc/analyzer/analyzer.h b/gcc/analyzer/analyzer.h index 02830e4..d42bee7 100644 --- a/gcc/analyzer/analyzer.h +++ b/gcc/analyzer/analyzer.h @@ -112,6 +112,7 @@ extern void print_quoted_type (pretty_printer *pp, tree t); extern int readability_comparator (const void *p1, const void *p2); extern int tree_cmp (const void *p1, const void *p2); extern tree fixup_tree_for_diagnostic (tree); +extern tree get_diagnostic_tree_for_gassign (const gassign *); /* A tree, extended with stack frame information for locals, so that we can distinguish between different values of locals within a potentially diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt index 7b77ae8..6ddb6e3 100644 --- a/gcc/analyzer/analyzer.opt +++ b/gcc/analyzer/analyzer.opt @@ -134,6 +134,10 @@ Wanalyzer-write-to-string-literal Common Var(warn_analyzer_write_to_string_literal) Init(1) Warning Warn about code paths which attempt to write to a string literal. +Wanalyzer-use-of-uninitialized-value +Common Var(warn_analyzer_use_of_uninitialized_value) Init(1) Warning +Warn about code paths in which an uninitialized value is used. + Wanalyzer-too-complex Common Var(warn_analyzer_too_complex) Init(0) Warning Warn if the code is too complicated for the analyzer to fully explore. diff --git a/gcc/analyzer/constraint-manager.cc b/gcc/analyzer/constraint-manager.cc index 51cf522..5b5a9de 100644 --- a/gcc/analyzer/constraint-manager.cc +++ b/gcc/analyzer/constraint-manager.cc @@ -1653,6 +1653,29 @@ on_liveness_change (const svalue_set &live_svalues, purge (p, NULL); } +class svalue_purger +{ +public: + svalue_purger (const svalue *sval) : m_sval (sval) {} + + bool should_purge_p (const svalue *sval) const + { + return sval->involves_p (m_sval); + } + +private: + const svalue *m_sval; +}; + +/* Purge any state involving SVAL. */ + +void +constraint_manager::purge_state_involving (const svalue *sval) +{ + svalue_purger p (sval); + purge (p, NULL); +} + /* Comparator for use by constraint_manager::canonicalize. Sort a pair of equiv_class instances, using the representative svalue as a sort key. */ diff --git a/gcc/analyzer/constraint-manager.h b/gcc/analyzer/constraint-manager.h index 3173610..2bb3215 100644 --- a/gcc/analyzer/constraint-manager.h +++ b/gcc/analyzer/constraint-manager.h @@ -269,6 +269,7 @@ public: void on_liveness_change (const svalue_set &live_svalues, const region_model *model); + void purge_state_involving (const svalue *sval); void canonicalize (); diff --git a/gcc/analyzer/diagnostic-manager.cc b/gcc/analyzer/diagnostic-manager.cc index d005fac..631fef6 100644 --- a/gcc/analyzer/diagnostic-manager.cc +++ b/gcc/analyzer/diagnostic-manager.cc @@ -722,6 +722,18 @@ saved_diagnostic::add_duplicate (saved_diagnostic *other) m_duplicates.safe_push (other); } +/* Return true if this diagnostic supercedes OTHER, and that OTHER should + therefore not be emitted. */ + +bool +saved_diagnostic::supercedes_p (const saved_diagnostic &other) const +{ + /* They should be at the same stmt. */ + if (m_stmt != other.m_stmt) + return false; + return m_d->supercedes_p (*other.m_d); +} + /* State for building a checker_path from a particular exploded_path. In particular, this precomputes reachability information: the set of source enodes for which a path be found to the diagnostic enode. */ @@ -1021,6 +1033,38 @@ public: } } + /* Handle interactions between the dedupe winners, so that some + diagnostics can supercede others (of different kinds). + + We want use-after-free to supercede use-of-unitialized-value, + so that if we have these at the same stmt, we don't emit + a use-of-uninitialized, just the use-after-free. */ + + void handle_interactions (diagnostic_manager *dm) + { + LOG_SCOPE (dm->get_logger ()); + auto_vec<const dedupe_key *> superceded; + for (auto outer : m_map) + { + const saved_diagnostic *outer_sd = outer.second; + for (auto inner : m_map) + { + const saved_diagnostic *inner_sd = inner.second; + if (inner_sd->supercedes_p (*outer_sd)) + { + superceded.safe_push (outer.first); + if (dm->get_logger ()) + dm->log ("sd[%i] \"%s\" superceded by sd[%i] \"%s\"", + outer_sd->get_index (), outer_sd->m_d->get_kind (), + inner_sd->get_index (), inner_sd->m_d->get_kind ()); + break; + } + } + } + for (auto iter : superceded) + m_map.remove (iter); + } + /* Emit the simplest diagnostic within each set. */ void emit_best (diagnostic_manager *dm, @@ -1095,6 +1139,8 @@ diagnostic_manager::emit_saved_diagnostics (const exploded_graph &eg) FOR_EACH_VEC_ELT (m_saved_diagnostics, i, sd) best_candidates.add (get_logger (), &pf, sd); + best_candidates.handle_interactions (this); + /* For each dedupe-key, call emit_saved_diagnostic on the "best" saved_diagnostic. */ best_candidates.emit_best (this, eg); diff --git a/gcc/analyzer/diagnostic-manager.h b/gcc/analyzer/diagnostic-manager.h index fc8ac26..ad2eb4d 100644 --- a/gcc/analyzer/diagnostic-manager.h +++ b/gcc/analyzer/diagnostic-manager.h @@ -58,6 +58,8 @@ public: unsigned get_index () const { return m_idx; } + bool supercedes_p (const saved_diagnostic &other) const; + //private: const state_machine *m_sm; const exploded_node *m_enode; diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc index dc07a79..7662a7f 100644 --- a/gcc/analyzer/engine.cc +++ b/gcc/analyzer/engine.cc @@ -108,14 +108,29 @@ impl_region_model_context (program_state *state, { } -void +bool impl_region_model_context::warn (pending_diagnostic *d) { LOG_FUNC (get_logger ()); + if (m_stmt == NULL && m_stmt_finder == NULL) + { + if (get_logger ()) + get_logger ()->log ("rejecting diagnostic: no stmt"); + delete d; + return false; + } if (m_eg) - m_eg->get_diagnostic_manager ().add_diagnostic - (m_enode_for_diag, m_enode_for_diag->get_supernode (), - m_stmt, m_stmt_finder, d); + { + m_eg->get_diagnostic_manager ().add_diagnostic + (m_enode_for_diag, m_enode_for_diag->get_supernode (), + m_stmt, m_stmt_finder, d); + return true; + } + else + { + delete d; + return false; + } } void @@ -155,6 +170,19 @@ impl_region_model_context::get_uncertainty () return m_uncertainty; } +/* Purge state involving SVAL. The region_model has already been purged, + so we only need to purge other state in the program_state: + the sm-state. */ + +void +impl_region_model_context::purge_state_involving (const svalue *sval) +{ + int i; + sm_state_map *smap; + FOR_EACH_VEC_ELT (m_new_state->m_checker_states, i, smap) + smap->purge_state_involving (sval, m_ext_state); +} + /* struct setjmp_record. */ int @@ -230,16 +258,15 @@ public: return model->get_fndecl_for_call (call, &old_ctxt); } - state_machine::state_t get_state (const gimple *stmt, + state_machine::state_t get_state (const gimple *stmt ATTRIBUTE_UNUSED, tree var) { logger * const logger = get_logger (); LOG_FUNC (logger); - impl_region_model_context old_ctxt - (m_eg, m_enode_for_diag, NULL, NULL/*m_enode->get_state ()*/, - NULL, stmt); + /* Use NULL ctxt on this get_rvalue call to avoid triggering + uninitialized value warnings. */ const svalue *var_old_sval - = m_old_state->m_region_model->get_rvalue (var, &old_ctxt); + = m_old_state->m_region_model->get_rvalue (var, NULL); state_machine::state_t current = m_old_smap->get_state (var_old_sval, m_eg.get_ext_state ()); @@ -263,12 +290,6 @@ public: { logger * const logger = get_logger (); LOG_FUNC (logger); - impl_region_model_context old_ctxt - (m_eg, m_enode_for_diag, NULL, NULL/*m_enode->get_state ()*/, - NULL, stmt); - const svalue *var_old_sval - = m_old_state->m_region_model->get_rvalue (var, &old_ctxt); - impl_region_model_context new_ctxt (m_eg, m_enode_for_diag, m_old_state, m_new_state, NULL, @@ -278,8 +299,9 @@ public: const svalue *origin_new_sval = m_new_state->m_region_model->get_rvalue (origin, &new_ctxt); + /* We use the new sval here to avoid issues with uninitialized values. */ state_machine::state_t current - = m_old_smap->get_state (var_old_sval, m_eg.get_ext_state ()); + = m_old_smap->get_state (var_new_sval, m_eg.get_ext_state ()); if (logger) logger->log ("%s: state transition of %qE: %s -> %s", m_sm.get_name (), @@ -1160,26 +1182,6 @@ fndecl_has_gimple_body_p (tree fndecl) namespace ana { -/* A pending_diagnostic subclass for implementing "__analyzer_dump_path". */ - -class dump_path_diagnostic - : public pending_diagnostic_subclass<dump_path_diagnostic> -{ -public: - bool emit (rich_location *richloc) FINAL OVERRIDE - { - inform (richloc, "path"); - return true; - } - - const char *get_kind () const FINAL OVERRIDE { return "dump_path_diagnostic"; } - - bool operator== (const dump_path_diagnostic &) const - { - return true; - } -}; - /* Modify STATE in place, applying the effects of the stmt at this node's point. */ @@ -1218,89 +1220,8 @@ exploded_node::on_stmt (exploded_graph &eg, bool unknown_side_effects = false; bool terminate_path = false; - switch (gimple_code (stmt)) - { - default: - /* No-op for now. */ - break; - - case GIMPLE_ASSIGN: - { - const gassign *assign = as_a <const gassign *> (stmt); - state->m_region_model->on_assignment (assign, &ctxt); - } - break; - - case GIMPLE_ASM: - /* No-op for now. */ - break; - - case GIMPLE_CALL: - { - /* Track whether we have a gcall to a function that's not recognized by - anything, for which we don't have a function body, or for which we - don't know the fndecl. */ - const gcall *call = as_a <const gcall *> (stmt); - - /* Debugging/test support. */ - if (is_special_named_call_p (call, "__analyzer_describe", 2)) - state->m_region_model->impl_call_analyzer_describe (call, &ctxt); - else if (is_special_named_call_p (call, "__analyzer_dump", 0)) - { - /* Handle the builtin "__analyzer_dump" by dumping state - to stderr. */ - state->dump (eg.get_ext_state (), true); - } - else if (is_special_named_call_p (call, "__analyzer_dump_capacity", 1)) - state->m_region_model->impl_call_analyzer_dump_capacity (call, &ctxt); - else if (is_special_named_call_p (call, "__analyzer_dump_path", 0)) - { - /* Handle the builtin "__analyzer_dump_path" by queuing a - diagnostic at this exploded_node. */ - ctxt.warn (new dump_path_diagnostic ()); - } - else if (is_special_named_call_p (call, "__analyzer_dump_region_model", - 0)) - { - /* Handle the builtin "__analyzer_dump_region_model" by dumping - the region model's state to stderr. */ - state->m_region_model->dump (false); - } - else if (is_special_named_call_p (call, "__analyzer_eval", 1)) - state->m_region_model->impl_call_analyzer_eval (call, &ctxt); - else if (is_special_named_call_p (call, "__analyzer_break", 0)) - { - /* Handle the builtin "__analyzer_break" by triggering a - breakpoint. */ - /* TODO: is there a good cross-platform way to do this? */ - raise (SIGINT); - } - else if (is_special_named_call_p (call, - "__analyzer_dump_exploded_nodes", - 1)) - { - /* This is handled elsewhere. */ - } - else if (is_setjmp_call_p (call)) - state->m_region_model->on_setjmp (call, this, &ctxt); - else if (is_longjmp_call_p (call)) - { - on_longjmp (eg, call, state, &ctxt); - return on_stmt_flags::terminate_path (); - } - else - unknown_side_effects - = state->m_region_model->on_call_pre (call, &ctxt, &terminate_path); - } - break; - - case GIMPLE_RETURN: - { - const greturn *return_ = as_a <const greturn *> (stmt); - state->m_region_model->on_return (return_, &ctxt); - } - break; - } + on_stmt_pre (eg, stmt, state, &terminate_path, + &unknown_side_effects, &ctxt); if (terminate_path) return on_stmt_flags::terminate_path (); @@ -1316,41 +1237,71 @@ exploded_node::on_stmt (exploded_graph &eg, 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; } - if (const gcall *call = dyn_cast <const gcall *> (stmt)) - state->m_region_model->on_call_post (call, unknown_side_effects, &ctxt); + on_stmt_post (stmt, state, unknown_side_effects, &ctxt); return on_stmt_flags (); } +/* Handle the pre-sm-state part of STMT, modifying STATE in-place. + Write true to *OUT_TERMINATE_PATH if the path should be terminated. + Write true to *OUT_UNKNOWN_SIDE_EFFECTS if the stmt has unknown + side effects. */ + +void +exploded_node::on_stmt_pre (exploded_graph &eg, + const gimple *stmt, + program_state *state, + bool *out_terminate_path, + bool *out_unknown_side_effects, + region_model_context *ctxt) +{ + /* Handle special-case calls that require the full program_state. */ + if (const gcall *call = dyn_cast <const gcall *> (stmt)) + { + if (is_special_named_call_p (call, "__analyzer_dump", 0)) + { + /* Handle the builtin "__analyzer_dump" by dumping state + to stderr. */ + state->dump (eg.get_ext_state (), true); + return; + } + else if (is_setjmp_call_p (call)) + { + state->m_region_model->on_setjmp (call, this, ctxt); + return; + } + else if (is_longjmp_call_p (call)) + { + on_longjmp (eg, call, state, ctxt); + *out_terminate_path = true; + return; + } + } + + /* Otherwise, defer to m_region_model. */ + state->m_region_model->on_stmt_pre (stmt, + out_terminate_path, + out_unknown_side_effects, + ctxt); +} + +/* Handle the post-sm-state part of STMT, modifying STATE in-place. */ + +void +exploded_node::on_stmt_post (const gimple *stmt, + program_state *state, + bool unknown_side_effects, + region_model_context *ctxt) +{ + if (const gcall *call = dyn_cast <const gcall *> (stmt)) + state->m_region_model->on_call_post (call, unknown_side_effects, ctxt); +} + /* Consider the effect of following superedge SUCC from this node. Return true if it's feasible to follow the edge, or false @@ -1415,7 +1366,7 @@ valid_longjmp_stack_p (const program_point &longjmp_point, where the enclosing function of the "setjmp" has returned (and thus the stack frame no longer exists). */ -class stale_jmp_buf : public pending_diagnostic_subclass<dump_path_diagnostic> +class stale_jmp_buf : public pending_diagnostic_subclass<stale_jmp_buf> { public: stale_jmp_buf (const gcall *setjmp_call, const gcall *longjmp_call, @@ -3763,6 +3714,13 @@ feasibility_state::maybe_update_for_edge (logger *logger, if (const gassign *assign = dyn_cast <const gassign *> (stmt)) m_model.on_assignment (assign, NULL); + else if (const gcall *call = dyn_cast <const gcall *> (stmt)) + { + bool terminate_path; + bool unknown_side_effects + = m_model.on_call_pre (call, NULL, &terminate_path); + m_model.on_call_post (call, unknown_side_effects, NULL); + } else if (const greturn *return_ = dyn_cast <const greturn *> (stmt)) m_model.on_return (return_, NULL); } diff --git a/gcc/analyzer/exploded-graph.h b/gcc/analyzer/exploded-graph.h index 1d8b73d..8f48d8a 100644 --- a/gcc/analyzer/exploded-graph.h +++ b/gcc/analyzer/exploded-graph.h @@ -46,7 +46,7 @@ class impl_region_model_context : public region_model_context uncertainty_t *uncertainty, logger *logger = NULL); - void warn (pending_diagnostic *d) FINAL OVERRIDE; + bool warn (pending_diagnostic *d) FINAL OVERRIDE; void on_svalue_leak (const svalue *) OVERRIDE; void on_liveness_change (const svalue_set &live_svalues, const region_model *model) FINAL OVERRIDE; @@ -74,6 +74,8 @@ class impl_region_model_context : public region_model_context uncertainty_t *get_uncertainty () FINAL OVERRIDE; + void purge_state_involving (const svalue *sval) FINAL OVERRIDE; + exploded_graph *m_eg; log_user m_logger; exploded_node *m_enode_for_diag; @@ -223,6 +225,17 @@ class exploded_node : public dnode<eg_traits> const gimple *stmt, program_state *state, uncertainty_t *uncertainty); + void on_stmt_pre (exploded_graph &eg, + const gimple *stmt, + program_state *state, + bool *out_terminate_path, + bool *out_unknown_side_effects, + region_model_context *ctxt); + void on_stmt_post (const gimple *stmt, + program_state *state, + bool unknown_side_effects, + region_model_context *ctxt); + bool on_edge (exploded_graph &eg, const superedge *succ, program_point *next_point, diff --git a/gcc/analyzer/pending-diagnostic.h b/gcc/analyzer/pending-diagnostic.h index 571fc1b..48e2b3e 100644 --- a/gcc/analyzer/pending-diagnostic.h +++ b/gcc/analyzer/pending-diagnostic.h @@ -154,6 +154,9 @@ class pending_diagnostic /* Hand-coded RTTI: get an ID for the subclass. */ virtual const char *get_kind () const = 0; + /* A vfunc for identifying "use of uninitialized value". */ + virtual bool use_of_uninit_p () const { return false; } + /* Compare for equality with OTHER, which might be of a different subclass. */ @@ -269,6 +272,16 @@ class pending_diagnostic { return false; } + + /* Vfunc for determining that this pending_diagnostic supercedes OTHER, + and that OTHER should therefore not be emitted. + They have already been tested for being at the same stmt. */ + + virtual bool + supercedes_p (const pending_diagnostic &other ATTRIBUTE_UNUSED) const + { + return false; + } }; /* A template to make it easier to make subclasses of pending_diagnostic. diff --git a/gcc/analyzer/program-state.cc b/gcc/analyzer/program-state.cc index 6d60c04..23cfcb0 100644 --- a/gcc/analyzer/program-state.cc +++ b/gcc/analyzer/program-state.cc @@ -372,21 +372,31 @@ sm_state_map::get_state (const svalue *sval, INIT_VAL(foo). */ if (m_sm.inherited_state_p ()) if (region_model_manager *mgr = ext_state.get_model_manager ()) - if (const initial_svalue *init_sval = sval->dyn_cast_initial_svalue ()) - { - const region *reg = init_sval->get_region (); - /* Try recursing upwards (up to the base region for the cluster). */ - if (!reg->base_region_p ()) - if (const region *parent_reg = reg->get_parent_region ()) - { - const svalue *parent_init_sval - = mgr->get_or_create_initial_value (parent_reg); - state_machine::state_t parent_state - = get_state (parent_init_sval, ext_state); - if (parent_state) - return parent_state; - } - } + { + if (const initial_svalue *init_sval = sval->dyn_cast_initial_svalue ()) + { + const region *reg = init_sval->get_region (); + /* Try recursing upwards (up to the base region for the + cluster). */ + if (!reg->base_region_p ()) + if (const region *parent_reg = reg->get_parent_region ()) + { + const svalue *parent_init_sval + = mgr->get_or_create_initial_value (parent_reg); + state_machine::state_t parent_state + = get_state (parent_init_sval, ext_state); + if (parent_state) + return parent_state; + } + } + else if (const sub_svalue *sub_sval = sval->dyn_cast_sub_svalue ()) + { + const svalue *parent_sval = sub_sval->get_parent (); + if (state_machine::state_t parent_state + = get_state (parent_sval, ext_state)) + return parent_state; + } + } return m_sm.get_default_state (sval); } @@ -596,7 +606,8 @@ 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) + if (!(sval->get_kind () == SK_INITIAL + || sval->get_kind () == SK_CONJURED)) return; svalue_set svals_to_unset; diff --git a/gcc/analyzer/region-model-impl-calls.cc b/gcc/analyzer/region-model-impl-calls.cc index 466d397..4be6550 100644 --- a/gcc/analyzer/region-model-impl-calls.cc +++ b/gcc/analyzer/region-model-impl-calls.cc @@ -84,7 +84,10 @@ call_details::call_details (const gcall *call, region_model *model, uncertainty_t * call_details::get_uncertainty () const { - return m_ctxt->get_uncertainty (); + if (m_ctxt) + return m_ctxt->get_uncertainty (); + else + return NULL; } /* If the callsite has a left-hand-side region, set it to RESULT @@ -173,6 +176,15 @@ call_details::dump (bool simple) const pp_flush (&pp); } +/* Get a conjured_svalue for this call for REG. */ + +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); +} + /* Implementations of specific functions. */ /* Handle the on_call_pre part of "alloca". */ @@ -305,6 +317,42 @@ region_model::impl_call_error (const call_details &cd, unsigned min_args, return true; } +/* Handle the on_call_pre part of "fgets" and "fgets_unlocked". */ + +void +region_model::impl_call_fgets (const call_details &cd) +{ + /* Ideally we would bifurcate state here between the + error vs no error cases. */ + const svalue *ptr_sval = cd.get_arg_svalue (0); + if (const region_svalue *ptr_to_region_sval + = ptr_sval->dyn_cast_region_svalue ()) + { + const region *reg = ptr_to_region_sval->get_pointee (); + 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 ()); + } +} + +/* Handle the on_call_pre part of "fread". */ + +void +region_model::impl_call_fread (const call_details &cd) +{ + const svalue *ptr_sval = cd.get_arg_svalue (0); + if (const region_svalue *ptr_to_region_sval + = ptr_sval->dyn_cast_region_svalue ()) + { + const region *reg = ptr_to_region_sval->get_pointee (); + 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 ()); + } +} + /* Handle the on_call_post part of "free", after sm-handling. If the ptr points to an underlying heap region, delete the region, diff --git a/gcc/analyzer/region-model-manager.cc b/gcc/analyzer/region-model-manager.cc index 55acb90..7a52a64 100644 --- a/gcc/analyzer/region-model-manager.cc +++ b/gcc/analyzer/region-model-manager.cc @@ -252,6 +252,10 @@ region_model_manager::get_or_create_unknown_svalue (tree type) const svalue * region_model_manager::get_or_create_initial_value (const region *reg) { + if (!reg->can_have_initial_svalue_p ()) + return get_or_create_poisoned_svalue (POISON_KIND_UNINIT, + reg->get_type ()); + /* The initial value of a cast is a cast of the initial value. */ if (const cast_region *cast_reg = reg->dyn_cast_cast_region ()) { diff --git a/gcc/analyzer/region-model-reachability.cc b/gcc/analyzer/region-model-reachability.cc index e165cda..1f65307 100644 --- a/gcc/analyzer/region-model-reachability.cc +++ b/gcc/analyzer/region-model-reachability.cc @@ -267,7 +267,6 @@ reachable_regions::handle_parm (const svalue *sval, tree param_type) void reachable_regions::mark_escaped_clusters (region_model_context *ctxt) { - gcc_assert (ctxt); auto_vec<const function_region *> escaped_fn_regs (m_mutable_base_regs.elements ()); for (hash_set<const region *>::iterator iter = m_mutable_base_regs.begin (); @@ -281,12 +280,15 @@ reachable_regions::mark_escaped_clusters (region_model_context *ctxt) if (const function_region *fn_reg = base_reg->dyn_cast_function_region ()) escaped_fn_regs.quick_push (fn_reg); } - /* Sort to ensure deterministic results. */ - escaped_fn_regs.qsort (region::cmp_ptr_ptr); - unsigned i; - const function_region *fn_reg; - FOR_EACH_VEC_ELT (escaped_fn_regs, i, fn_reg) - ctxt->on_escaped_function (fn_reg->get_fndecl ()); + if (ctxt) + { + /* Sort to ensure deterministic results. */ + escaped_fn_regs.qsort (region::cmp_ptr_ptr); + unsigned i; + const function_region *fn_reg; + FOR_EACH_VEC_ELT (escaped_fn_regs, i, fn_reg) + ctxt->on_escaped_function (fn_reg->get_fndecl ()); + } } /* Dump SET to PP, sorting it to avoid churn when comparing dumps. */ diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index acbbd11..3fe2cce 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -221,6 +221,23 @@ region_to_value_map::can_merge_with_p (const region_to_value_map &other, return true; } +/* Purge any state involving SVAL. */ + +void +region_to_value_map::purge_state_involving (const svalue *sval) +{ + auto_vec<const region *> to_purge; + for (auto iter : *this) + { + const region *iter_reg = iter.first; + const svalue *iter_sval = iter.second; + if (iter_reg->involves_p (sval) || iter_sval->involves_p (sval)) + to_purge.safe_push (iter_reg); + } + for (auto iter : to_purge) + m_hash_map.remove (iter); +} + /* class region_model. */ /* Ctor for region_model: construct an "empty" model. */ @@ -442,6 +459,11 @@ public: const char *get_kind () const FINAL OVERRIDE { return "poisoned_value_diagnostic"; } + bool use_of_uninit_p () const FINAL OVERRIDE + { + return m_pkind == POISON_KIND_UNINIT; + } + bool operator== (const poisoned_value_diagnostic &other) const { return m_expr == other.m_expr; @@ -453,6 +475,16 @@ public: { default: gcc_unreachable (); + case POISON_KIND_UNINIT: + { + diagnostic_metadata m; + m.add_cwe (457); /* "CWE-457: Use of Uninitialized Variable". */ + return warning_meta (rich_loc, m, + OPT_Wanalyzer_use_of_uninitialized_value, + "use of uninitialized value %qE", + m_expr); + } + break; case POISON_KIND_FREED: { diagnostic_metadata m; @@ -482,6 +514,9 @@ public: { default: gcc_unreachable (); + case POISON_KIND_UNINIT: + return ev.formatted_print ("use of uninitialized value %qE here", + m_expr); case POISON_KIND_FREED: return ev.formatted_print ("use after %<free%> of %qE here", m_expr); @@ -782,6 +817,41 @@ region_model::get_gassign_result (const gassign *assign, } } +/* Check for SVAL being poisoned, adding a warning to CTXT. + Return SVAL, or, if a warning is added, another value, to avoid + repeatedly complaining about the same poisoned value in followup code. */ + +const svalue * +region_model::check_for_poison (const svalue *sval, + tree expr, + region_model_context *ctxt) const +{ + if (!ctxt) + return sval; + + if (const poisoned_svalue *poisoned_sval = sval->dyn_cast_poisoned_svalue ()) + { + /* If we have an SSA name for a temporary, we don't want to print + '<unknown>'. + Poisoned values are shared by type, and so we can't reconstruct + the tree other than via the def stmts, using + fixup_tree_for_diagnostic. */ + tree diag_arg = fixup_tree_for_diagnostic (expr); + enum poison_kind pkind = poisoned_sval->get_poison_kind (); + if (ctxt->warn (new poisoned_value_diagnostic (diag_arg, pkind))) + { + /* We only want to report use of a poisoned value at the first + place it gets used; return an unknown value to avoid generating + a chain of followup warnings. */ + sval = m_mgr->get_or_create_unknown_svalue (sval->get_type ()); + } + + return sval; + } + + return sval; +} + /* Update this model for the ASSIGN stmt, using CTXT to report any diagnostics. */ @@ -798,6 +868,8 @@ region_model::on_assignment (const gassign *assign, region_model_context *ctxt) for some SVALUE. */ if (const svalue *sval = get_gassign_result (assign, ctxt)) { + tree expr = get_diagnostic_tree_for_gassign (assign); + check_for_poison (sval, expr, ctxt); set_value (lhs_reg, sval, ctxt); return; } @@ -863,6 +935,109 @@ region_model::on_assignment (const gassign *assign, region_model_context *ctxt) } } +/* A pending_diagnostic subclass for implementing "__analyzer_dump_path". */ + +class dump_path_diagnostic + : public pending_diagnostic_subclass<dump_path_diagnostic> +{ +public: + bool emit (rich_location *richloc) FINAL OVERRIDE + { + inform (richloc, "path"); + return true; + } + + const char *get_kind () const FINAL OVERRIDE { return "dump_path_diagnostic"; } + + bool operator== (const dump_path_diagnostic &) const + { + return true; + } +}; + +/* Handle the pre-sm-state part of STMT, modifying this object in-place. + Write true to *OUT_TERMINATE_PATH if the path should be terminated. + Write true to *OUT_UNKNOWN_SIDE_EFFECTS if the stmt has unknown + side effects. */ + +void +region_model::on_stmt_pre (const gimple *stmt, + bool *out_terminate_path, + bool *out_unknown_side_effects, + region_model_context *ctxt) +{ + switch (gimple_code (stmt)) + { + default: + /* No-op for now. */ + break; + + case GIMPLE_ASSIGN: + { + const gassign *assign = as_a <const gassign *> (stmt); + on_assignment (assign, ctxt); + } + break; + + case GIMPLE_ASM: + /* No-op for now. */ + break; + + case GIMPLE_CALL: + { + /* Track whether we have a gcall to a function that's not recognized by + anything, for which we don't have a function body, or for which we + don't know the fndecl. */ + const gcall *call = as_a <const gcall *> (stmt); + + /* Debugging/test support. */ + if (is_special_named_call_p (call, "__analyzer_describe", 2)) + impl_call_analyzer_describe (call, ctxt); + else if (is_special_named_call_p (call, "__analyzer_dump_capacity", 1)) + impl_call_analyzer_dump_capacity (call, ctxt); + else if (is_special_named_call_p (call, "__analyzer_dump_path", 0)) + { + /* Handle the builtin "__analyzer_dump_path" by queuing a + diagnostic at this exploded_node. */ + ctxt->warn (new dump_path_diagnostic ()); + } + else if (is_special_named_call_p (call, "__analyzer_dump_region_model", + 0)) + { + /* Handle the builtin "__analyzer_dump_region_model" by dumping + the region model's state to stderr. */ + dump (false); + } + else if (is_special_named_call_p (call, "__analyzer_eval", 1)) + impl_call_analyzer_eval (call, ctxt); + else if (is_special_named_call_p (call, "__analyzer_break", 0)) + { + /* Handle the builtin "__analyzer_break" by triggering a + breakpoint. */ + /* TODO: is there a good cross-platform way to do this? */ + raise (SIGINT); + } + else if (is_special_named_call_p (call, + "__analyzer_dump_exploded_nodes", + 1)) + { + /* This is handled elsewhere. */ + } + else + *out_unknown_side_effects = on_call_pre (call, ctxt, + out_terminate_path); + } + break; + + case GIMPLE_RETURN: + { + const greturn *return_ = as_a <const greturn *> (stmt); + on_return (return_, ctxt); + } + break; + } +} + /* Update this model for the CALL stmt, using CTXT to report any diagnostics - the first half. @@ -885,6 +1060,22 @@ region_model::on_call_pre (const gcall *call, region_model_context *ctxt, bool unknown_side_effects = false; + /* Some of the cases below update the lhs of the call based on the + return value, but not all. Provide a default value, which may + get overwritten below. */ + if (tree lhs = gimple_call_lhs (call)) + { + const region *lhs_region = get_lvalue (lhs, ctxt); + if (TREE_CODE (lhs) == SSA_NAME) + { + const svalue *sval + = m_mgr->get_or_create_conjured_svalue (TREE_TYPE (lhs), call, + lhs_region); + purge_state_involving (sval, ctxt); + set_value (lhs_region, sval, ctxt); + } + } + if (gimple_call_internal_p (call)) { switch (gimple_call_internal_fn (call)) @@ -994,6 +1185,17 @@ region_model::on_call_pre (const gcall *call, region_model_context *ctxt, else unknown_side_effects = true; } + else if (is_named_call_p (callee_fndecl, "fgets", call, 3) + || is_named_call_p (callee_fndecl, "fgets_unlocked", call, 3)) + { + impl_call_fgets (cd); + return false; + } + else if (is_named_call_p (callee_fndecl, "fread", call, 4)) + { + impl_call_fread (cd); + return false; + } else if (is_named_call_p (callee_fndecl, "getchar", call, 0)) { /* No side-effects (tracking stream state is out-of-scope @@ -1029,19 +1231,6 @@ region_model::on_call_pre (const gcall *call, region_model_context *ctxt, else unknown_side_effects = true; - /* Some of the above cases update the lhs of the call based on the - return value. If we get here, it hasn't been done yet, so do that - now. */ - if (tree lhs = gimple_call_lhs (call)) - { - const region *lhs_region = get_lvalue (lhs, ctxt); - if (TREE_CODE (lhs) == SSA_NAME) - { - const svalue *sval = m_mgr->get_or_create_initial_value (lhs_region); - set_value (lhs_region, sval, ctxt); - } - } - return unknown_side_effects; } @@ -1090,6 +1279,38 @@ region_model::on_call_post (const gcall *call, handle_unrecognized_call (call, ctxt); } +/* Purge state involving SVAL from this region_model, using CTXT + (if non-NULL) to purge other state in a program_state. + + For example, if we're at the def-stmt of an SSA name, then we need to + purge any 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. */ + +void +region_model::purge_state_involving (const svalue *sval, + region_model_context *ctxt) +{ + m_store.purge_state_involving (sval, m_mgr); + m_constraints->purge_state_involving (sval); + m_dynamic_extents.purge_state_involving (sval); + if (ctxt) + ctxt->purge_state_involving (sval); +} + /* Handle a call CALL to a function with unknown behavior. Traverse the regions in this model, determining what regions are @@ -1135,7 +1356,7 @@ region_model::handle_unrecognized_call (const gcall *call, } } - uncertainty_t *uncertainty = ctxt->get_uncertainty (); + uncertainty_t *uncertainty = ctxt ? ctxt->get_uncertainty () : NULL; /* Purge sm-state for the svalues that were reachable, both in non-mutable and mutable form. */ @@ -1144,14 +1365,16 @@ region_model::handle_unrecognized_call (const gcall *call, iter != reachable_regs.end_reachable_svals (); ++iter) { const svalue *sval = (*iter); - ctxt->on_unknown_change (sval, false); + if (ctxt) + ctxt->on_unknown_change (sval, false); } for (svalue_set::iterator iter = reachable_regs.begin_mutable_svals (); iter != reachable_regs.end_mutable_svals (); ++iter) { const svalue *sval = (*iter); - ctxt->on_unknown_change (sval, true); + if (ctxt) + ctxt->on_unknown_change (sval, true); if (uncertainty) uncertainty->on_mutable_sval_at_unknown_call (sval); } @@ -1603,6 +1826,8 @@ region_model::get_rvalue (path_var pv, region_model_context *ctxt) const assert_compat_types (result_sval->get_type (), TREE_TYPE (pv.m_tree)); + result_sval = check_for_poison (result_sval, pv.m_tree, ctxt); + return result_sval; } @@ -4307,7 +4532,7 @@ test_stack_frames () /* Verify that p (which was pointing at the local "x" in the popped frame) has been poisoned. */ - const svalue *new_p_sval = model.get_rvalue (p, &ctxt); + const svalue *new_p_sval = model.get_rvalue (p, NULL); ASSERT_EQ (new_p_sval->get_kind (), SK_POISONED); ASSERT_EQ (new_p_sval->dyn_cast_poisoned_svalue ()->get_poison_kind (), POISON_KIND_POPPED_STACK); @@ -5397,7 +5622,7 @@ test_alloca () /* Verify that the pointers to the alloca region are replaced by poisoned values when the frame is popped. */ model.pop_frame (NULL, NULL, &ctxt); - ASSERT_EQ (model.get_rvalue (p, &ctxt)->get_kind (), SK_POISONED); + ASSERT_EQ (model.get_rvalue (p, NULL)->get_kind (), SK_POISONED); } /* Verify that svalue::involves_p works. */ diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h index cf5232d..71f6b3e 100644 --- a/gcc/analyzer/region-model.h +++ b/gcc/analyzer/region-model.h @@ -171,6 +171,8 @@ public: bool can_merge_with_p (const region_to_value_map &other, region_to_value_map *out) const; + void purge_state_involving (const svalue *sval); + private: hash_map_t m_hash_map; }; @@ -470,6 +472,8 @@ public: void dump_to_pp (pretty_printer *pp, bool simple) const; void dump (bool simple) const; + const svalue *get_or_create_conjured_svalue (const region *) const; + private: const gcall *m_call; region_model *m_model; @@ -518,6 +522,12 @@ class region_model void canonicalize (); bool canonicalized_p () const; + void + on_stmt_pre (const gimple *stmt, + bool *out_terminate_path, + bool *out_unknown_side_effects, + region_model_context *ctxt); + void on_assignment (const gassign *stmt, region_model_context *ctxt); const svalue *get_gassign_result (const gassign *assign, region_model_context *ctxt); @@ -527,6 +537,8 @@ class region_model bool unknown_side_effects, region_model_context *ctxt); + void purge_state_involving (const svalue *sval, region_model_context *ctxt); + /* Specific handling for on_call_pre. */ bool impl_call_alloca (const call_details &cd); void impl_call_analyzer_describe (const gcall *call, @@ -539,6 +551,8 @@ class region_model bool impl_call_calloc (const call_details &cd); bool impl_call_error (const call_details &cd, unsigned min_args, bool *out_terminate_path); + void impl_call_fgets (const call_details &cd); + void impl_call_fread (const call_details &cd); void impl_call_free (const call_details &cd); bool impl_call_malloc (const call_details &cd); void impl_call_memcpy (const call_details &cd); @@ -727,6 +741,10 @@ class region_model bool called_from_main_p () const; const svalue *get_initial_value_for_global (const region *reg) const; + const svalue *check_for_poison (const svalue *sval, + tree expr, + region_model_context *ctxt) const; + void check_for_writable_region (const region* dest_reg, region_model_context *ctxt) const; @@ -757,7 +775,9 @@ class region_model class region_model_context { public: - virtual void warn (pending_diagnostic *d) = 0; + /* Hook for clients to store pending diagnostics. + Return true if the diagnostic was stored, or false if it was deleted. */ + virtual bool warn (pending_diagnostic *d) = 0; /* Hook for clients to be notified when an SVAL that was reachable in a previous state is no longer live, so that clients can emit warnings @@ -799,6 +819,9 @@ class region_model_context virtual void on_escaped_function (tree fndecl) = 0; virtual uncertainty_t *get_uncertainty () = 0; + + /* Hook for clients to purge state involving SVAL. */ + virtual void purge_state_involving (const svalue *sval) = 0; }; /* A "do nothing" subclass of region_model_context. */ @@ -806,7 +829,7 @@ class region_model_context class noop_region_model_context : public region_model_context { public: - void warn (pending_diagnostic *) OVERRIDE {} + bool warn (pending_diagnostic *) OVERRIDE { return false; } void on_svalue_leak (const svalue *) OVERRIDE {} void on_liveness_change (const svalue_set &, const region_model *) OVERRIDE {} @@ -829,6 +852,8 @@ public: void on_escaped_function (tree) OVERRIDE {} uncertainty_t *get_uncertainty () OVERRIDE { return NULL; } + + void purge_state_involving (const svalue *sval ATTRIBUTE_UNUSED) OVERRIDE {} }; /* A subclass of region_model_context for determining if operations fail @@ -931,9 +956,10 @@ using namespace ::selftest; class test_region_model_context : public noop_region_model_context { public: - void warn (pending_diagnostic *d) FINAL OVERRIDE + bool warn (pending_diagnostic *d) FINAL OVERRIDE { m_diagnostics.safe_push (d); + return true; } unsigned get_num_diagnostics () const { return m_diagnostics.length (); } diff --git a/gcc/analyzer/region.cc b/gcc/analyzer/region.cc index 4633717..6cccb0f 100644 --- a/gcc/analyzer/region.cc +++ b/gcc/analyzer/region.cc @@ -168,6 +168,109 @@ region::maybe_get_frame_region () const return NULL; } +/* Get the memory space of this region. */ + +enum memory_space +region::get_memory_space () const +{ + const region *iter = this; + while (iter) + { + switch (iter->get_kind ()) + { + default: + break; + case RK_GLOBALS: + return MEMSPACE_GLOBALS; + case RK_CODE: + case RK_FUNCTION: + case RK_LABEL: + return MEMSPACE_CODE; + case RK_FRAME: + case RK_STACK: + case RK_ALLOCA: + return MEMSPACE_STACK; + case RK_HEAP: + case RK_HEAP_ALLOCATED: + return MEMSPACE_HEAP; + case RK_STRING: + return MEMSPACE_READONLY_DATA; + } + if (iter->get_kind () == RK_CAST) + iter = iter->dyn_cast_cast_region ()->get_original_region (); + else + iter = iter->get_parent_region (); + } + return MEMSPACE_UNKNOWN; +} + +/* Subroutine for use by region_model_manager::get_or_create_initial_value. + Return true if this region has an initial_svalue. + Return false if attempting to use INIT_VAL(this_region) should give + the "UNINITIALIZED" poison value. */ + +bool +region::can_have_initial_svalue_p () const +{ + const region *base_reg = get_base_region (); + + /* Check for memory spaces that are uninitialized by default. */ + enum memory_space mem_space = base_reg->get_memory_space (); + switch (mem_space) + { + default: + gcc_unreachable (); + case MEMSPACE_UNKNOWN: + case MEMSPACE_CODE: + case MEMSPACE_GLOBALS: + case MEMSPACE_READONLY_DATA: + /* Such regions have initial_svalues. */ + return true; + + case MEMSPACE_HEAP: + /* Heap allocations are uninitialized by default. */ + return false; + + case MEMSPACE_STACK: + if (tree decl = base_reg->maybe_get_decl ()) + { + /* See the assertion in frame_region::get_region_for_local for the + tree codes we need to handle here. */ + switch (TREE_CODE (decl)) + { + default: + gcc_unreachable (); + + case PARM_DECL: + /* Parameters have initial values. */ + return true; + + case VAR_DECL: + case RESULT_DECL: + /* Function locals don't have initial values. */ + return false; + + case SSA_NAME: + { + tree ssa_name = decl; + /* SSA names that are the default defn of a PARM_DECL + have initial_svalues; other SSA names don't. */ + if (SSA_NAME_IS_DEFAULT_DEF (ssa_name) + && SSA_NAME_VAR (ssa_name) + && TREE_CODE (SSA_NAME_VAR (ssa_name)) == PARM_DECL) + return true; + else + return false; + } + } + } + + /* If we have an on-stack region that isn't associated with a decl + or SSA name, then we have VLA/alloca, which is uninitialized. */ + return false; + } +} + /* If this region is a decl_region, return the decl. Otherwise return NULL. */ @@ -584,6 +687,20 @@ region::non_null_p () const } } +/* Return true iff this region is defined in terms of SVAL. */ + +bool +region::involves_p (const svalue *sval) const +{ + if (const symbolic_region *symbolic_reg = dyn_cast_symbolic_region ()) + { + if (symbolic_reg->get_pointer ()->involves_p (sval)) + return true; + } + + return false; +} + /* Comparator for trees to impose a deterministic ordering on T1 and T2. */ diff --git a/gcc/analyzer/region.h b/gcc/analyzer/region.h index 353d5c4..a17e73c 100644 --- a/gcc/analyzer/region.h +++ b/gcc/analyzer/region.h @@ -25,6 +25,18 @@ along with GCC; see the file COPYING3. If not see namespace ana { +/* An enum for identifying different spaces within memory. */ + +enum memory_space +{ + MEMSPACE_UNKNOWN, + MEMSPACE_CODE, + MEMSPACE_GLOBALS, + MEMSPACE_STACK, + MEMSPACE_HEAP, + MEMSPACE_READONLY_DATA +}; + /* An enum for discriminating between the different concrete subclasses of region. */ @@ -123,6 +135,8 @@ public: bool base_region_p () const; bool descendent_of_p (const region *elder) const; const frame_region *maybe_get_frame_region () const; + enum memory_space get_memory_space () const; + bool can_have_initial_svalue_p () const; tree maybe_get_decl () const; @@ -141,6 +155,8 @@ public: static int cmp_ptr_ptr (const void *, const void *); + bool involves_p (const svalue *sval) const; + region_offset get_offset () const; /* Attempt to get the size of this region as a concrete number of bytes. diff --git a/gcc/analyzer/sm-malloc.cc b/gcc/analyzer/sm-malloc.cc index 40e64b3..9707a68 100644 --- a/gcc/analyzer/sm-malloc.cc +++ b/gcc/analyzer/sm-malloc.cc @@ -1198,6 +1198,25 @@ public: funcname, ev.m_expr); } + /* Implementation of pending_diagnostic::supercedes_p for + use_after_free. + + We want use-after-free to supercede use-of-unitialized-value, + so that if we have these at the same stmt, we don't emit + a use-of-uninitialized, just the use-after-free. + (this is because we fully purge information about freed + buffers when we free them to avoid state explosions, so + that if they are accessed after the free, it looks like + they are uninitialized). */ + + bool supercedes_p (const pending_diagnostic &other) const FINAL OVERRIDE + { + if (other.use_of_uninit_p ()) + return true; + + return false; + } + private: diagnostic_event_id_t m_free_event; const deallocator *m_deallocator; diff --git a/gcc/analyzer/store.cc b/gcc/analyzer/store.cc index a65c741..0042a20 100644 --- a/gcc/analyzer/store.cc +++ b/gcc/analyzer/store.cc @@ -1316,6 +1316,38 @@ binding_cluster::mark_region_as_unknown (store_manager *mgr, bind (mgr, reg, sval); } +/* Purge state involving SVAL. */ + +void +binding_cluster::purge_state_involving (const svalue *sval, + region_model_manager *sval_mgr) +{ + auto_vec<const binding_key *> to_remove; + for (auto iter : m_map) + { + const binding_key *iter_key = iter.first; + if (const symbolic_binding *symbolic_key + = iter_key->dyn_cast_symbolic_binding ()) + { + const region *reg = symbolic_key->get_region (); + if (reg->involves_p (sval)) + to_remove.safe_push (iter_key); + } + const svalue *iter_sval = iter.second; + if (iter_sval->involves_p (sval)) + { + const svalue *new_sval + = sval_mgr->get_or_create_unknown_svalue (iter_sval->get_type ()); + m_map.put (iter_key, new_sval); + } + } + for (auto iter : to_remove) + { + m_map.remove (iter); + m_touched = true; + } +} + /* Get any SVAL bound to REG within this cluster via kind KIND, without checking parent regions of REG. */ @@ -2447,6 +2479,29 @@ store::mark_region_as_unknown (store_manager *mgr, const region *reg, cluster->mark_region_as_unknown (mgr, reg, uncertainty); } +/* Purge state involving SVAL. */ + +void +store::purge_state_involving (const svalue *sval, + region_model_manager *sval_mgr) +{ + auto_vec <const region *> base_regs_to_purge; + for (auto iter : m_cluster_map) + { + const region *base_reg = iter.first; + if (base_reg->involves_p (sval)) + base_regs_to_purge.safe_push (base_reg); + else + { + binding_cluster *cluster = iter.second; + cluster->purge_state_involving (sval, sval_mgr); + } + } + + for (auto iter : base_regs_to_purge) + purge_cluster (iter); +} + /* Get the cluster for BASE_REG, or NULL (const version). */ const binding_cluster * diff --git a/gcc/analyzer/store.h b/gcc/analyzer/store.h index 2ac2923..bc58694 100644 --- a/gcc/analyzer/store.h +++ b/gcc/analyzer/store.h @@ -198,6 +198,7 @@ private: class byte_range; class concrete_binding; +class symbolic_binding; /* Abstract base class for describing ranges of bits within a binding_map that can have svalues bound to them. */ @@ -220,6 +221,8 @@ public: virtual const concrete_binding *dyn_cast_concrete_binding () const { return NULL; } + virtual const symbolic_binding *dyn_cast_symbolic_binding () const + { return NULL; } }; /* A concrete range of bits. */ @@ -420,6 +423,9 @@ public: void dump_to_pp (pretty_printer *pp, bool simple) const FINAL OVERRIDE; + const symbolic_binding *dyn_cast_symbolic_binding () const FINAL OVERRIDE + { return this; } + const region *get_region () const { return m_region; } static int cmp_ptr_ptr (const void *, const void *); @@ -563,6 +569,8 @@ public: void zero_fill_region (store_manager *mgr, const region *reg); void mark_region_as_unknown (store_manager *mgr, const region *reg, uncertainty_t *uncertainty); + void purge_state_involving (const svalue *sval, + region_model_manager *sval_mgr); const svalue *get_binding (store_manager *mgr, const region *reg) const; const svalue *get_binding_recursive (store_manager *mgr, @@ -697,6 +705,8 @@ public: void zero_fill_region (store_manager *mgr, const region *reg); void mark_region_as_unknown (store_manager *mgr, const region *reg, uncertainty_t *uncertainty); + void purge_state_involving (const svalue *sval, + region_model_manager *sval_mgr); const binding_cluster *get_cluster (const region *base_reg) const; binding_cluster *get_cluster (const region *base_reg); diff --git a/gcc/analyzer/svalue.cc b/gcc/analyzer/svalue.cc index 70c23f0..22da769 100644 --- a/gcc/analyzer/svalue.cc +++ b/gcc/analyzer/svalue.cc @@ -158,6 +158,13 @@ svalue::can_merge_p (const svalue *other, || (other->get_kind () == SK_UNMERGEABLE)) return NULL; + /* Reject attempts to merge poisoned svalues with other svalues + (either non-poisoned, or other kinds of poison), so that e.g. + we identify paths in which a variable is conditionally uninitialized. */ + if (get_kind () == SK_POISONED + || other->get_kind () == SK_POISONED) + return NULL; + /* Reject attempts to merge NULL pointers with not-NULL-pointers. */ if (POINTER_TYPE_P (get_type ())) { @@ -516,6 +523,12 @@ public: m_found = true; } + void visit_conjured_svalue (const conjured_svalue *candidate) + { + if (candidate == m_needle) + m_found = true; + } + bool found_p () const { return m_found; } private: @@ -528,8 +541,9 @@ private: bool svalue::involves_p (const svalue *other) const { - /* Currently only implemented for initial_svalue. */ - gcc_assert (other->get_kind () == SK_INITIAL); + /* Currently only implemented for these kinds. */ + gcc_assert (other->get_kind () == SK_INITIAL + || other->get_kind () == SK_CONJURED); involvement_visitor v (other); accept (&v); @@ -811,6 +825,8 @@ poison_kind_to_str (enum poison_kind kind) { default: gcc_unreachable (); + case POISON_KIND_UNINIT: + return "uninit"; case POISON_KIND_FREED: return "freed"; case POISON_KIND_POPPED_STACK: @@ -847,6 +863,18 @@ poisoned_svalue::accept (visitor *v) const v->visit_poisoned_svalue (this); } +/* Implementation of svalue::maybe_fold_bits_within vfunc + for poisoned_svalue. */ + +const svalue * +poisoned_svalue::maybe_fold_bits_within (tree type, + const bit_range &, + region_model_manager *mgr) const +{ + /* Bits within a poisoned value are also poisoned. */ + return mgr->get_or_create_poisoned_svalue (m_kind, type); +} + /* class setjmp_svalue's implementation is in engine.cc, so that it can use the declaration of exploded_node. */ diff --git a/gcc/analyzer/svalue.h b/gcc/analyzer/svalue.h index 5552fcf..54b97f8 100644 --- a/gcc/analyzer/svalue.h +++ b/gcc/analyzer/svalue.h @@ -324,6 +324,9 @@ public: enum poison_kind { + /* For use to describe uninitialized memory. */ + POISON_KIND_UNINIT, + /* For use to describe freed memory. */ POISON_KIND_FREED, @@ -378,6 +381,11 @@ public: void dump_to_pp (pretty_printer *pp, bool simple) const FINAL OVERRIDE; void accept (visitor *v) const FINAL OVERRIDE; + const svalue * + maybe_fold_bits_within (tree type, + const bit_range &subrange, + region_model_manager *mgr) const FINAL OVERRIDE; + enum poison_kind get_poison_kind () const { return m_kind; } private: |