Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
Pushed to trunk as r13-1562-g897b3b31f0a94b.
gcc/analyzer/ChangeLog:
PR analyzer/106225
* sm-taint.cc (taint_state_machine::on_stmt): Move handling of
assignments from division to...
(taint_state_machine::check_for_tainted_divisor): ...this new
function. Reject warning when the divisor is known to be non-zero.
* sm.cc: Include "analyzer/program-state.h".
(sm_context::get_old_region_model): New.
* sm.h (sm_context::get_old_region_model): New decl.
gcc/testsuite/ChangeLog:
PR analyzer/106225
* gcc.dg/analyzer/taint-divisor-1.c: Add test coverage for various
correct and incorrect checks against zero.
Signed-off-by: David Malcolm <dmalc...@redhat.com>
---
gcc/analyzer/sm-taint.cc | 51 ++++++++++----
gcc/analyzer/sm.cc | 12 ++++
gcc/analyzer/sm.h | 2 +
.../gcc.dg/analyzer/taint-divisor-1.c | 66 +++++++++++++++++++
4 files changed, 119 insertions(+), 12 deletions(-)
diff --git a/gcc/analyzer/sm-taint.cc b/gcc/analyzer/sm-taint.cc
index d2d03c3d602..4075cf6d868 100644
--- a/gcc/analyzer/sm-taint.cc
+++ b/gcc/analyzer/sm-taint.cc
@@ -109,6 +109,9 @@ private:
const supernode *node,
const gcall *call,
tree callee_fndecl) const;
+ void check_for_tainted_divisor (sm_context *sm_ctxt,
+ const supernode *node,
+ const gassign *assign) const;
public:
/* State for a "tainted" value: unsanitized data potentially under an
@@ -803,18 +806,7 @@ taint_state_machine::on_stmt (sm_context *sm_ctxt,
case ROUND_MOD_EXPR:
case RDIV_EXPR:
case EXACT_DIV_EXPR:
- {
- tree divisor = gimple_assign_rhs2 (assign);;
- state_t state = sm_ctxt->get_state (stmt, divisor);
- enum bounds b;
- if (get_taint (state, TREE_TYPE (divisor), &b))
- {
- tree diag_divisor = sm_ctxt->get_diagnostic_tree (divisor);
- sm_ctxt->warn (node, stmt, divisor,
- new tainted_divisor (*this, diag_divisor, b));
- sm_ctxt->set_next_state (stmt, divisor, m_stop);
- }
- }
+ check_for_tainted_divisor (sm_ctxt, node, assign);
break;
}
}
@@ -989,6 +981,41 @@ taint_state_machine::check_for_tainted_size_arg
(sm_context *sm_ctxt,
}
}
+/* Complain if ASSIGN (a division operation) has a tainted divisor
+ that could be zero. */
+
+void
+taint_state_machine::check_for_tainted_divisor (sm_context *sm_ctxt,
+ const supernode *node,
+ const gassign *assign) const
+{
+ const region_model *old_model = sm_ctxt->get_old_region_model ();
+ if (!old_model)
+ return;
+
+ tree divisor_expr = gimple_assign_rhs2 (assign);;
+ const svalue *divisor_sval = old_model->get_rvalue (divisor_expr, NULL);
+
+ state_t state = sm_ctxt->get_state (assign, divisor_sval);
+ enum bounds b;
+ if (get_taint (state, TREE_TYPE (divisor_expr), &b))
+ {
+ const svalue *zero_sval
+ = old_model->get_manager ()->get_or_create_int_cst
+ (TREE_TYPE (divisor_expr), 0);
+ tristate ts
+ = old_model->eval_condition (divisor_sval, NE_EXPR, zero_sval);
+ if (ts.is_true ())
+ /* The divisor is known to not equal 0: don't warn. */
+ return;
+
+ tree diag_divisor = sm_ctxt->get_diagnostic_tree (divisor_expr);
+ sm_ctxt->warn (node, assign, divisor_expr,
+ new tainted_divisor (*this, diag_divisor, b));
+ sm_ctxt->set_next_state (assign, divisor_sval, m_stop);
+ }
+}
+
} // anonymous namespace
/* Internal interface to this file. */
diff --git a/gcc/analyzer/sm.cc b/gcc/analyzer/sm.cc
index 24c20b894cd..d17d5c765b4 100644
--- a/gcc/analyzer/sm.cc
+++ b/gcc/analyzer/sm.cc
@@ -40,6 +40,7 @@ along with GCC; see the file COPYING3. If not see
#include "analyzer/program-point.h"
#include "analyzer/store.h"
#include "analyzer/svalue.h"
+#include "analyzer/program-state.h"
#if ENABLE_ANALYZER
@@ -159,6 +160,17 @@ state_machine::to_json () const
return sm_obj;
}
+/* class sm_context. */
+
+const region_model *
+sm_context::get_old_region_model () const
+{
+ if (const program_state *old_state = get_old_program_state ())
+ return old_state->m_region_model;
+ else
+ return NULL;
+}
+
/* Create instances of the various state machines, each using LOGGER,
and populate OUT with them. */
diff --git a/gcc/analyzer/sm.h b/gcc/analyzer/sm.h
index e80ef1fac37..353a6db53b0 100644
--- a/gcc/analyzer/sm.h
+++ b/gcc/analyzer/sm.h
@@ -279,6 +279,8 @@ public:
virtual const program_state *get_old_program_state () const = 0;
virtual const program_state *get_new_program_state () const = 0;
+ const region_model *get_old_region_model () const;
+
protected:
sm_context (int sm_idx, const state_machine &sm)
: m_sm_idx (sm_idx), m_sm (sm) {}
diff --git a/gcc/testsuite/gcc.dg/analyzer/taint-divisor-1.c
b/gcc/testsuite/gcc.dg/analyzer/taint-divisor-1.c
index 5a5a0b93ce0..b7c1faef1c4 100644
--- a/gcc/testsuite/gcc.dg/analyzer/taint-divisor-1.c
+++ b/gcc/testsuite/gcc.dg/analyzer/taint-divisor-1.c
@@ -24,3 +24,69 @@ int test_2 (FILE *f)
fread (&s, sizeof (s), 1, f);
return s.a % s.b; /* { dg-warning "use of attacker-controlled value 's\\.b'
as divisor without checking for zero" } */
}
+
+/* We shouldn't complain if the divisor has been checked for zero. */
+
+int test_checked_ne_zero (FILE *f)
+{
+ struct st1 s;
+ fread (&s, sizeof (s), 1, f);
+ if (s.b)
+ return s.a / s.b; /* { dg-bogus "divisor" } */
+ else
+ return 0;
+}
+
+int test_checked_gt_zero (FILE *f)
+{
+ struct st1 s;
+ fread (&s, sizeof (s), 1, f);
+ if (s.b > 0)
+ return s.a / s.b; /* { dg-bogus "divisor" } */
+ else
+ return 0;
+}
+
+int test_checked_lt_zero (FILE *f)
+{
+ struct st1 s;
+ fread (&s, sizeof (s), 1, f);
+ if (s.b < 0)
+ return s.a / s.b; /* { dg-bogus "divisor" } */
+ else
+ return 0;
+}
+
+/* We should complain if the check on the divisor still allows it to be
+ zero. */
+
+int test_checked_ge_zero (FILE *f)
+{
+ struct st1 s;
+ fread (&s, sizeof (s), 1, f);
+ if (s.b >= 0)
+ return s.a / s.b; /* { dg-warning "use of attacker-controlled value
's\\.b' as divisor without checking for zero" } */
+ else
+ return 0;
+}
+
+int test_checked_le_zero (FILE *f)
+{
+ struct st1 s;
+ fread (&s, sizeof (s), 1, f);
+ if (s.b <= 0)
+ return s.a / s.b; /* { dg-warning "use of attacker-controlled value
's\\.b' as divisor without checking for zero" } */
+ else
+ return 0;
+}
+
+int test_checked_eq_zero (FILE *f)
+{
+ struct st1 s;
+ fread (&s, sizeof (s), 1, f);
+ /* Wrong sense of test. */
+ if (s.b != 0)
+ return 0;
+ else
+ return s.a / s.b; /* { dg-warning "use of attacker-controlled value
's\\.b' as divisor without checking for zero" } */
+}
--
2.26.3
