analyzer: fix false positives from -Wanalyzer-tainted-divisor [PR106225]

(cherry picked from 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 <dmalcolm@redhat.com>

diff --git a/gcc/analyzer/sm-taint.cc b/gcc/analyzer/sm-taint.cc
index 17669ae768562aa02a962499b393c64364ffc272..70da585f391738c5aafcf94d991a797f1e122d19 100644 (file)
--- 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
@@ -792,18 +795,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;
        }
     }
@@ -978,6 +970,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 515f86da86a0a355671d6b8c09e0835b40b02895..09ad68b22247251314b7ab783a8141b7b5b1fe9f 100644 (file)
--- 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 7ce1c73e217aa727da9b72b1c83aea59c886abca..e3e9a8d557b6682acb5a345805175de6cef7ac27 100644 (file)
--- a/gcc/analyzer/sm.h
+++ b/gcc/analyzer/sm.h
@@ -278,6 +278,8 @@ public:
 
   const svalue *get_old_svalue (tree expr) const;
 
+  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 5a5a0b93ce09ad72cb8843aa4137d29313b1488d..b7c1faef1c478f4dbc576098589ab1d0a8143bec 100644 (file)
--- 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" } */
+}