linux-kernel - Re: [PATCH] tools/thermal: fix memory leak in realloc failure handling

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: <93c87b5c-ae8f-d27d-0342-faa884a3142e@linaro.org>
Date:   Mon, 25 Sep 2023 11:42:03 +0200
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Kuan-Wei Chiu <visitorckw@...il.com>, rafael@...nel.org
Cc:     amitk@...nel.org, rui.zhang@...el.com, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] tools/thermal: fix memory leak in realloc failure
 handling


Hi Kuan-Wei,

On 24/09/2023 08:50, Kuan-Wei Chiu wrote:
> In the previous code, there was a memory leak issue where the
> previously allocated memory was not freed upon a failed realloc
> operation. This patch addresses the problem by releasing the old memory
> before setting the pointer to NULL in case of a realloc failure. This
> ensures that memory is properly managed and avoids potential memory
> leaks.

Thanks for reporting the issue and proposing the fix.

The description is not accurate actually, neither the fix.

What is happening is we are losing the pointer information as the 'mds' 
variable is a global variable. So the assignation will overwrite the 
current pointer if it fails. That leads to a NULL pointer dereference in 
the mainloop_del.

Looking closer to the code, it seems 'mds' is not used as the stored 
information is not accessed.

For my understanding, we can just remove the:

static struct mainloop_data **mds

and

static unsigned short nrhandler;

along with the associated code


> Signed-off-by: Kuan-Wei Chiu <visitorckw@...il.com>
> ---
>   tools/thermal/lib/mainloop.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/thermal/lib/mainloop.c b/tools/thermal/lib/mainloop.c
> index 94cbbcbd1c14..6dcc4090d47e 100644
> --- a/tools/thermal/lib/mainloop.c
> +++ b/tools/thermal/lib/mainloop.c
> @@ -62,9 +62,13 @@ int mainloop_add(int fd, mainloop_callback_t cb, void *data)
>   	struct mainloop_data *md;
>   
>   	if (fd >= nrhandler) {
> -		mds = realloc(mds, sizeof(*mds) * (fd + 1));
> -		if (!mds)
> +		struct mainloop_data **mds_tmp =
> +			realloc(mds, sizeof(*mds) * (fd + 1));
> +		if (!mds_tmp) {
> +			free(mds);
>   			return -1;
> +		}
> +		mds = mds_tmp;
>   		nrhandler = fd + 1;
>   	}
>   

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog