tools/hv: Memory Leak on realloc

[PATCH] tools/hv: Memory Leak on realloc

Posted by Malaya Kumar Rout 9 months ago

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

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

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

Posted by Wei Liu 8 months, 2 weeks ago

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

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

Posted by malaya kumar rout 8 months, 2 weeks ago

On Mon, Apr 7, 2025 at 11:07 AM Wei Liu <wei.liu@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-kernel@vger.kernel.org
> >     linux-hyperv@vger.kernel.org
> > Signed-off-by: Malaya Kumar Rout <malayarout91@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
> >

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

Posted by malaya kumar rout 8 months, 2 weeks ago

On Sat, Mar 22, 2025 at 10:17 PM Malaya Kumar Rout
<malayarout91@gmail.com> 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
>
> CC: linux-kernel@vger.kernel.org
>     linux-hyperv@vger.kernel.org
> Signed-off-by: Malaya Kumar Rout <malayarout91@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
>

This is a courteous reminder regarding the current patch.
Any feedback or insights you may have would be greatly appreciated.

Thanks & Regards,
Malaya Kumar Rout