Odd smatch issue?
static longlx_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
Quoted 4 lines · John Levon
static long
lx_cap_update_priv(void)
Quoted 8 lines
{
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.
--- smatch_estate.c | 4 +++- smatch_extra.c | 27 ++------------------------- 2 files changed, 5 insertions(+), 26 deletions(-)
smatch_estate.c +3 -1
@@ -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)));
smatch_extra.c +2 -25
@@ -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 ;
Signature · 1 line
2.17.1
Quoted 11 lines · Dan Carpenter
Quoted 10 lines
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]; }
Quoted 5 lines
Thanks John,
I am testing this patch:
[PATCH] extra: preserve
hard_maxafter 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
Try this patch?
regards,
dan carpenter
From 04a5ff12178fe7f6cab1eb0e2d370d8dfb17ea2b Mon Sep 17 00:00:00 2001
From: Dan Carpenter <[email protected]>
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.
--- smatch_estate.c | 19 +++++++++++++++++++ smatch_extra.h | 1 + smatch_implied.c | 4 ++-- 3 files changed, 22 insertions(+), 2 deletions(-)
smatch_estate.c +19 -0
@@ -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;
smatch_extra.h +1 -0
@@ -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);
smatch_implied.c +2 -2
@@ -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);
Signature · 1 line
2.17.1
Quoted 1 line · Dan Carpenter
Try this patch?
With both applied, smatch now catches the bad code, thanks!
Time to run it across everything...
regards
john
Quoted 7 lines · John Levon
Quoted 1 line · Dan Carpenter
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