[PATCH] Staging: wlan-ng: memory allocated inside mkimage() is not freed if subsequent calls fails.

[PATCH] Staging: wlan-ng: memory allocated inside mkimage() is not freed if subsequent calls fails.

[Claudiu Beznea]

This patch frees memory allocated inside mkimage() in case mkimage()
or any other subsequent calls inside prism2_fwapply() from prism2fw.c
file fails. To fix this I introduces goto labels where the free
operation is done in case some operations fails. After the introduction
of goto labels has been done, in order to use the same return path,
"return x" instuctions were replaced with "goto" instuctions.

Signed-off-by: Claudiu Beznea <claudiu.beznea@gmail.com>
---
 drivers/staging/wlan-ng/prism2fw.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/wlan-ng/prism2fw.c b/drivers/staging/wlan-ng/prism2fw.c
index 8564d9e..56bffd9 100644
--- a/drivers/staging/wlan-ng/prism2fw.c
+++ b/drivers/staging/wlan-ng/prism2fw.c
@@ -278,7 +278,8 @@ static int prism2_fwapply(const struct ihex_binrec *rfptr,
 	/* Build the PDA we're going to use. */
 	if (read_cardpda(&pda, wlandev)) {
 		netdev_err(wlandev->netdev, "load_cardpda failed, exiting.\n");
-		return 1;
+		result = 1;
+		goto out;
 	}
 
 	/* read the card's PRI-SUP */
@@ -315,55 +316,58 @@ static int prism2_fwapply(const struct ihex_binrec *rfptr,
 	if (result) {
 		netdev_err(wlandev->netdev,
 			   "Failed to read the data exiting.\n");
-		return 1;
+		goto out;
 	}
 
 	result = validate_identity();
-
 	if (result) {
 		netdev_err(wlandev->netdev, "Incompatible firmware image.\n");
-		return 1;
+		goto out;
 	}
 
 	if (startaddr == 0x00000000) {
 		netdev_err(wlandev->netdev,
 			   "Can't RAM download a Flash image!\n");
-		return 1;
+		result = 1;
+		goto out;
 	}
 
 	/* Make the image chunks */
 	result = mkimage(fchunk, &nfchunks);
 	if (result) {
 		netdev_err(wlandev->netdev, "Failed to make image chunk.\n");
-		return 1;
+		goto free_chunks;
 	}
 
 	/* Do any plugging */
 	result = plugimage(fchunk, nfchunks, s3plug, ns3plug, &pda);
 	if (result) {
 		netdev_err(wlandev->netdev, "Failed to plug data.\n");
-		return 1;
+		goto free_chunks;
 	}
 
 	/* Insert any CRCs */
-	if (crcimage(fchunk, nfchunks, s3crc, ns3crc)) {
+	result = crcimage(fchunk, nfchunks, s3crc, ns3crc);
+	if (result) {
 		netdev_err(wlandev->netdev, "Failed to insert all CRCs\n");
-		return 1;
+		goto free_chunks;
 	}
 
 	/* Write the image */
 	result = writeimage(wlandev, fchunk, nfchunks);
 	if (result) {
 		netdev_err(wlandev->netdev, "Failed to ramwrite image data.\n");
-		return 1;
+		goto free_chunks;
 	}
 
+	netdev_info(wlandev->netdev, "prism2_usb: firmware loading finished.\n");
+
+free_chunks:
 	/* clear any allocated memory */
 	free_chunks(fchunk, &nfchunks);
 	free_srecs();
 
-	netdev_info(wlandev->netdev, "prism2_usb: firmware loading finished.\n");
-
+out:
 	return result;
 }
 
-- 
1.9.1

Re: [PATCH] Staging: wlan-ng: memory allocated inside mkimage() is not freed if subsequent calls fails.

[Dan Carpenter]

This patch makes the code better and it's an improvement so I'm fine
with merging it as-is.

On Sun, Apr 24, 2016 at 07:40:13PM +0300, Claudiu Beznea wrote:
> This patch frees memory allocated inside mkimage() in case mkimage()
> or any other subsequent calls inside prism2_fwapply() from prism2fw.c
> file fails. To fix this I introduces goto labels where the free
> operation is done in case some operations fails. After the introduction
> of goto labels has been done, in order to use the same return path,
> "return x" instuctions were replaced with "goto" instuctions.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@gmail.com>
> ---
>  drivers/staging/wlan-ng/prism2fw.c | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/wlan-ng/prism2fw.c b/drivers/staging/wlan-ng/prism2fw.c
> index 8564d9e..56bffd9 100644
> --- a/drivers/staging/wlan-ng/prism2fw.c
> +++ b/drivers/staging/wlan-ng/prism2fw.c
> @@ -278,7 +278,8 @@ static int prism2_fwapply(const struct ihex_binrec *rfptr,
>  	/* Build the PDA we're going to use. */
>  	if (read_cardpda(&pda, wlandev)) {
>  		netdev_err(wlandev->netdev, "load_cardpda failed, exiting.\n");
> -		return 1;
> +		result = 1;
> +		goto out;

It's better to do direct returns instead of misleading gotos that don't
do anything.  "Future proofing" is a waste of time and introduces more
bugs than it prevents.  Just write for the present.  Present proof the
code.

>  	}
>  
>  	/* read the card's PRI-SUP */
> @@ -315,55 +316,58 @@ static int prism2_fwapply(const struct ihex_binrec *rfptr,
>  	if (result) {
>  		netdev_err(wlandev->netdev,
>  			   "Failed to read the data exiting.\n");
> -		return 1;
> +		goto out;

After this should be goto free_srecs not goto out.  I don't know that
it matters...

>  	}
>  
>  	result = validate_identity();
> -
>  	if (result) {
>  		netdev_err(wlandev->netdev, "Incompatible firmware image.\n");
> -		return 1;
> +		goto out;
>  	}
>  
>  	if (startaddr == 0x00000000) {
>  		netdev_err(wlandev->netdev,
>  			   "Can't RAM download a Flash image!\n");
> -		return 1;
> +		result = 1;
> +		goto out;
>  	}
>  
>  	/* Make the image chunks */
>  	result = mkimage(fchunk, &nfchunks);
>  	if (result) {
>  		netdev_err(wlandev->netdev, "Failed to make image chunk.\n");
> -		return 1;
> +		goto free_chunks;

This works, but really mkimage() should clean up it's own allocations
on failure.  Otherwise it is a layering violation.

The rest of the patch is fine.

regards,
dan carpenter

[PATCH] Staging: wlan-ng: memory allocated inside mkimage() is not freed if subsequent calls fails.

[Claudiu Beznea]

This patch frees memory allocated inside mkimage() in case mkimage()
or any other subsequent calls inside prism2_fwapply() from prism2fw.c
file fails. To fix this I introduces goto labels where the free
operation is done in case some operations fails. After the introduction
of goto labels has been done, in order to use the same return path,
"return x" instuctions were replaced with "goto" instuctions.

Signed-off-by: Claudiu Beznea <claudiu.beznea@gmail.com>
---
 drivers/staging/wlan-ng/prism2fw.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/wlan-ng/prism2fw.c b/drivers/staging/wlan-ng/prism2fw.c
index 8564d9e..56bffd9 100644
--- a/drivers/staging/wlan-ng/prism2fw.c
+++ b/drivers/staging/wlan-ng/prism2fw.c
@@ -278,7 +278,8 @@ static int prism2_fwapply(const struct ihex_binrec *rfptr,
 	/* Build the PDA we're going to use. */
 	if (read_cardpda(&pda, wlandev)) {
 		netdev_err(wlandev->netdev, "load_cardpda failed, exiting.\n");
-		return 1;
+		result = 1;
+		goto out;
 	}
 
 	/* read the card's PRI-SUP */
@@ -315,55 +316,58 @@ static int prism2_fwapply(const struct ihex_binrec *rfptr,
 	if (result) {
 		netdev_err(wlandev->netdev,
 			   "Failed to read the data exiting.\n");
-		return 1;
+		goto out;
 	}
 
 	result = validate_identity();
-
 	if (result) {
 		netdev_err(wlandev->netdev, "Incompatible firmware image.\n");
-		return 1;
+		goto out;
 	}
 
 	if (startaddr == 0x00000000) {
 		netdev_err(wlandev->netdev,
 			   "Can't RAM download a Flash image!\n");
-		return 1;
+		result = 1;
+		goto out;
 	}
 
 	/* Make the image chunks */
 	result = mkimage(fchunk, &nfchunks);
 	if (result) {
 		netdev_err(wlandev->netdev, "Failed to make image chunk.\n");
-		return 1;
+		goto free_chunks;
 	}
 
 	/* Do any plugging */
 	result = plugimage(fchunk, nfchunks, s3plug, ns3plug, &pda);
 	if (result) {
 		netdev_err(wlandev->netdev, "Failed to plug data.\n");
-		return 1;
+		goto free_chunks;
 	}
 
 	/* Insert any CRCs */
-	if (crcimage(fchunk, nfchunks, s3crc, ns3crc)) {
+	result = crcimage(fchunk, nfchunks, s3crc, ns3crc);
+	if (result) {
 		netdev_err(wlandev->netdev, "Failed to insert all CRCs\n");
-		return 1;
+		goto free_chunks;
 	}
 
 	/* Write the image */
 	result = writeimage(wlandev, fchunk, nfchunks);
 	if (result) {
 		netdev_err(wlandev->netdev, "Failed to ramwrite image data.\n");
-		return 1;
+		goto free_chunks;
 	}
 
+	netdev_info(wlandev->netdev, "prism2_usb: firmware loading finished.\n");
+
+free_chunks:
 	/* clear any allocated memory */
 	free_chunks(fchunk, &nfchunks);
 	free_srecs();
 
-	netdev_info(wlandev->netdev, "prism2_usb: firmware loading finished.\n");
-
+out:
 	return result;
 }
 
-- 
1.9.1