linux-kernel - Re: [PATCH] tools/hv: Memory Leak on realloc

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] [day] [month] [year] [list]

Message-ID: <CAE2+fR_t2-207Ji+YoFKt2QrotCfAME0Ean9EbPEn-9c7HuONQ@mail.gmail.com>
Date: Mon, 7 Apr 2025 12:44:23 +0530
From: malaya kumar rout <malayarout91@...il.com>
To: Wei Liu <wei.liu@...nel.org>
Cc: linux-kernel@...r.kernel.org, "K. Y. Srinivasan" <kys@...rosoft.com>, 
	Haiyang Zhang <haiyangz@...rosoft.com>, Dexuan Cui <decui@...rosoft.com>, 
	linux-hyperv@...r.kernel.org
Subject: Re: [PATCH] tools/hv: Memory Leak on realloc

On Mon, Apr 7, 2025 at 11:07 AM Wei Liu <wei.liu@...nel.org> wrote:
>
> 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.
>
I greatly appreciate your prompt review.

Thanks,
Malaya Kumar Rout

> >
> > 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
> >