From 5fbd937aac9dd833265c40b76fb1be6421892b3f Mon Sep 17 00:00:00 2001 From: David Malcolm Date: Wed, 14 Aug 2019 12:19:28 -0400 Subject: [PATCH 078/124] FIXME: improvements to sm-taint.cc --- gcc/analyzer/region-model.cc | 2 +- gcc/analyzer/sm-taint.cc | 95 +++++++++++++++++++++++++++++---- gcc/testsuite/gcc.dg/analyzer/taint-1.c | 32 +++++++---- 3 files changed, 106 insertions(+), 23 deletions(-) diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index 13d583d..532a086 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -3866,7 +3866,7 @@ region_model::get_representative_path_var (region_id rid) const /* FIXME: it is always a COMPONENT_REF? */ if (parent_pv.m_tree) return path_var (build3 (COMPONENT_REF, - NULL_TREE, /* FIXME: type */ + TREE_TYPE (child_key), parent_pv.m_tree, child_key, NULL_TREE), parent_pv.m_stack_depth); } diff --git a/gcc/analyzer/sm-taint.cc b/gcc/analyzer/sm-taint.cc index 2c8a7bf..edd7990 100644 --- a/gcc/analyzer/sm-taint.cc +++ b/gcc/analyzer/sm-taint.cc @@ -97,12 +97,20 @@ public: //////////////////////////////////////////////////////////////////////////// +enum bounds +{ + BOUNDS_NONE, + BOUNDS_UPPER, + BOUNDS_LOWER, +}; + class tainted_array_index : public pending_diagnostic_subclass { public: - tainted_array_index (const taint_state_machine &sm, tree arg) - : m_sm (sm), m_arg (arg) {} + tainted_array_index (const taint_state_machine &sm, tree arg, + enum bounds has_bounds) + : m_sm (sm), m_arg (arg), m_has_bounds (has_bounds) {} const char *get_kind () const FINAL OVERRIDE { return "tainted_array_index"; } @@ -115,10 +123,29 @@ public: { metadata m; m.add_cwe (129); - warning_at (rich_loc, m, OPT_Wanalyzer_tainted_array_index, - "use of tainted value %qE in array lookup" - " without bounds checking", - m_arg); + switch (m_has_bounds) + { + default: + gcc_unreachable (); + case BOUNDS_NONE: + warning_at (rich_loc, m, OPT_Wanalyzer_tainted_array_index, + "use of tainted value %qE in array lookup" + " without bounds checking", + m_arg); + break; + case BOUNDS_UPPER: + warning_at (rich_loc, m, OPT_Wanalyzer_tainted_array_index, + "use of tainted value %qE in array lookup" + " without lower-bounds checking", + m_arg); + break; + case BOUNDS_LOWER: + warning_at (rich_loc, m, OPT_Wanalyzer_tainted_array_index, + "use of tainted value %qE in array lookup" + " without upper-bounds checking", + m_arg); + break; + } } label_text describe_state_change (const evdesc::state_change &change) @@ -134,19 +161,40 @@ public: return change.formatted_print ("%qE gets an unchecked value here", change.m_expr); } + else if (change.m_new_state == m_sm.m_has_lb) + return change.formatted_print ("%qE has its lower bound checked here", + change.m_expr); + else if (change.m_new_state == m_sm.m_has_ub) + return change.formatted_print ("%qE has its upper bound checked here", + change.m_expr); return label_text (); } label_text describe_final_event (const evdesc::final_event &ev) FINAL OVERRIDE { - return ev.formatted_print ("use of tainted value %qE in array lookup" - " without bounds checking", - m_arg); + switch (m_has_bounds) + { + default: + gcc_unreachable (); + case BOUNDS_NONE: + return ev.formatted_print ("use of tainted value %qE in array lookup" + " without bounds checking", + m_arg); + case BOUNDS_UPPER: + return ev.formatted_print ("use of tainted value %qE in array lookup" + " without lower-bounds checking", + m_arg); + case BOUNDS_LOWER: + return ev.formatted_print ("use of tainted value %qE in array lookup" + " without upper-bounds checking", + m_arg); + } } private: const taint_state_machine &m_sm; tree m_arg; + enum bounds m_has_bounds; }; //////////////////////////////////////////////////////////////////////////// @@ -201,9 +249,34 @@ taint_state_machine::on_stmt (sm_context *sm_ctxt, { tree arg = TREE_OPERAND (rhs1, 1); arg = sm_ctxt->get_readable_tree (arg); - sm_ctxt->warn_for_state (node, stmt, arg, m_tainted, - new tainted_array_index (*this, arg)); + + /* Unsigned types have an implicit lower bound. */ + bool is_unsigned = false; + if (INTEGRAL_TYPE_P (TREE_TYPE (arg))) + is_unsigned = TYPE_UNSIGNED (TREE_TYPE (arg)); + + /* Complain about missing bounds. */ + sm_ctxt->warn_for_state + (node, stmt, arg, m_tainted, + new tainted_array_index (*this, arg, + is_unsigned + ? BOUNDS_LOWER : BOUNDS_NONE)); sm_ctxt->on_transition (node, stmt, arg, m_tainted, m_stop); + + /* Complain about missing upper bound. */ + sm_ctxt->warn_for_state (node, stmt, arg, m_has_lb, + new tainted_array_index (*this, arg, + BOUNDS_LOWER)); + sm_ctxt->on_transition (node, stmt, arg, m_has_lb, m_stop); + + /* Complain about missing lower bound. */ + if (!is_unsigned) + { + sm_ctxt->warn_for_state (node, stmt, arg, m_has_ub, + new tainted_array_index (*this, arg, + BOUNDS_UPPER)); + sm_ctxt->on_transition (node, stmt, arg, m_has_ub, m_stop); + } } } diff --git a/gcc/testsuite/gcc.dg/analyzer/taint-1.c b/gcc/testsuite/gcc.dg/analyzer/taint-1.c index 128f094..df27e2b 100644 --- a/gcc/testsuite/gcc.dg/analyzer/taint-1.c +++ b/gcc/testsuite/gcc.dg/analyzer/taint-1.c @@ -32,7 +32,6 @@ char test_2(struct foo *f, int i) return f->buf[f->i]; } -#if 0 char test_3(FILE *f) { struct foo tmp; @@ -51,9 +50,9 @@ char test_4(FILE *f) struct foo tmp; if (1 == fread(&tmp, sizeof(tmp), 1, f)) { - if (tmp.i >= 0) { - /* BUG: has a lower bound, but not an upper bound: */ - return tmp.buf[tmp.i]; + if (tmp.i >= 0) { /* { dg-message "'tmp.i' has an unchecked value here \\(from 'tmp'\\)" } */ + /* { dg-message "'tmp.i' has its lower bound checked here" "" { target *-*-* } .-1 } */ + return tmp.buf[tmp.i]; /* { dg-warning "use of tainted value 'tmp.i' in array lookup without upper-bounds checking" } */ } } return 0; @@ -64,9 +63,9 @@ char test_5(FILE *f) struct foo tmp; if (1 == fread(&tmp, sizeof(tmp), 1, f)) { - if (tmp.i < 256) { - /* BUG: has an upper bound, but not a lower bound: */ - return tmp.buf[tmp.i]; + if (tmp.i < 256) { /* { dg-message "'tmp.i' has an unchecked value here \\(from 'tmp'\\)" } */ + /* { dg-message "'tmp.i' has its upper bound checked here" "" { target *-*-* } .-1 } */ + return tmp.buf[tmp.i]; /* { dg-warning "use of tainted value 'tmp.i' in array lookup without lower-bounds checking" } */ } } return 0; @@ -84,15 +83,24 @@ char test_6(FILE *f) struct bar tmp; if (1 == fread(&tmp, sizeof(tmp), 1, f)) { + return tmp.buf[tmp.i]; /* { dg-warning "use of tainted value 'tmp.i' in array lookup without upper-bounds checking" } */ + } + return 0; +} + +char test_7(FILE *f) +{ + struct bar tmp; + + if (1 == fread(&tmp, sizeof(tmp), 1, f)) { if (tmp.i >= 0) { - /* BUG: has an implicit lower bound, but not an upper bound: */ - return tmp.buf[tmp.i]; + return tmp.buf[tmp.i]; /* { dg-warning "use of tainted value 'tmp.i' in array lookup without upper-bounds checking" } */ } } return 0; } -char test_7(FILE *f) +char test_8(FILE *f) { struct bar tmp; @@ -105,7 +113,9 @@ char test_7(FILE *f) return 0; } -char test_8(FILE *f) +// FIXME: +#if 0 +char test_9(FILE *f) { struct foo tmp; -- 1.8.5.3