lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z_NknPrx26vMuJ-9@liuwe-devbox-ubuntu-v2.tail21d00.ts.net>
Date: Mon, 7 Apr 2025 05:37:32 +0000
From: Wei Liu <wei.liu@...nel.org>
To: Malaya Kumar Rout <malayarout91@...il.com>
Cc: linux-kernel@...r.kernel.org, "K. Y. Srinivasan" <kys@...rosoft.com>,
	Haiyang Zhang <haiyangz@...rosoft.com>,
	Wei Liu <wei.liu@...nel.org>, Dexuan Cui <decui@...rosoft.com>,
	linux-hyperv@...r.kernel.org
Subject: Re: [PATCH] tools/hv: Memory Leak on realloc

On Sat, Mar 22, 2025 at 10:16:47PM +0530, Malaya Kumar Rout wrote:
> Static analysis for hv_kvp_daemon.c with cppcheck : error:
> 
> hv_kvp_daemon.c:359:3: error: Common realloc mistake:
> 'record' nulled but not freed upon failure [memleakOnRealloc]
> record = realloc(record, sizeof(struct kvp_record) *
> 
> If realloc() fails, record is now NULL.
> If we directly assign this NULL to record, the reference to the previously allocated memory is lost.
> This causes a memory leak because the old allocated memory remains but is no longer accessible.
> 
> A temporary pointer was utilized when invoking realloc() to prevent
> the loss of the original allocation in the event of a failure

While this patch finds a problem, it misses the big picture.

If realloc fails, the process quits. It depends on the OS to reclaim the
memory. Freeing this one instance to prevent a memory leak is not
sufficient -- there can be already memory allocated prior to a failure.

Unless this program is sufficiently reworked to be resilient against
OOM, this change itself does not bring much value.

That being said, thank you for writing a patch and went through the
trouble to submit it.

Thanks,
Wei.

> 
> CC: linux-kernel@...r.kernel.org
>     linux-hyperv@...r.kernel.org
> Signed-off-by: Malaya Kumar Rout <malayarout91@...il.com>
> ---
>  tools/hv/hv_kvp_daemon.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> index 04ba035d67e9..6807832209f0 100644
> --- a/tools/hv/hv_kvp_daemon.c
> +++ b/tools/hv/hv_kvp_daemon.c
> @@ -356,11 +356,14 @@ 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));
> -
> -		if (record == NULL)
> +		struct kvp_record *temp = realloc(record, sizeof(struct kvp_record) *
> +				ENTRIES_PER_BLOCK * (num_blocks + 1));
> +		if (!temp) {
> +			free(record);
> +			record = NULL;
>  			return 1;
> +		}
> +		record = temp;
>  		kvp_file_info[pool].num_blocks++;
>  
>  	}
> -- 
> 2.43.0
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ