Odd smatch issue?

Odd smatch issue?

[John Levon]

static long
lx_cap_update_priv(void)
{
        const int lx_cap_mapping[4] = { 0, 0, 0 };
        int i = 63;
        /* enabling the below line disables the warning */
        //int cap_set = i == 0;
        lx_cap_mapping[i];
}


/home/gk/src/smatch/smatch: a.c:8 lx_cap_update_priv() error: buffer overflow 'lx_cap_mapping' 4 <= 63

Distilled down from some real code, obviously, but it seems odd that
just *looking* at "i" means smatch can't figure out its max value?

thanks
john

Re: Odd smatch issue?

[Dan Carpenter]

On Fri, Jan 11, 2019 at 12:32:41PM +0000, John Levon wrote:
> 
> 
> static long
> lx_cap_update_priv(void)
> {
>         const int lx_cap_mapping[4] = { 0, 0, 0 };
>         int i = 63;
>         /* enabling the below line disables the warning */
>         //int cap_set = i == 0;
>         lx_cap_mapping[i];
> }
> 

Thanks John,

I am testing this patch:

[PATCH] extra: preserve hard_max after comparisons to zero

John Levon reported that if you had code like:

int lx_cap_mapping[4];
int i = 63;

if (i)
	;
lx_cap_mapping[i] = 0;

The code should trigger a warning but it doesn't.

I initially thought the hard_max was getting lost in
merge_estates().  I changed that to say if we were merging an
impossible state with a possible one then we should keep the
hard max from the possible one.  It turns out that wasn't
the issue, but I sort of think it's the correct thing to do
anyway.

The real problem is that we don't preserve the hard_max after
comparisons to zero, whether it's impossible or not.  That is
handled in match_condition().  I changed that function to re-use
handle_comparison().  After that because the original "i" had a
hard_max then handle_comparison() sets the hard_max flag for
both the possible and the impossible sides.

Reported-by: John Levon <levon@movementarian.org>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 smatch_estate.c |  4 +++-
 smatch_extra.c  | 27 ++-------------------------
 2 files changed, 5 insertions(+), 26 deletions(-)

diff --git a/smatch_estate.c b/smatch_estate.c
index 61a8fd34..69b84d91 100644
--- a/smatch_estate.c
+++ b/smatch_estate.c
@@ -43,7 +43,9 @@ struct smatch_state *merge_estates(struct smatch_state *s1, struct smatch_state
 	tmp = alloc_estate_rl(value_ranges);
 	rlist = get_shared_relations(estate_related(s1), estate_related(s2));
 	set_related(tmp, rlist);
-	if (estate_has_hard_max(s1) && estate_has_hard_max(s2))
+
+	if ((estate_has_hard_max(s1) && (!estate_rl(s2) || estate_has_hard_max(s2))) ||
+	    (estate_has_hard_max(s2) && (!estate_rl(s1) || estate_has_hard_max(s1))))
 		estate_set_hard_max(tmp);
 
 	estate_set_fuzzy_max(tmp, sval_max(estate_get_fuzzy_max(s1), estate_get_fuzzy_max(s2)));
diff --git a/smatch_extra.c b/smatch_extra.c
index c2d97a88..e781c80f 100644
--- a/smatch_extra.c
+++ b/smatch_extra.c
@@ -1989,11 +1989,6 @@ static void handle_MOD_condition(struct expression *expr)
 /* this is actually hooked from smatch_implied.c...  it's hacky, yes */
 void __extra_match_condition(struct expression *expr)
 {
-	struct smatch_state *pre_state;
-	struct smatch_state *true_state;
-	struct smatch_state *false_state;
-	struct range_list *pre_rl;
-
 	expr = strip_expr(expr);
 	switch (expr->type) {
 	case EXPR_CALL:
@@ -2001,27 +1996,9 @@ void __extra_match_condition(struct expression *expr)
 		return;
 	case EXPR_PREOP:
 	case EXPR_SYMBOL:
-	case EXPR_DEREF: {
-		sval_t zero;
-
-		zero = sval_blank(expr);
-		zero.value = 0;
-
-		pre_state = get_extra_state(expr);
-		if (estate_is_empty(pre_state))
-			return;
-		if (pre_state)
-			pre_rl = estate_rl(pre_state);
-		else
-			get_absolute_rl(expr, &pre_rl);
-		if (possibly_true_rl(pre_rl, SPECIAL_EQUAL, rl_zero()))
-			false_state = alloc_estate_sval(zero);
-		else
-			false_state = alloc_estate_empty();
-		true_state = alloc_estate_rl(remove_range(pre_rl, zero, zero));
-		set_extra_expr_true_false(expr, true_state, false_state);
+	case EXPR_DEREF:
+		handle_comparison(get_type(expr), expr, SPECIAL_NOTEQUAL, zero_expr());
 		return;
-	}
 	case EXPR_COMPARE:
 		match_comparison(expr);
 		return;
-- 
2.17.1

Re: Odd smatch issue?

[John Levon]

On Mon, Jan 14, 2019 at 01:18:04PM +0300, Dan Carpenter wrote:

> > static long
> > lx_cap_update_priv(void)
> > {
> >         const int lx_cap_mapping[4] = { 0, 0, 0 };
> >         int i = 63;
> >         /* enabling the below line disables the warning */
> >         //int cap_set = i == 0;
> >         lx_cap_mapping[i];
> > }
> > 
> 
> Thanks John,
> 
> I am testing this patch:
> 
> [PATCH] extra: preserve hard_max after comparisons to zero

Thanks for taking a look. Unfortunately this still doesn't seem to cover
the original case which the above was boiled down from. This does:

 76 #define LX_CAP_CAPISSET(id, cap) \                                               
 77         (((id < 32) && (((0x1 << id) & cap[0]) != 0)) || \                       
 78         ((id >= 32) && (((0x1 << (id - 32) & cap[1]) != 0))))                    


221         for (i = 0; i <= LX_CAP_MAX_CHECK; i++) {                                
222                 cap_set = LX_CAP_CAPISSET(i, cap);                               
223                 if (lx_cap_mapping[i] == NULL || i > LX_CAP_MAX_VALID) {         

(The code is bug is on line :223 where we need to reverse the order of the checks.)

regards
john

Re: Odd smatch issue?

[Dan Carpenter]

Try this patch?

regards,
dan carpenter

From 04a5ff12178fe7f6cab1eb0e2d370d8dfb17ea2b Mon Sep 17 00:00:00 2001
From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Mon, 14 Jan 2019 17:35:31 +0300
Subject: [PATCH] implied: Preserve ->hard_max for fake history states

In smatch_implied.c we sometimes split an estate (extra state)
into two parts to create a fake history.  This happens when we
have an if statement like "if (x < 10) {" and we hadn't
previously known that x=0-9 was an "interesting" range before.
Maybe we had only known that x could be in the 0-20 range.

So instead of one state, we split it into two states.  But
unfortunately the ->hard_max information was not preserved so
we missed out on some array overflow warnings.

Reported-by: John Levon <levon@movementarian.org>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 smatch_estate.c  | 19 +++++++++++++++++++
 smatch_extra.h   |  1 +
 smatch_implied.c |  4 ++--
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/smatch_estate.c b/smatch_estate.c
index 69b84d91..6c3075a4 100644
--- a/smatch_estate.c
+++ b/smatch_estate.c
@@ -274,6 +274,25 @@ struct smatch_state *clone_estate(struct smatch_state *state)
 	return ret;
 }
 
+struct smatch_state *clone_partial_estate(struct smatch_state *state, struct range_list *rl)
+{
+	struct smatch_state *ret;
+
+	if (!state)
+		return NULL;
+
+	rl = cast_rl(estate_type(state), rl);
+
+	ret = alloc_estate_rl(rl);
+	set_related(ret, clone_related_list(estate_related(state)));
+	if (estate_has_hard_max(state))
+		estate_set_hard_max(ret);
+	if (estate_has_fuzzy_max(state))
+		estate_set_fuzzy_max(ret, estate_get_fuzzy_max(state));
+
+	return ret;
+}
+
 struct smatch_state *alloc_estate_empty(void)
 {
 	struct smatch_state *state;
diff --git a/smatch_extra.h b/smatch_extra.h
index d48cdf1f..e8b350f6 100644
--- a/smatch_extra.h
+++ b/smatch_extra.h
@@ -117,6 +117,7 @@ struct smatch_state *alloc_estate_rl(struct range_list *rl);
 struct smatch_state *alloc_estate_whole(struct symbol *type);
 struct smatch_state *clone_estate(struct smatch_state *state);
 struct smatch_state *clone_estate_cast(struct symbol *type, struct smatch_state *state);
+struct smatch_state *clone_partial_estate(struct smatch_state *state, struct range_list *rl);
 
 struct smatch_state *merge_estates(struct smatch_state *s1, struct smatch_state *s2);
 
diff --git a/smatch_implied.c b/smatch_implied.c
index 9e36a24c..df00634d 100644
--- a/smatch_implied.c
+++ b/smatch_implied.c
@@ -149,10 +149,10 @@ static int create_fake_history(struct sm_state *sm, int comparison, struct range
 	true_sm = clone_sm(sm);
 	false_sm = clone_sm(sm);
 
-	true_sm->state = alloc_estate_rl(cast_rl(estate_type(sm->state), true_rl));
+	true_sm->state = clone_partial_estate(sm->state, true_rl);
 	free_slist(&true_sm->possible);
 	add_possible_sm(true_sm, true_sm);
-	false_sm->state = alloc_estate_rl(cast_rl(estate_type(sm->state), false_rl));
+	false_sm->state = clone_partial_estate(sm->state, false_rl);
 	free_slist(&false_sm->possible);
 	add_possible_sm(false_sm, false_sm);
 
-- 
2.17.1

Re: Odd smatch issue?

[John Levon]

On Mon, Jan 14, 2019 at 05:38:00PM +0300, Dan Carpenter wrote:

> Try this patch?

With both applied, smatch now catches the bad code, thanks!

Time to run it across everything...

regards
john

Re: Odd smatch issue?

[Dan Carpenter]

On Mon, Jan 14, 2019 at 02:47:36PM +0000, John Levon wrote:
> On Mon, Jan 14, 2019 at 05:38:00PM +0300, Dan Carpenter wrote:
> 
> > Try this patch?
> 
> With both applied, smatch now catches the bad code, thanks!
> 
> Time to run it across everything...

Very lightly tested, btw.  Especially patch 2 might have subtle side
effects.  Probably good ones.  Or maybe no side effects...

regards,
dan carpenter