staging: rtl8192u: compare strcmp result to zero
PATCH 1/1 staging: rtl8192u: compare strcmp result to zero
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.
---
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;
2849
2850 //set WEP40 first, it will be modified according to WEP104 or WEP40 at other place
2851 if (!strcmp(param->u.crypt.alg, "WEP"))
2852 module = "ieee80211_crypt_wep";
2853 else if (!strcmp(param->u.crypt.alg, "TKIP"))
2854 module = "ieee80211_crypt_tkip";
2855 else if (!strcmp(param->u.crypt.alg, "CCMP"))
2856 module = "ieee80211_crypt_ccmp";
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c +2 -2
@@ -688,9 +688,9 @@ int ieee80211_wx_get_encode_ext(struct ieee80211_device *ieee, }else {if (strcmp (crypt->ops->name,"WEP" ) ==0 ) ext->alg =IW_ENCODE_ALG_WEP ; -else if (strcmp (crypt->ops->name,"TKIP" )) +else if (strcmp (crypt->ops->name,"TKIP" ) ==0 ) ext->alg =IW_ENCODE_ALG_TKIP ; -else if (strcmp (crypt->ops->name,"CCMP" )) +else if (strcmp (crypt->ops->name,"CCMP" ) ==0 ) ext->alg =IW_ENCODE_ALG_CCMP ;else return -EINVAL ;
Signature · 1 line
2.32.0
Quoted 8 lines · Rebecca Mckeever
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->nameequals any string other than "WEP" or "TKIP".
Quoted 8 lines
Similarly, the second else if branch does not execute when its arguments are equal, and it only executes when
crypt->ops->nameequals "TKIP". The else branch never executes.It is unlikely that this is working as intended.
Signed-off-by Rebecca Mckeever <[email protected]>---
Looks good. How did you find this bug?
Quoted 9 lines
There is a similiar issue in
drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.cbut 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) &&
2847strcmp(param->u.crypt.alg, "TKIP"))
2848 gotoskip_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?
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.
Hi Dan,
Quoted 13 lines · Dan Carpenter
Quoted 12 lines · Rebecca Mckeever
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->nameequals 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->nameequals "TKIP". The else branch never executes.
Quoted 6 lines
Quoted 4 lines
It is unlikely that this is working as intended.
Signed-off-by Rebecca Mckeever <[email protected]>---
Looks good. How did you find this bug?
I noticed it when I was trying to understand the surrounding code when preparing another patch.
Quoted 20 lines
Reviewed-by Dan Carpenter <[email protected]>Quoted 9 lines
There is a similiar issue in
drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.cbut 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) &&
2847strcmp(param->u.crypt.alg, "TKIP"))
2848 gotoskip_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()andrtl8192_init_priv_variable()so both are always true and the if statement is dead code.
Quoted 4 lines
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?
Quoted 7 lines
regards,
dan carpenterPs: 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