* Odd smatch issue?
@ 2019-01-11 12:32 John Levon
2019-01-14 10:18 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: John Levon @ 2019-01-11 12:32 UTC (permalink / raw)
To: smatch
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Odd smatch issue? 2019-01-11 12:32 Odd smatch issue? John Levon @ 2019-01-14 10:18 ` Dan Carpenter 2019-01-14 12:42 ` John Levon 0 siblings, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2019-01-14 10:18 UTC (permalink / raw) To: John Levon; +Cc: smatch 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: Odd smatch issue? 2019-01-14 10:18 ` Dan Carpenter @ 2019-01-14 12:42 ` John Levon 2019-01-14 14:38 ` Dan Carpenter 0 siblings, 1 reply; 6+ messages in thread From: John Levon @ 2019-01-14 12:42 UTC (permalink / raw) To: Dan Carpenter; +Cc: smatch 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Odd smatch issue? 2019-01-14 12:42 ` John Levon @ 2019-01-14 14:38 ` Dan Carpenter 2019-01-14 14:47 ` John Levon 0 siblings, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2019-01-14 14:38 UTC (permalink / raw) To: John Levon; +Cc: smatch 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: Odd smatch issue? 2019-01-14 14:38 ` Dan Carpenter @ 2019-01-14 14:47 ` John Levon 2019-01-14 14:51 ` Dan Carpenter 0 siblings, 1 reply; 6+ messages in thread From: John Levon @ 2019-01-14 14:47 UTC (permalink / raw) To: Dan Carpenter; +Cc: smatch 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Odd smatch issue? 2019-01-14 14:47 ` John Levon @ 2019-01-14 14:51 ` Dan Carpenter 0 siblings, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2019-01-14 14:51 UTC (permalink / raw) To: John Levon; +Cc: smatch 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-01-14 14:52 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-11 12:32 Odd smatch issue? John Levon 2019-01-14 10:18 ` Dan Carpenter 2019-01-14 12:42 ` John Levon 2019-01-14 14:38 ` Dan Carpenter 2019-01-14 14:47 ` John Levon 2019-01-14 14:51 ` Dan Carpenter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.