Hi Dan, On Wed, Apr 20, 2022 at 12:42:20PM +0300, Dan Carpenter wrote: > On Sat, Apr 16, 2022 at 05:24:34AM -0500, Rebecca Mckeever wrote: > > Add " == 0" to the condition in both else if branches to address a > > possible bug. strcmp returns 0 when its arguments are equal, which > > evaluates to false, often leading to errors when used in if statements. > > > > Currently, the statement in the first else if branch does not execute > > when its arguments are equal, but it does execute when crypt->ops->name > > equals any string other than "WEP" or "TKIP". > > > > Similarly, the second else if branch does not execute when its arguments > > are equal, and it only executes when crypt->ops->name equals "TKIP". > > The else branch never executes. > > > > It is unlikely that this is working as intended. > > > > Signed-off-by: Rebecca Mckeever <remckee0@xxxxxxxxx> > > --- > > Looks good. How did you find this bug? I noticed it when I was trying to understand the surrounding code when preparing another patch. > > Reviewed-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx> > > > There is a similiar issue in > > drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c > > but I'm not sure if it's incorrect. The strcmp on line 2847 isn't > > negated, but the ones on lines 2851, 2853, and 2855 are. > > > > 2845 /* IPW HW cannot build TKIP MIC, host decryption still needed. */ > > 2846 if (!(ieee->host_encrypt || ieee->host_decrypt) && > > 2847 strcmp(param->u.crypt.alg, "TKIP")) > > 2848 goto skip_host_crypt; > > You're right, but also I suspect this whole if statement is wrong. > > The if statement is only triggered if both ->host_encrypt and > ->host_decrypt are disabled. (Too many negatives). I think both are > set in alloc_ieee80211() and rtl8192_init_priv_variable() so both are > always true and the if statement is dead code. > > How does the code match with the comment? > > Fixing this probably requires testing. Maybe we could add this to the > TODO list or maybe add a comment? There isn't currently a TODO file for this driver. Adding a TODO file would probably be more effective than a comment? > > regards, > dan carpenter > > Ps: When you have a !(foo || bar) then it's often more readable to > write it as !foo && !bar, but in this case it doesn't really answer any > of the core questions so don't bother. Yeah, I think you're right, !foo && !bar makes more sense to me. Thanks, Rebecca
