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-ker...@vger.kernel.org
> linux-hyperv@vger.kernel.org
> Signed-off-by: Malaya Kumar Rout <malayarou...@gmail.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
>
