Messages in this thread | | | Subject | Re: [PATCH] mac80211: initialize variable have_higher_than_11mbit | From | Tom Rix <> | Date | Wed, 29 Dec 2021 11:26:00 -0800 |
| |
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 >>>> >
| |