Re: [PATCH] mm: hugetlb: checking for IS_ERR() instead of NULL

[Dan Carpenter <dan.carpenter@oracle.com>]

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);
}