From: Nigel Christian <nigel.l.christian@gmail.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: kernel-janitors@vger.kernel.org
Subject: Re: [PATCH] mm: hugetlb: checking for IS_ERR() instead of NULL
Date: Tue, 1 Jun 2021 17:23:46 -0400 [thread overview]
Message-ID: <YLalYt3m8jzoL4ie@fedora> (raw)
In-Reply-To: <20210601205006.GA10983@kadam>
On Tue, Jun 01, 2021 at 11:50:06PM +0300, Dan Carpenter wrote:
> 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.
Ha, you make it look easy. Let me know if I can help with testing
Should I just add below to my smatch and recompile,
or is there an experimental branch to build from?
>
> 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);
> }
>
>
next prev parent reply other threads:[~2021-06-01 21:23 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-01 9:25 [PATCH] mm: hugetlb: checking for IS_ERR() instead of NULL Dan Carpenter
2021-06-01 10:52 ` Mina Almasry
2021-06-01 17:54 ` Nigel Christian
2021-06-01 19:00 ` Dan Carpenter
2021-06-01 19:51 ` Dan Carpenter
2021-06-01 20:50 ` Dan Carpenter
2021-06-01 21:23 ` Nigel Christian [this message]
2021-06-02 6:11 ` Dan Carpenter
2021-06-02 14:47 ` Dan Carpenter
2021-06-02 16:01 ` Nigel Christian
2021-06-04 13:34 ` Dan Carpenter
2021-06-04 14:14 ` Nigel Christian
2021-06-04 14:21 ` Dan Carpenter
2021-06-02 14:22 ` Dan Carpenter
2021-06-02 15:57 ` Nigel Christian
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YLalYt3m8jzoL4ie@fedora \
--to=nigel.l.christian@gmail.com \
--cc=dan.carpenter@oracle.com \
--cc=kernel-janitors@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).