| Message ID | 20211223162848.3243702-1-trix@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | mac80211: initialize variable have_higher_than_11mbit | expand |
On Fri, Dec 24, 2021 at 6:01 AM Tom Rix <trix@redhat.com> wrote: > > > On 12/23/21 12:30 PM, Nick Desaulniers wrote: > > On Thu, Dec 23, 2021 at 8:29 AM <trix@redhat.com> wrote: > >> From: Tom Rix <trix@redhat.com> > >> > >> Clang static analysis reports this warnings > >> > >> mlme.c:5332:7: warning: Branch condition evaluates to a > >> garbage value > >> have_higher_than_11mbit) > >> ^~~~~~~~~~~~~~~~~~~~~~~ > >> > >> have_higher_than_11mbit is only set to true some of the time in > >> ieee80211_get_rates() but is checked all of the time. So > >> have_higher_than_11mbit needs to be initialized to false. > > LGTM. There's only one caller of ieee80211_get_rates() today; if there > > were others, they could make a similar mistake in the future. An > > alternate approach: ieee80211_get_rates() could unconditionally write > > false before the loop that could later write true. Then call sites > > don't need to worry about this conditional assignment. Perhaps that > > would be preferable? If not: > > The have_higher_than_11mbit variable had previously be initialized to false. > > The commit 5d6a1b069b7f moved the variable without initializing. I'm not disagreeing with that. My point is that these sometimes uninitialized warnings you're finding+fixing with clang static analyzer are demonstrating a recurring pattern with code. When _not_ using the static analyzer, -Wuninitialized and -Wsometimes-uninitialized work in Clang by building a control flow graph, but they only analyze a function locally. For example, consider the following code: ``` _Bool is_thursday(void); void hello(int); void init (int* x) { if (is_thursday()) *x = 1; } void foo (void) { int x; init(&x); hello(x); } ``` (Clang+GCC today will warn on the above; x is considered to "escape" the scope of foo as init could write the address of x to a global. Instead clang's static analyzer will take the additional time to analyze the callee. But here's a spooky question: what happens when init is in another translation unit? IIRC, the static analyzer doesn't do cross TU analysis; I could be wrong though, I haven't run it in a while.) My point is that you're sending patches initializing x, when I think it might be nicer to instead have functions like init always write a value (unconditionally, rather than conditionally). That way other callers of init don't have to worry about sometimes initialized variables. > > Tom > > > > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > > >> Fixes: 5d6a1b069b7f ("mac80211: set basic rates earlier") > >> Signed-off-by: Tom Rix <trix@redhat.com> > >> --- > >> net/mac80211/mlme.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c > >> index 51f55c4ee3c6e..766cbbc9c3a72 100644 > >> --- a/net/mac80211/mlme.c > >> +++ b/net/mac80211/mlme.c > >> @@ -5279,7 +5279,7 @@ static int ieee80211_prep_connection(struct ieee80211_sub_if_data *sdata, > >> */ > >> if (new_sta) { > >> u32 rates = 0, basic_rates = 0; > >> - bool have_higher_than_11mbit; > >> + bool have_higher_than_11mbit = false; > >> int min_rate = INT_MAX, min_rate_index = -1; > >> const struct cfg80211_bss_ies *ies; > >> int shift = ieee80211_vif_get_shift(&sdata->vif); > >> -- > >> 2.26.3 > >> > > >
On 12/28/21 10:55 AM, Nick Desaulniers wrote: > On Fri, Dec 24, 2021 at 6:01 AM Tom Rix <trix@redhat.com> wrote: >> >> On 12/23/21 12:30 PM, Nick Desaulniers wrote: >>> On Thu, Dec 23, 2021 at 8:29 AM <trix@redhat.com> wrote: >>>> From: Tom Rix <trix@redhat.com> >>>> >>>> Clang static analysis reports this warnings >>>> >>>> mlme.c:5332:7: warning: Branch condition evaluates to a >>>> garbage value >>>> have_higher_than_11mbit) >>>> ^~~~~~~~~~~~~~~~~~~~~~~ >>>> >>>> have_higher_than_11mbit is only set to true some of the time in >>>> ieee80211_get_rates() but is checked all of the time. So >>>> have_higher_than_11mbit needs to be initialized to false. >>> LGTM. There's only one caller of ieee80211_get_rates() today; if there >>> were others, they could make a similar mistake in the future. An >>> alternate approach: ieee80211_get_rates() could unconditionally write >>> false before the loop that could later write true. Then call sites >>> don't need to worry about this conditional assignment. Perhaps that >>> would be preferable? If not: >> The have_higher_than_11mbit variable had previously be initialized to false. >> >> The commit 5d6a1b069b7f moved the variable without initializing. > I'm not disagreeing with that. > > My point is that these sometimes uninitialized warnings you're > finding+fixing with clang static analyzer are demonstrating a > recurring pattern with code. > > When _not_ using the static analyzer, -Wuninitialized and > -Wsometimes-uninitialized work in Clang by building a control flow > graph, but they only analyze a function locally. > > For example, consider the following code: > ``` > _Bool is_thursday(void); > void hello(int); > > void init (int* x) { > if (is_thursday()) > *x = 1; > } > > void foo (void) { > int x; > init(&x); > hello(x); > } > ``` > (Clang+GCC today will warn on the above; x is considered to "escape" > the scope of foo as init could write the address of x to a global. > Instead clang's static analyzer will take the additional time to > analyze the callee. But here's a spooky question: what happens when > init is in another translation unit? IIRC, the static analyzer doesn't > do cross TU analysis; I could be wrong though, I haven't run it in a > while.) > > My point is that you're sending patches initializing x, when I think > it might be nicer to instead have functions like init always write a > value (unconditionally, rather than conditionally). That way other > callers of init don't have to worry about sometimes initialized > variables. The variable is passed to only to the static function ieee80211_get_rates(). Tom > >> Tom >> >>> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> >>> >>>> Fixes: 5d6a1b069b7f ("mac80211: set basic rates earlier") >>>> Signed-off-by: Tom Rix <trix@redhat.com> >>>> --- >>>> net/mac80211/mlme.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c >>>> index 51f55c4ee3c6e..766cbbc9c3a72 100644 >>>> --- a/net/mac80211/mlme.c >>>> +++ b/net/mac80211/mlme.c >>>> @@ -5279,7 +5279,7 @@ static int ieee80211_prep_connection(struct ieee80211_sub_if_data *sdata, >>>> */ >>>> if (new_sta) { >>>> u32 rates = 0, basic_rates = 0; >>>> - bool have_higher_than_11mbit; >>>> + bool have_higher_than_11mbit = false; >>>> int min_rate = INT_MAX, min_rate_index = -1; >>>> const struct cfg80211_bss_ies *ies; >>>> int shift = ieee80211_vif_get_shift(&sdata->vif); >>>> -- >>>> 2.26.3 >>>> >
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c index 51f55c4ee3c6e..766cbbc9c3a72 100644 --- a/net/mac80211/mlme.c +++ b/net/mac80211/mlme.c @@ -5279,7 +5279,7 @@ static int ieee80211_prep_connection(struct ieee80211_sub_if_data *sdata, */ if (new_sta) { u32 rates = 0, basic_rates = 0; - bool have_higher_than_11mbit; + bool have_higher_than_11mbit = false; int min_rate = INT_MAX, min_rate_index = -1; const struct cfg80211_bss_ies *ies; int shift = ieee80211_vif_get_shift(&sdata->vif);