On Tue, Jun 01, 2021 at 10:51:23PM +0300, Dan Carpenter wrote: > The other thing which might be interesting is if you pass a NULL > to IS_ERR() and then dereference the NULL then print a warning about > that. This has a lot of overlaps with some of my existing checks, but > it's still a new idea so it belongs in a separate check. It's fine and > good even if one bug triggers a lot of different warnings. I'll write > that, hang on, brb. 100% untested. :) I'll test it tonight. There are a bunch of ways to write a check like this. This test is based on copy and paste, guess work, and instinct. I normally just start writing the simplest check I can and test that, then I refine it based on whatever the common false postives are. In this code, do I need to have a modification hook? Probably not, but it was in the original code I copy and pasted and it seemed harmless. Slightly ugly perhaps? I knew from experience that I want to check if it's an explicit NULL pointer passed to IS_ERR(). There are a few ways to write that. I could have looked at the values or I could have looked at the ->possible values. I probably should have looked at the values instead... The __in_fake_assign assignment is copy and paste. I shoud probably delete that but it's harmless and a potential speed up. It was in the check_check_deref.c and I don't remember why. Probably it's essential. I'm not happy with the DEREF_HOOK api. I've been planning to re-write that but I haven't yet. 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" #include "smatch_slist.h" static int my_id; STATE(null); static void is_ok(struct sm_state *sm, struct expression *mod_expr) { set_state(my_id, sm->name, sm->sym, &undefined); } /* * The expr_has_null_exact() function means that it was explicitly * assigned NULL, not just that it is potentially NULL. */ static bool expr_has_null_exact(struct expression *expr) { struct sm_state *sm, *tmp; sval_t sval; sm = get_sm_state_expr(SMATCH_EXTRA, expr); if (!sm) return false; FOR_EACH_PTR(sm->possible, tmp) { if (!estate_get_single_value(tmp->state, &sval)) continue; if (sval.value == 0) return true; } END_FOR_EACH_PTR(tmp); return false; } static void match_is_err(const char *fn, struct expression *expr, void *unused) { struct expression *arg; arg = get_argument_from_call_expr(expr->args, 0); if (!expr_has_null_exact(arg)) return; set_state_expr(my_id, arg, &null); } static void check_dereference(struct expression *expr) { char *name; if (__in_fake_assign) return; if (get_state_expr(my_id, expr) != &null) return; if (implied_not_equal(expr, 0)) return; name = expr_to_str(expr); sm_error("potential NULL dereference '%s'", name); free_string(name); } static void match_dereferences(struct expression *expr) { if (expr->type != EXPR_PREOP) return; check_dereference(expr->unop); } void check_null_deref_after_IS_ERR(int id) { my_id = id; if (option_project != PROJ_KERNEL) return; add_function_hook("IS_ERR", &match_is_err, NULL); add_hook(&match_dereferences, DEREF_HOOK); add_modification_hook(my_id, &is_ok); }
