[PATCH] staging: wilc1000: arrays can't be NULL

[PATCH] staging: wilc1000: arrays can't be NULL

[Luis de Bethencourt]

hif_drv->usr_scan_req.net.net_info[i] contains found_net_info structs
which have the following element:
u8 bssid[6];

pstrNetworkInfo, of type network_info, also contains an u8 array named
bssid.

request->ssids is an array of cfg80211_ssid structs. Making ssid:
u8 ssid[IEEE80211_MAX_SSID_LEN];

In these 3 cases the arrays are being checked against NULL, which can't
happen. Removing the checks since they will always be true.

Found with smatch:
drivers/staging/wilc1000/host_interface.c:1234 Handle_RcvdNtwrkInfo() warn: this array is probably non-NULL. 'hif_drv->usr_scan_req.net_info[i].bssid'
drivers/staging/wilc1000/host_interface.c:1235 Handle_RcvdNtwrkInfo() warn: this array is probably non-NULL. 'pstrNetworkInfo->bssid'
drivers/staging/wilc1000/host_interface.c:1253 Handle_RcvdNtwrkInfo() warn: this array is probably non-NULL. 'hif_drv->usr_scan_req.net_info[hif_drv->usr_scan_req.rcvd_ch_cnt].bssid'
drivers/staging/wilc1000/host_interface.c:1254 Handle_RcvdNtwrkInfo() warn: this array is probably non-NULL. 'pstrNetworkInfo->bssid'

Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com>
---
Hi,

I am aware this patch gives a few checkpatch.pl warnings about lines being
over 80 characters. Fixing that would be a completely different issue, and
a lengthy one since the file has loads of them.

Hopefully somebody else picks that up. Maybe I should send a hit to the
kernelnewbies mailing list :)

Thanks,
Luis


 drivers/staging/wilc1000/host_interface.c         | 38 ++++++++++-------------
 drivers/staging/wilc1000/wilc_wfi_cfgoperations.c |  3 +-
 2 files changed, 17 insertions(+), 24 deletions(-)

diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
index 9535842..7d5745a 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -1231,17 +1231,14 @@ static s32 Handle_RcvdNtwrkInfo(struct wilc_vif *vif,
 		}
 
 		for (i = 0; i < hif_drv->usr_scan_req.rcvd_ch_cnt; i++) {
-			if ((hif_drv->usr_scan_req.net_info[i].bssid) &&
-			    (pstrNetworkInfo->bssid)) {
-				if (memcmp(hif_drv->usr_scan_req.net_info[i].bssid,
-					   pstrNetworkInfo->bssid, 6) == 0) {
-					if (pstrNetworkInfo->rssi <= hif_drv->usr_scan_req.net_info[i].rssi) {
-						goto done;
-					} else {
-						hif_drv->usr_scan_req.net_info[i].rssi = pstrNetworkInfo->rssi;
-						bNewNtwrkFound = false;
-						break;
-					}
+			if (memcmp(hif_drv->usr_scan_req.net_info[i].bssid,
+				   pstrNetworkInfo->bssid, 6) == 0) {
+				if (pstrNetworkInfo->rssi <= hif_drv->usr_scan_req.net_info[i].rssi) {
+					goto done;
+				} else {
+					hif_drv->usr_scan_req.net_info[i].rssi = pstrNetworkInfo->rssi;
+					bNewNtwrkFound = false;
+					break;
 				}
 			}
 		}
@@ -1250,20 +1247,17 @@ static s32 Handle_RcvdNtwrkInfo(struct wilc_vif *vif,
 			if (hif_drv->usr_scan_req.rcvd_ch_cnt < MAX_NUM_SCANNED_NETWORKS) {
 				hif_drv->usr_scan_req.net_info[hif_drv->usr_scan_req.rcvd_ch_cnt].rssi = pstrNetworkInfo->rssi;
 
-				if (hif_drv->usr_scan_req.net_info[hif_drv->usr_scan_req.rcvd_ch_cnt].bssid &&
-				    pstrNetworkInfo->bssid) {
-					memcpy(hif_drv->usr_scan_req.net_info[hif_drv->usr_scan_req.rcvd_ch_cnt].bssid,
-					       pstrNetworkInfo->bssid, 6);
+				memcpy(hif_drv->usr_scan_req.net_info[hif_drv->usr_scan_req.rcvd_ch_cnt].bssid,
+				       pstrNetworkInfo->bssid, 6);
 
-					hif_drv->usr_scan_req.rcvd_ch_cnt++;
+				hif_drv->usr_scan_req.rcvd_ch_cnt++;
 
-					pstrNetworkInfo->new_network = true;
-					pJoinParams = host_int_ParseJoinBssParam(pstrNetworkInfo);
+				pstrNetworkInfo->new_network = true;
+				pJoinParams = host_int_ParseJoinBssParam(pstrNetworkInfo);
 
-					hif_drv->usr_scan_req.scan_result(SCAN_EVENT_NETWORK_FOUND, pstrNetworkInfo,
-									  hif_drv->usr_scan_req.arg,
-									  pJoinParams);
-				}
+				hif_drv->usr_scan_req.scan_result(SCAN_EVENT_NETWORK_FOUND, pstrNetworkInfo,
+								  hif_drv->usr_scan_req.arg,
+								  pJoinParams);
 			}
 		} else {
 			pstrNetworkInfo->new_network = false;
diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index 51aff4f..3ddfa4a 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -625,8 +625,7 @@ static int scan(struct wiphy *wiphy, struct cfg80211_scan_request *request)
 
 
 			for (i = 0; i < request->n_ssids; i++) {
-				if (request->ssids[i].ssid &&
-				    request->ssids[i].ssid_len != 0) {
+				if (request->ssids[i].ssid_len != 0) {
 					strHiddenNetwork.net_info[i].ssid = kmalloc(request->ssids[i].ssid_len, GFP_KERNEL);
 					memcpy(strHiddenNetwork.net_info[i].ssid, request->ssids[i].ssid, request->ssids[i].ssid_len);
 					strHiddenNetwork.net_info[i].ssid_len = request->ssids[i].ssid_len;
-- 
2.5.1

Re: [PATCH] staging: wilc1000: arrays can't be NULL

[Joe Perches]

On Thu, 2016-06-23 at 18:57 +0100, Luis de Bethencourt wrote:
> hif_drv->usr_scan_req.net.net_info[i] contains found_net_info structs
> which have the following element:
> u8 bssid[6];
[]
> I am aware this patch gives a few checkpatch.pl warnings about lines being
> over 80 characters. Fixing that would be a completely different issue, and
> a lengthy one since the file has loads of them.
> 
> Hopefully somebody else picks that up. Maybe I should send a hit to the
> kernelnewbies mailing list :)

Or not.

really_long_identifiers™ makes using 80 columns silly.

The hungarian could probably be converted though.

A log of the memcpy and memcpy uses could probably be
converted to ether_addr_<foo> too.

Re: [PATCH] staging: wilc1000: arrays can't be NULL

[Luis de Bethencourt]

On 23/06/16 20:24, Joe Perches wrote:
> On Thu, 2016-06-23 at 18:57 +0100, Luis de Bethencourt wrote:
>> hif_drv->usr_scan_req.net.net_info[i] contains found_net_info structs
>> which have the following element:
>> u8 bssid[6];
> []
>> I am aware this patch gives a few checkpatch.pl warnings about lines being
>> over 80 characters. Fixing that would be a completely different issue, and
>> a lengthy one since the file has loads of them.
>>
>> Hopefully somebody else picks that up. Maybe I should send a hit to the
>> kernelnewbies mailing list :)
> 
> Or not.
> 
> really_long_identifiers™ makes using 80 columns silly.

I agree. Not a priority, at all.

> 
> The hungarian could probably be converted though.
> 

I could look into this tomorrow.

I noticed, for example these 3 in the same function:
struct wid strWIDList[8];
u32 u32WidsCount = 0, dummyval = 0;
u8 *pu8CurrByte = NULL;

Not pretty and cleaning those should take little time.

> A log of the memcpy and memcpy uses could probably be
> converted to ether_addr_<foo> too.
> 

Switching memcpy for ether_addr_copy and memcmp for ether_addr_equal.

I could send a patch for this as well, but I would need to have somebody
test it for me. Or maybe get this hardware for myself and do it properly.

Do you approve of my original patch?

Thanks for the review :)
Luis

Re: [PATCH] staging: wilc1000: arrays can't be NULL

[Julian Calaby]

Hi Joe,

On Fri, Jun 24, 2016 at 5:24 AM, Joe Perches <joe@perches.com> wrote:
> On Thu, 2016-06-23 at 18:57 +0100, Luis de Bethencourt wrote:
>> hif_drv->usr_scan_req.net.net_info[i] contains found_net_info structs
>> which have the following element:
>> u8 bssid[6];
> []
>> I am aware this patch gives a few checkpatch.pl warnings about lines being
>> over 80 characters. Fixing that would be a completely different issue, and
>> a lengthy one since the file has loads of them.
>>
>> Hopefully somebody else picks that up. Maybe I should send a hit to the
>> kernelnewbies mailing list :)
>
> Or not.
>
> really_long_identifiers™ makes using 80 columns silly.
>
> The hungarian could probably be converted though.

The main developers of this driver are slowly working through the
driver's style issues, which is part of the reason why it's in
staging.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH] staging: wilc1000: arrays can't be NULL

[Luis de Bethencourt]

On 24/06/16 00:15, Julian Calaby wrote:
> Hi Joe,
> 
> On Fri, Jun 24, 2016 at 5:24 AM, Joe Perches <joe@perches.com> wrote:
>> On Thu, 2016-06-23 at 18:57 +0100, Luis de Bethencourt wrote:
>>> hif_drv->usr_scan_req.net.net_info[i] contains found_net_info structs
>>> which have the following element:
>>> u8 bssid[6];
>> []
>>> I am aware this patch gives a few checkpatch.pl warnings about lines being
>>> over 80 characters. Fixing that would be a completely different issue, and
>>> a lengthy one since the file has loads of them.
>>>
>>> Hopefully somebody else picks that up. Maybe I should send a hit to the
>>> kernelnewbies mailing list :)
>>
>> Or not.
>>
>> really_long_identifiers™ makes using 80 columns silly.
>>
>> The hungarian could probably be converted though.
> 
> The main developers of this driver are slowly working through the
> driver's style issues, which is part of the reason why it's in
> staging.
> 
> Thanks,
> 

I understand Julian,

All the maintainers listed in the MAINTAINERS file are in CC. I will wait for
them to OK the suggestion of sending a patch fixing the Hungarian Notation.

Didn't mean to step on your toes. I just wanted to help.

Code in staging is cared for by a lot of people :)

Luis

Re: [PATCH] staging: wilc1000: arrays can't be NULL

[Julian Calaby]

Hi Luis,

On Fri, Jun 24, 2016 at 9:50 AM, Luis de Bethencourt
<luisbg@osg.samsung.com> wrote:
> On 24/06/16 00:15, Julian Calaby wrote:
>> Hi Joe,
>>
>> On Fri, Jun 24, 2016 at 5:24 AM, Joe Perches <joe@perches.com> wrote:
>>> On Thu, 2016-06-23 at 18:57 +0100, Luis de Bethencourt wrote:
>>>> hif_drv->usr_scan_req.net.net_info[i] contains found_net_info structs
>>>> which have the following element:
>>>> u8 bssid[6];
>>> []
>>>> I am aware this patch gives a few checkpatch.pl warnings about lines being
>>>> over 80 characters. Fixing that would be a completely different issue, and
>>>> a lengthy one since the file has loads of them.
>>>>
>>>> Hopefully somebody else picks that up. Maybe I should send a hit to the
>>>> kernelnewbies mailing list :)
>>>
>>> Or not.
>>>
>>> really_long_identifiers™ makes using 80 columns silly.
>>>
>>> The hungarian could probably be converted though.
>>
>> The main developers of this driver are slowly working through the
>> driver's style issues, which is part of the reason why it's in
>> staging.
>>
>> Thanks,
>>
>
> I understand Julian,
>
> All the maintainers listed in the MAINTAINERS file are in CC. I will wait for
> them to OK the suggestion of sending a patch fixing the Hungarian Notation.

I was letting you know that this work is going to happen, not
dissuading you from doing it.

> Didn't mean to step on your toes. I just wanted to help.

No toes were stepped on. As I said, this was not a "don't do that"
message, this was an "it's going to happen eventually" message.

> Code in staging is cared for by a lot of people :)

Indeed it is.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH] staging: wilc1000: arrays can't be NULL

[Luis de Bethencourt]

On 24/06/16 00:54, Julian Calaby wrote:
> Hi Luis,
> 
> On Fri, Jun 24, 2016 at 9:50 AM, Luis de Bethencourt
> <luisbg@osg.samsung.com> wrote:
>> On 24/06/16 00:15, Julian Calaby wrote:
>>> Hi Joe,
>>>
>>> On Fri, Jun 24, 2016 at 5:24 AM, Joe Perches <joe@perches.com> wrote:
>>>> On Thu, 2016-06-23 at 18:57 +0100, Luis de Bethencourt wrote:
>>>>> hif_drv->usr_scan_req.net.net_info[i] contains found_net_info structs
>>>>> which have the following element:
>>>>> u8 bssid[6];
>>>> []
>>>>> I am aware this patch gives a few checkpatch.pl warnings about lines being
>>>>> over 80 characters. Fixing that would be a completely different issue, and
>>>>> a lengthy one since the file has loads of them.
>>>>>
>>>>> Hopefully somebody else picks that up. Maybe I should send a hit to the
>>>>> kernelnewbies mailing list :)
>>>>
>>>> Or not.
>>>>
>>>> really_long_identifiers™ makes using 80 columns silly.
>>>>
>>>> The hungarian could probably be converted though.
>>>
>>> The main developers of this driver are slowly working through the
>>> driver's style issues, which is part of the reason why it's in
>>> staging.
>>>
>>> Thanks,
>>>
>>
>> I understand Julian,
>>
>> All the maintainers listed in the MAINTAINERS file are in CC. I will wait for
>> them to OK the suggestion of sending a patch fixing the Hungarian Notation.
> 
> I was letting you know that this work is going to happen, not
> dissuading you from doing it.
> 
>> Didn't mean to step on your toes. I just wanted to help.
> 
> No toes were stepped on. As I said, this was not a "don't do that"
> message, this was an "it's going to happen eventually" message.
> 
>> Code in staging is cared for by a lot of people :)
> 
> Indeed it is.
> 
> Thanks,
> 

Gotcha.

I will send the Hungarian Notation change tomorrow. Since it is some small help.

I will let the memcpy/memcp to ether_addr_* change for the maintainers. I believe
it will happen soon.

Thanks for your input.

Luis