lib/flash: fix resource leak in flash_setup_buffer() error paths
diff mbox

Message ID 1462242428-583-1-git-send-email-andrew.donnellan@au1.ibm.com
State Superseded
Headers show

Commit Message

Andrew Donnellan May 3, 2016, 2:27 a.m. UTC
Some error paths in flash_setup_buffer() don't free the flash_info struct
allocated in that function before they return. Change them to goto the
cleanup code at the end. Separate the cleanup code into separate labels for
calling arch_flash_close() and just freeing memory.

Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>

---

Found by Coverity Scan. Build-tested only. sammj, please bikeshed :)
---
 lib/flash/flash.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

Cyril Bur May 3, 2016, 8:07 a.m. UTC | #1
On Tue,  3 May 2016 12:27:08 +1000
Andrew Donnellan <andrew.donnellan@au1.ibm.com> wrote:

> Some error paths in flash_setup_buffer() don't free the flash_info struct
> allocated in that function before they return. Change them to goto the
> cleanup code at the end. Separate the cleanup code into separate labels for
> calling arch_flash_close() and just freeing memory.

Looks good, I know talloc can be rather magic but surely it can't work that
out...

While you're at it though, missed one...

> 
> Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
> 
> ---
> 
> Found by Coverity Scan. Build-tested only. sammj, please bikeshed :)
> ---
>  lib/flash/flash.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/flash/flash.c b/lib/flash/flash.c
> index b6188a0..107ba9e 100644
> --- a/lib/flash/flash.c
> +++ b/lib/flash/flash.c
> @@ -82,26 +82,26 @@ static struct flash_info *flash_setup_buffer(void *ctx, const char *partition)
>  	rc = arch_flash_init(&info->bl, NULL, true);
>  	if (rc) {
>  		pb_log("Failed to init mtd device\n");
> -		return NULL;
> +		goto out;
>  	}
>  
>  	rc = blocklevel_get_info(info->bl, &info->path, &info->size,
>  				 &info->erase_granule);
>  	if (rc) {
>  		pb_log("Failed to retrieve blocklevel info\n");
> -		return NULL;
> +	        goto out_close;
>  	}
>  
>  	rc = ffs_init(0, info->size, info->bl, &info->ffs, 1);
>  	if (rc) {
>  		pb_log("%s: Failed to init ffs\n", __func__);
> -		goto out;
> +		goto out_close;
>  	}
>  
>  	rc = partition_info(info, partition);
>  	if (rc) {
>  		pb_log("Failed to retrieve partition info\n");

		ffs_close(info->ffs);

> -		goto out;
> +		goto out_close;
>  	}
>  
>  	/* Check if there is a second flash side. If there is not, or
> @@ -139,8 +139,9 @@ static struct flash_info *flash_setup_buffer(void *ctx, const char *partition)
>  	}
>  
>  	return info;
> -out:
> +out_close:
>  	arch_flash_close(info->bl, NULL);
> +out:
>  	talloc_free(info);
>  	return NULL;
>  }
Andrew Donnellan May 4, 2016, 12:55 a.m. UTC | #2
On 03/05/16 18:07, Cyril Bur wrote:
> Looks good, I know talloc can be rather magic but surely it can't work that
> out...
>
> While you're at it though, missed one...
>
> 		ffs_close(info->ffs);

Thanks Cyril - will submit V2

Patch
diff mbox

diff --git a/lib/flash/flash.c b/lib/flash/flash.c
index b6188a0..107ba9e 100644
--- a/lib/flash/flash.c
+++ b/lib/flash/flash.c
@@ -82,26 +82,26 @@  static struct flash_info *flash_setup_buffer(void *ctx, const char *partition)
 	rc = arch_flash_init(&info->bl, NULL, true);
 	if (rc) {
 		pb_log("Failed to init mtd device\n");
-		return NULL;
+		goto out;
 	}
 
 	rc = blocklevel_get_info(info->bl, &info->path, &info->size,
 				 &info->erase_granule);
 	if (rc) {
 		pb_log("Failed to retrieve blocklevel info\n");
-		return NULL;
+	        goto out_close;
 	}
 
 	rc = ffs_init(0, info->size, info->bl, &info->ffs, 1);
 	if (rc) {
 		pb_log("%s: Failed to init ffs\n", __func__);
-		goto out;
+		goto out_close;
 	}
 
 	rc = partition_info(info, partition);
 	if (rc) {
 		pb_log("Failed to retrieve partition info\n");
-		goto out;
+		goto out_close;
 	}
 
 	/* Check if there is a second flash side. If there is not, or
@@ -139,8 +139,9 @@  static struct flash_info *flash_setup_buffer(void *ctx, const char *partition)
 	}
 
 	return info;
-out:
+out_close:
 	arch_flash_close(info->bl, NULL);
+out:
 	talloc_free(info);
 	return NULL;
 }