From bb7ed7ff46604e9298cdabe48ab9189a8adc82cc Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Fri, 11 Jan 2013 10:50:33 +0300 Subject: [PATCH] *new* check_missing_break.c: check for fall through case statements This was my attempt to find missing break statements. Falling through is, of course, allowed by C but so much of the time when people do it, it is unintentional. This check tests if a value from the previous statement gets over written in the next case statement without being used. I perhaps over thought things and should just print an error for every time it falls through. Normally static checker tools let you silence the warning by putting a /* fall through */ comment on the line before. Smatch works on the pre-processed code so it can't see the comments. It is what it is. This found 7 kernel bugs and I fixed them all so now it is 100% false positives. Signed-off-by: Dan Carpenter --- check_list.h | 1 + check_missing_break.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++ smatch.h | 1 + smatch_flow.c | 1 + 4 files changed, 124 insertions(+) create mode 100644 check_missing_break.c diff --git a/check_list.h b/check_list.h index cb684256..0479c6f3 100644 --- a/check_list.h +++ b/check_list.h @@ -82,6 +82,7 @@ CK(check_sizeof_pointer) CK(check_or_vs_and) CK(check_passes_sizeof) CK(check_assign_vs_compare) +CK(check_missing_break) /* <- your test goes here */ /* CK(register_template) */ diff --git a/check_missing_break.c b/check_missing_break.c new file mode 100644 index 00000000..2a82ce48 --- /dev/null +++ b/check_missing_break.c @@ -0,0 +1,121 @@ +/* + * sparse/check_missing_break.c + * + * Copyright (C) 2013 Oracle. + * + * Licensed under the Open Software License version 1.1 + * + */ + +/* + * The way I'm detecting missing breaks is if there is an assignment inside a + * switch statement which is over written. + * + */ + +#include "smatch.h" +#include "smatch_slist.h" + +static int my_id; +static struct expression *skip_this; + +/* + * It goes like this: + * - Allocate a state which stores the switch expression. I wanted to + * just have a state &assigned but we need to know the switch statement where + * it was assigned. + * - If it gets used then we change it to &used. + * - For unmatched states we use &used (because of cleanness, not because we need + * to). + * - If we merge inside a case statement and one of the states is &assigned (or + * if it is &nobreak) then &nobreak is used. + * + * We print an error when we assign something to a &no_break symbol. + * + */ + +STATE(used); +STATE(no_break); + +static struct smatch_state *alloc_my_state(struct expression *expr) +{ + struct smatch_state *state; + char *name; + + state = __alloc_smatch_state(0); + expr = strip_expr(expr); + name = get_variable_from_expr_complex(expr, NULL); + state->name = alloc_sname(name); + free_string(name); + state->data = expr; + return state; +} + +static void match_assign(struct expression *expr) +{ + struct expression *left; + + if (expr->op != '=') + return; + if (!get_switch_expr()) + return; + left = strip_expr(expr->left); + if (get_state_expr(my_id, left) == &no_break) { + char *name; + + name = get_variable_from_expr(left, NULL); + sm_msg("warn: missing break? reassigning '%s'", name); + free_string(name); + } + + set_state_expr(my_id, left, alloc_my_state(get_switch_expr())); + skip_this = left; +} + +static void match_symbol(struct expression *expr) +{ + expr = strip_expr(expr); + if (expr == skip_this) + return; + set_state_expr(my_id, expr, &used); +} + +static struct smatch_state *unmatched_state(struct sm_state *sm) +{ + return &used; +} + +static int in_case; +struct smatch_state *merge_hook(struct smatch_state *s1, struct smatch_state *s2) +{ + struct expression *switch_expr; + + if (s1 == &no_break || s2 == &no_break) + return &no_break; + if (!in_case) + return &used; + switch_expr = get_switch_expr(); + if (s1->data == switch_expr || s2->data == switch_expr) + return &no_break; + return &used; +} + +static void match_stmt(struct statement *stmt) +{ + if (stmt->type == STMT_CASE) + in_case = 1; + else + in_case = 0; +} + +void check_missing_break(int id) +{ + my_id = id; + + add_unmatched_state_hook(my_id, &unmatched_state); + add_merge_hook(my_id, &merge_hook); + + add_hook(&match_assign, ASSIGNMENT_HOOK); + add_hook(&match_symbol, SYM_HOOK); + add_hook(&match_stmt, STMT_HOOK); +} diff --git a/smatch.h b/smatch.h index b1f6d718..3aee5522 100644 --- a/smatch.h +++ b/smatch.h @@ -304,6 +304,7 @@ int in_condition(void); void smatch (int argc, char **argv); int inside_loop(void); +struct expression *get_switch_expr(void); int in_expression_statement(void); void __split_expr(struct expression *expr); void __split_stmt(struct statement *stmt); diff --git a/smatch_flow.c b/smatch_flow.c index 66970b41..0e8b644a 100644 --- a/smatch_flow.c +++ b/smatch_flow.c @@ -37,6 +37,7 @@ int __bail_on_rest_of_function = 0; char *get_function(void) { return cur_func; } int get_lineno(void) { return __smatch_lineno; } int inside_loop(void) { return !!loop_count; } +struct expression *get_switch_expr(void) { return top_expression(switch_expr_stack); } int in_expression_statement(void) { return !!__expr_stmt_count; } static void split_symlist(struct symbol_list *sym_list); -- 2.11.4.GIT