From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 2209) id 4ABA5385AE7F; Wed, 27 Jul 2022 21:55:59 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 4ABA5385AE7F MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="utf-8" From: David Malcolm To: gcc-cvs@gcc.gnu.org Subject: [gcc r12-8638] analyzer: fix false positives from -Wanalyzer-tainted-divisor [PR106225] X-Act-Checkin: gcc X-Git-Author: David Malcolm X-Git-Refname: refs/heads/releases/gcc-12 X-Git-Oldrev: 09cb9c88ef8e2c0c89ada9cde2caf1a960db7a77 X-Git-Newrev: 71a4f739c218746df70612eeb844024d1fe206bb Message-Id: <20220727215559.4ABA5385AE7F@sourceware.org> Date: Wed, 27 Jul 2022 21:55:59 +0000 (GMT) X-BeenThere: gcc-cvs@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-cvs mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 27 Jul 2022 21:55:59 -0000 https://gcc.gnu.org/g:71a4f739c218746df70612eeb844024d1fe206bb commit r12-8638-g71a4f739c218746df70612eeb844024d1fe206bb Author: David Malcolm Date: Wed Jul 27 17:38:55 2022 -0400 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 Diff: --- gcc/analyzer/sm-taint.cc | 51 ++++++++++++++----- gcc/analyzer/sm.cc | 12 +++++ gcc/analyzer/sm.h | 2 + gcc/testsuite/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 17669ae7685..70da585f391 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 @@ -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 515f86da86a..09ad68b2224 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 7ce1c73e217..e3e9a8d557b 100644 --- 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 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" } */ +}