[PATCH] Staging, rtl8192e, softmac: remove redundant memset and fix mem leak

[PATCH] Staging, rtl8192e, softmac: remove redundant memset and fix mem leak

[Jesper Juhl]

In drivers/staging/rtl8192e/rtllib_softmac.c::rtllib_rx_assoc_resp()
we allocate memory for 'network' with kzalloc() and then proceed to
zero the already zeroed mem we got from kzalloc() with
memset(). That's redundant, so remove the memset()

We also fail to kfree() the memory we allocated for 'network' if we do not enter

  if (ieee->current_network.qos_data.supported == 1) {

and the variable then goes out of scope.

To fix that I simply moved the kfree() that was inside that 'if'
statement to instead be just after it. It then covers both the case
where we take the branch and when we don't.

Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
 drivers/staging/rtl8192e/rtllib_softmac.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

  Compile tested only.

diff --git a/drivers/staging/rtl8192e/rtllib_softmac.c b/drivers/staging/rtl8192e/rtllib_softmac.c
index 1637f11..c5a15db 100644
--- a/drivers/staging/rtl8192e/rtllib_softmac.c
+++ b/drivers/staging/rtl8192e/rtllib_softmac.c
@@ -2234,7 +2234,6 @@ inline int rtllib_rx_assoc_resp(struct rtllib_device *ieee, struct sk_buff *skb,
 
 			if (!network)
 				return 1;
-			memset(network, 0, sizeof(*network));
 			ieee->state = RTLLIB_LINKED;
 			ieee->assoc_id = aid;
 			ieee->softmac_stats.rx_ass_ok++;
@@ -2259,8 +2258,8 @@ inline int rtllib_rx_assoc_resp(struct rtllib_device *ieee, struct sk_buff *skb,
 					ieee->handle_assoc_response(ieee->dev,
 						 (struct rtllib_assoc_response_frame *)header,
 						 network);
-				kfree(network);
 			}
+			kfree(network);
 
 			kfree(ieee->assocresp_ies);
 			ieee->assocresp_ies = NULL;
-- 
1.7.9


-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH] Staging, rtl8192e, softmac: remove redundant memset and fix mem leak

[Larry Finger]

On 02/12/2012 05:15 PM, Jesper Juhl wrote:
> In drivers/staging/rtl8192e/rtllib_softmac.c::rtllib_rx_assoc_resp()
> we allocate memory for 'network' with kzalloc() and then proceed to
> zero the already zeroed mem we got from kzalloc() with
> memset(). That's redundant, so remove the memset()
>
> We also fail to kfree() the memory we allocated for 'network' if we do not enter
>
>    if (ieee->current_network.qos_data.supported == 1) {
>
> and the variable then goes out of scope.
>
> To fix that I simply moved the kfree() that was inside that 'if'
> statement to instead be just after it. It then covers both the case
> where we take the branch and when we don't.
>
> Signed-off-by: Jesper Juhl<jj@chaosbits.net>
> ---
>   drivers/staging/rtl8192e/rtllib_softmac.c |    3 +--
>   1 files changed, 1 insertions(+), 2 deletions(-)
>
>    Compile tested only.

ACKed-by: Larry Finger <Larry.Finger@lwfinger.net>

Thanks,

Larry

>
> diff --git a/drivers/staging/rtl8192e/rtllib_softmac.c b/drivers/staging/rtl8192e/rtllib_softmac.c
> index 1637f11..c5a15db 100644
> --- a/drivers/staging/rtl8192e/rtllib_softmac.c
> +++ b/drivers/staging/rtl8192e/rtllib_softmac.c
> @@ -2234,7 +2234,6 @@ inline int rtllib_rx_assoc_resp(struct rtllib_device *ieee, struct sk_buff *skb,
>
>   			if (!network)
>   				return 1;
> -			memset(network, 0, sizeof(*network));
>   			ieee->state = RTLLIB_LINKED;
>   			ieee->assoc_id = aid;
>   			ieee->softmac_stats.rx_ass_ok++;
> @@ -2259,8 +2258,8 @@ inline int rtllib_rx_assoc_resp(struct rtllib_device *ieee, struct sk_buff *skb,
>   					ieee->handle_assoc_response(ieee->dev,
>   						 (struct rtllib_assoc_response_frame *)header,
>   						 network);
> -				kfree(network);
>   			}
> +			kfree(network);
>
>   			kfree(ieee->assocresp_ies);
>   			ieee->assocresp_ies = NULL;

Re: [PATCH] Staging, rtl8192e, softmac: remove redundant memset and fix mem leak

[Sean MacLennan]

On Mon, 13 Feb 2012 00:15:02 +0100 (CET)
Jesper Juhl <jj@chaosbits.net> wrote:

> We also fail to kfree() the memory we allocated for 'network' if we
> do not enter
> 
>   if (ieee->current_network.qos_data.supported == 1) {
> 
> and the variable then goes out of scope.
> 
> To fix that I simply moved the kfree() that was inside that 'if'
> statement to instead be just after it. It then covers both the case
> where we take the branch and when we don't.

Nice catch! We know that the driver leaks memory if left running for a
long time, this will help!

I would recommend a small change: instead of moving the kfree() out of
the loop, why not move the kzalloc into it? The qos_data.supported == 0
is the normal case (at least for me), so why not save an alloc?

Something like this:

diff --git a/drivers/staging/rtl8192e/rtllib_softmac.c b/drivers/staging/rtl8192e/rtllib_softmac.c
index 1637f11..59b991f 100644
--- a/drivers/staging/rtl8192e/rtllib_softmac.c
+++ b/drivers/staging/rtl8192e/rtllib_softmac.c
@@ -2228,13 +2228,6 @@ inline int rtllib_rx_assoc_resp(struct rtllib_device *ieee, struct sk_buff *skb,
 	     (ieee->iw_mode == IW_MODE_INFRA)) {
 		errcode = assoc_parse(ieee, skb, &aid);
 		if (0 == errcode) {
-			struct rtllib_network *network =
-				 kzalloc(sizeof(struct rtllib_network),
-				 GFP_ATOMIC);
-
-			if (!network)
-				return 1;
-			memset(network, 0, sizeof(*network));
 			ieee->state = RTLLIB_LINKED;
 			ieee->assoc_id = aid;
 			ieee->softmac_stats.rx_ass_ok++;
@@ -2242,6 +2235,13 @@ inline int rtllib_rx_assoc_resp(struct rtllib_device *ieee, struct sk_buff *skb,
 			/* Let the register setting default with Legacy station */
 			assoc_resp = (struct rtllib_assoc_response_frame *)skb->data;
 			if (ieee->current_network.qos_data.supported == 1) {
+				struct rtllib_network *network =
+					kzalloc(sizeof(struct rtllib_network),
+						GFP_ATOMIC);
+
+				if (!network)
+					return 1;
+
 				if (rtllib_parse_info_param(ieee, assoc_resp->info_element,
 							rx_stats->len - sizeof(*assoc_resp),
 							network, rx_stats)) {

Cheers,
   Sean