Re: [PATCH 1/6] staging: rtl8188eu: use safe iterator in rtw_free_network_queue

Dan Carpenter
2021-05-17T16:01:03Z

Thanks for catching these... I've created a new Smatch static checker
warning for this but it only works for list_for_each_entry().
Eventually someone would have run the coccinelle script to convert these
list_for_each loops into list_for_each_entry(). Otherwise you have to
parse container_of() and I've been meaning to do that for a while but I
haven't yet.

Anyway, I'm going to test it out overnight and see what it finds. It's
sort a new use for the modification_hook(), before I had only ever used
it to silence warnings but this check uses it to trigger warnings. So
perhaps it will generate a lot of false positives. We'll see.

It sets the state of the iterator to &start at the start of the loop
and if it's not &start state at the end then it prints a warning.

regards,
dan carpenter

/*
* Copyright (C) 2021 Oracle.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
* as published by the Free Software Foundation; either version 2
* of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt
*/

#include "smatch.h"
#include "smatch_extra.h"

static int my_id;

STATE(start);
STATE(watch);

static struct statement *iterator_stmt, *pre_stmt, *post_stmt;

static void set_watch(struct sm_state *sm, struct expression *mod_expr)
{
set_state(my_id, sm->name, sm->sym, &watch);
}

static bool is_list_macro(const char *macro)
{
if (strcmp(macro, "list_for_each_entry") == 0)
return true;
return false;
}

static void match_iterator_statement(struct statement *stmt)
{
const char *macro;

if (stmt->type != STMT_ITERATOR ||
!stmt->iterator_pre_statement ||
!stmt->iterator_post_statement)
return;

macro = get_macro_name(stmt->pos);
if (!macro)
return;
if (!is_list_macro(macro))
return;

iterator_stmt = stmt;
pre_stmt = stmt->iterator_pre_statement;
post_stmt = stmt->iterator_post_statement;
}

static bool stmt_matches(struct expression *expr, struct statement *stmt)
{
struct expression *tmp;
struct statement *parent;

if (!stmt)
return false;
while ((tmp = expr_get_parent_expr(expr)))
expr = tmp;

parent = expr_get_parent_stmt(expr);
return parent == stmt;
}

static char *get_iterator_member(void)
{
struct expression *expr;

if (!iterator_stmt ||
!iterator_stmt->iterator_pre_condition)
return NULL;

expr = iterator_stmt->iterator_pre_condition;
if (expr->type != EXPR_PREOP || expr->op != '!')
return NULL;
expr = strip_parens(expr->unop);
if (expr->type != EXPR_COMPARE)
return NULL;
expr = strip_parens(expr->left);
if (expr->type != EXPR_PREOP || expr->op != '&')
return NULL;
expr = strip_expr(expr->unop);
if (expr->type != EXPR_DEREF || !expr->member)
return NULL;
return expr->member->name;
}

static void match_pre_statement(struct expression *expr)
{
char *name, *member;
struct symbol *sym;
char buf[64];

if (!stmt_matches(expr, pre_stmt))
return;

name = expr_to_var_sym(expr->left, &sym);
if (!name)
return;
member = get_iterator_member();

snprintf(buf, sizeof(buf), "%s->%s.next", name, member);
set_state(my_id, buf, sym, &start);
}

static void match_post_statement(struct expression *expr)
{
struct smatch_state *state;
char *name, *member;
struct symbol *sym;
char buf[64];

if (!stmt_matches(expr, post_stmt))
return;

name = expr_to_var_sym(expr->left, &sym);
if (!name)
return;
member = get_iterator_member();

snprintf(buf, sizeof(buf), "%s->%s.next", name, member);
state = get_state(my_id, buf, sym);
if (!state || state == &start)
return;

sm_warning("iterator '%s' changed during iteration", buf);
}

void check_list_set_inside(int id)
{
my_id = id;

if (option_project != PROJ_KERNEL)
return;

add_hook(match_iterator_statement, STMT_HOOK);
add_hook(match_pre_statement, ASSIGNMENT_HOOK);
add_hook(match_post_statement, ASSIGNMENT_HOOK);

add_modification_hook(my_id, &set_watch);
}