On Mon, Apr 7, 2025 at 11:07 AM Wei Liu <wei....@kernel.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-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
> >
