I added the smatch mailing list to the CC.
On Mon, Nov 17, 2014 at 06:13:51PM -0800, Jörn Engel wrote:
> Hello Dan!
>
> I recently came across a patch that had a serious bugfix hidden inside
> it: dbcbc95cd858
>
> inside iscsit_handle_data_out() cmd was uninitialized and
> iscsit_check_dataout_hdr() wouldn't always initialize it - so we had
> code paths using an uninitialized variable and should have seen compiler
> warnings.
>
> - Gcc versions 4.4 through 4.9 didn't warn. I tried many different
> options, none resulted in a warning.
> - clang gave no warning.
> - clang static checker gave no warning.
> - sparse gave no warning.
> - smatch gave no warning. The README file in the git tree is for
> sparse, so I may have used it wrong. But with either default
> parameters of -Wuninitialized there is no warning.
Sorry for that. I've pushed some changes so hopefully the documentation
to build and run is easier to find now. It's under Documentation/.
>
> Not a single compiler or code checker is finding this bug. That scares
> me, as we likely have a dozen more such bugs hidden inside the kernel.
>
> As a testcase, I can reduce the code down to nine lines:
>
> void callee(int *arg);
> int caller(void)
> {
> int arg;
> callee(&arg);
> if (!arg)
> return 1;
> return 0;
> }
>
> Is this something you could cook up a test for? I would like need two
> weeks to work myself through the source code before I can even get
> started.
That test case will drive you nuts. You need to have cross function
analysis for there to be a manageable number of false positives.
Smatch does cross function analysis, but playing with this I see that I
need to make some improvements here. There are still way way too many
false positives. I have some ideas here.
Anyway, just for laughs, I've attached the check I'm working on. You'll
need to add it check_list.h and recompile.
regards,
dan carpenter
/*
* Copyright (C) 2014 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_slist.h"
#include "smatch_extra.h"
static int my_id;
STATE(uninitialized);
STATE(initialized);
static void match_declarations(struct symbol *sym)
{
if (sym->initializer)
return;
/* Smatch is crap at tracking arrays */
if ((get_base_type(sym))->type == SYM_ARRAY)
return;
if (sym->ctype.modifiers & MOD_STATIC)
return;
if (!sym->ident)
return;
set_state(my_id, sym->ident->name, sym, &uninitialized);
}
static void extra_mod_hook(const char *name, struct symbol *sym, struct smatch_state *state)
{
if (!sym->ident)
return;
if (strcmp(name, sym->ident->name) != 0)
return;
set_state(my_id, name, sym, &initialized);
}
static void match_assign(struct expression *expr)
{
struct expression *right;
set_state_expr(my_id, expr->left, &initialized);
right = strip_expr(expr->right);
if (right->type == EXPR_PREOP && right->op == '&')
set_state_expr(my_id, right->unop, &initialized);
}
static int is_initialized(struct expression *expr)
{
struct sm_state *sm;
expr = strip_expr(expr);
if (expr->type != EXPR_SYMBOL)
return 1;
sm = get_sm_state_expr(my_id, expr);
if (!sm)
return 1;
if (!slist_has_state(sm->possible, &uninitialized))
return 1;
return 0;
}
static void match_dereferences(struct expression *expr)
{
char *name;
if (expr->type != EXPR_PREOP)
return;
if (is_initialized(expr->unop))
return;
name = expr_to_str(expr->unop);
sm_msg("error: XXX potentially dereferencing uninitialized '%s'.", name);
free_string(name);
set_state_expr(my_id, expr->unop, &initialized);
}
static void match_condition(struct expression *expr)
{
char *name;
if (is_initialized(expr))
return;
name = expr_to_str(expr);
sm_msg("error: XXX potentially using uninitialized '%s'.", name);
free_string(name);
set_state_expr(my_id, expr, &initialized);
}
static void match_call(struct expression *expr)
{
struct expression *arg;
char *name;
FOR_EACH_PTR(expr->args, arg) {
if (is_initialized(arg))
continue;
name = expr_to_str(arg);
sm_msg("warn: XXX passing uninitialized '%s'", name);
free_string(name);
set_state_expr(my_id, arg, &initialized);
} END_FOR_EACH_PTR(arg);
}
void check_uninitialized(int id)
{
my_id = id;
add_hook(&match_declarations, DECLARATION_HOOK);
add_extra_mod_hook(&extra_mod_hook);
add_hook(&match_assign, ASSIGNMENT_HOOK);
add_hook(&match_dereferences, DEREF_HOOK);
add_hook(&match_condition, CONDITION_HOOK);
add_hook(&match_call, FUNCTION_CALL_HOOK);
}
