[PATCH] tools: hv: kvp: fix memory leak in realloc failure handling

[PATCH] tools: hv: kvp: fix memory leak in realloc failure handling

[Kuan-Wei Chiu]

In the previous code, there was a memory leak issue where the
previously allocated memory was not freed upon a failed realloc
operation. This patch addresses the problem by releasing the old memory
before setting the pointer to NULL in case of a realloc failure. This
ensures that memory is properly managed and avoids potential memory
leaks.

Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
---
 tools/hv/hv_kvp_daemon.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index 27f5e7dfc2f7..af180278d56d 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -209,11 +209,13 @@ static void kvp_update_mem_state(int pool)
 			 * We have more data to read.
 			 */
 			num_blocks++;
-			record = realloc(record, alloc_unit * num_blocks);
+			struct kvp_record *record_tmp =
+				realloc(record, alloc_unit * num_blocks);
 
-			if (record == NULL) {
+			if (record_tmp == NULL) {
 				syslog(LOG_ERR, "malloc failed");
 				kvp_release_lock(pool);
+				free(record);
 				exit(EXIT_FAILURE);
 			}
 			continue;
@@ -345,11 +347,15 @@ static int kvp_key_add_or_modify(int pool, const __u8 *key, int key_size,
 	 */
 	if (num_records == (ENTRIES_PER_BLOCK * num_blocks)) {
 		/* Need to allocate a larger array for reg entries. */
-		record = realloc(record, sizeof(struct kvp_record) *
-			 ENTRIES_PER_BLOCK * (num_blocks + 1));
+		struct kvp_record *record_tmp = realloc(
+			record, sizeof(struct kvp_record) * ENTRIES_PER_BLOCK *
+					(num_blocks + 1));
 
-		if (record == NULL)
+		if (record_tmp == NULL) {
+			free(record);
 			return 1;
+		}
+		record = record_tmp;
 		kvp_file_info[pool].num_blocks++;
 
 	}
-- 
2.25.1

Re: [PATCH] tools: hv: kvp: fix memory leak in realloc failure handling

[Kuan-Wei Chiu]

On Sun, Sep 24, 2023 at 01:51:48PM +0800, Kuan-Wei Chiu wrote:
> In the previous code, there was a memory leak issue where the
> previously allocated memory was not freed upon a failed realloc
> operation. This patch addresses the problem by releasing the old memory
> before setting the pointer to NULL in case of a realloc failure. This
> ensures that memory is properly managed and avoids potential memory
> leaks.
> 
> Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
> ---
>  tools/hv/hv_kvp_daemon.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> index 27f5e7dfc2f7..af180278d56d 100644
> --- a/tools/hv/hv_kvp_daemon.c
> +++ b/tools/hv/hv_kvp_daemon.c
> @@ -209,11 +209,13 @@ static void kvp_update_mem_state(int pool)
>  			 * We have more data to read.
>  			 */
>  			num_blocks++;
> -			record = realloc(record, alloc_unit * num_blocks);
> +			struct kvp_record *record_tmp =
> +				realloc(record, alloc_unit * num_blocks);
>  
> -			if (record == NULL) {
> +			if (record_tmp == NULL) {
>  				syslog(LOG_ERR, "malloc failed");
>  				kvp_release_lock(pool);
> +				free(record);
>  				exit(EXIT_FAILURE);
>  			}
>  			continue;
> @@ -345,11 +347,15 @@ static int kvp_key_add_or_modify(int pool, const __u8 *key, int key_size,
>  	 */
>  	if (num_records == (ENTRIES_PER_BLOCK * num_blocks)) {
>  		/* Need to allocate a larger array for reg entries. */
> -		record = realloc(record, sizeof(struct kvp_record) *
> -			 ENTRIES_PER_BLOCK * (num_blocks + 1));
> +		struct kvp_record *record_tmp = realloc(
> +			record, sizeof(struct kvp_record) * ENTRIES_PER_BLOCK *
> +					(num_blocks + 1));
>  
> -		if (record == NULL)
> +		if (record_tmp == NULL) {
> +			free(record);
>  			return 1;
> +		}
> +		record = record_tmp;
>  		kvp_file_info[pool].num_blocks++;
>  
>  	}
> -- 
> 2.25.1
>
After tracing the code more thoroughly, I have come to the realization
that the original codebase already handles memory management correctly.
It verifies the success of the realloc operation before updating the
pointer, which means there is no memory leak issue, and there is no
need to release memory explicitly.

Consequently, my proposed changes are unnecessary and could potentially
introduce problems if implemented.

Best regards,
Kuan-Wei Chiu