Re: Found some errors and other oddities, largely by means of a static code analysis program

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Many of my other comments apply.

> diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> index 23d539d..1d4475d 100644
> --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> @@ -170,7 +170,7 @@ static inline char *translate_scan(struct _adapter *padapter,
>  	s8 *p;
>  	u32 i = 0, ht_ielen = 0;
>  	u16	cap, ht_cap = false, mcs_rate;
> -	u8	rssi, bw_40MHz = 0, short_GI = 0;
> +	u8	rssi;
>  
>  	if ((pnetwork->network.Configuration.DSConfig < 1) ||
>  	    (pnetwork->network.Configuration.DSConfig > 14)) {
> @@ -197,10 +197,6 @@ static inline char *translate_scan(struct _adapter *padapter,
>  		ht_cap = true;
>  		pht_capie = (struct ieee80211_ht_cap *)(p + 2);
>  		memcpy(&mcs_rate , pht_capie->supp_mcs_set, 2);
> -		bw_40MHz = (pht_capie->cap_info&IEEE80211_HT_CAP_SUP_WIDTH)
> -			   ? 1 : 0;
> -		short_GI = (pht_capie->cap_info&(IEEE80211_HT_CAP_SGI_20 |
> -			    IEEE80211_HT_CAP_SGI_40)) ? 1 : 0;
>  	}
>  	/* Add the protocol name */
>  	iwe.cmd = SIOCGIWNAME;

Good.

> @@ -286,8 +282,8 @@ static inline char *translate_scan(struct _adapter *padapter,
>  		u8 wpa_ie[255], rsn_ie[255];
>  		u16 wpa_len = 0, rsn_len = 0;
>  		int n;
> -		sint out_len = 0;
> -		out_len = r8712_get_sec_ie(pnetwork->network.IEs,
> +
> +		r8712_get_sec_ie(pnetwork->network.IEs,
>  					   pnetwork->network.
>  					   IELength, rsn_ie, &rsn_len,
>  					   wpa_ie, &wpa_len);

Not sure.  Presumably good.

> @@ -1133,13 +1129,12 @@ static int r871x_wx_set_mlme(struct net_device *dev,
>  			     union iwreq_data *wrqu, char *extra)
>  {
>  	int ret = 0;
> -	u16 reason;
>  	struct _adapter *padapter = (struct _adapter *)netdev_priv(dev);
>  	struct iw_mlme *mlme = (struct iw_mlme *) extra;
>  
>  	if (mlme == NULL)
>  		return -1;
> -	reason = cpu_to_le16(mlme->reason_code);
> +	cpu_to_le16(mlme->reason_code);

Delete the cpu_to_le16() as well.

>  	switch (mlme->cmd) {
>  	case IW_MLME_DEAUTH:
>  		if (!r8712_set_802_11_disassociate(padapter))
> @@ -1465,10 +1460,7 @@ static int r8711_wx_get_rate(struct net_device *dev,
>  				RTL8712_RF_2T2R == rf_type)
>  				max_rate = (bw_40MHz) ? ((short_GI) ? 300 :
>  					    270) : ((short_GI) ? 144 : 130);
> -			else if (mcs_rate & 0x0080) /* MCS7 */
> -				max_rate = (bw_40MHz) ? ((short_GI) ? 150 :
> -					    135) : ((short_GI) ? 72 : 65);
> -			else /* default MCS7 */
> +			else /* default MCS7  and  MCS7 */
>  				max_rate = (bw_40MHz) ? ((short_GI) ? 150 :
>  					    135) : ((short_GI) ? 72 : 65);

This is probably duplicated because of a bug.  Hopefully one of the
maintainers can comment.

>  			max_rate *= 2; /* Mbps/2 */
> @@ -1822,6 +1814,7 @@ static int r871x_wx_set_enc_ext(struct net_device *dev,
>  		alg_name = "CCMP";
>  		break;
>  	default:
> +		kfree(param);
>  		return -EINVAL;

Good.  But this belongs in a separate patch.

>  	}
>  	strncpy((char *)param->u.crypt.alg, alg_name, IEEE_CRYPT_ALG_NAME_LEN);
> diff --git a/drivers/staging/rtl8712/rtl871x_mlme.c b/drivers/staging/rtl8712/rtl871x_mlme.c
> index 3ea99ae..f126763 100644
> --- a/drivers/staging/rtl8712/rtl871x_mlme.c
> +++ b/drivers/staging/rtl8712/rtl871x_mlme.c
> @@ -1274,22 +1274,30 @@ sint r8712_set_key(struct _adapter *adapter,
>  			psecuritypriv->DefKey[keyid].skey, keylen);
>  		break;
>  	case _TKIP_:
> -		if (keyid < 1 || keyid > 2)
> +		if (keyid < 1 || keyid > 2) {
> +			kfree((unsigned char *)pcmd);
> +			kfree((unsigned char *)psetkeyparm);
>  			return _FAIL;

The cast is wrong and anyway it's not needed.  This should be:

			ret = _FAIL;
			goto err_free_keyparm;

> +		}
>  		keylen = 16;
>  		memcpy(psetkeyparm->key,
>  			&psecuritypriv->XGrpKey[keyid - 1], keylen);
>  		psetkeyparm->grpkey = 1;
>  		break;
>  	case _AES_:
> -		if (keyid < 1 || keyid > 2)
> +        if (keyid < 1 || keyid > 2) {
> +			kfree((unsigned char *)pcmd);
> +			kfree((unsigned char *)psetkeyparm);
>  			return _FAIL;

			ret = _FAIL;
			goto err_free_keyparm;

> +		}
>  		keylen = 16;
>  		memcpy(psetkeyparm->key,
>  			&psecuritypriv->XGrpKey[keyid - 1], keylen);
>  		psetkeyparm->grpkey = 1;
>  		break;
>  	default:
> +		kfree((unsigned char *)pcmd);
> +		kfree((unsigned char *)psetkeyparm);
>  		return _FAIL;

		ret = _FAIL;
		goto err_free_keyparm;

>  	}
>  	pcmd->cmdcode = _SetKey_CMD_;

Then at the end of the function:

	return _SUCCESS;

err_free_keyparm;
	kfree(psetkeyparm);
err_free_pcmd:
	kfree(pcmd);

	return _FAIL;
}

The reset of the patch seems valid but it needs to be broken up into
patches which do "one thing per patch" and the patch descriptions need
to be fixed.

regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux