Re: [musl] [PATCH] time/tz: Fix memory leak when do_tzset() is repeated

[Rich Felker <dalias@aerifal.cx>]

On Tue, May 09, 2023 at 08:44:24PM +0800, Xiaoming Ni wrote:
> When do_tzset() and setenv("TZ") are repeatedly called,
> "old_tz" is repeatedly applied for memory and is not released,
>  triggering memory leakage.
> 
> 	s = getenv("TZ");
> 	i = strlen(s);
> 	if (i >= old_tz_size) {
> 		old_tz = malloc(old_tz_size);// without free old value
> 	}
> 
> add free(old_tz) to avoid memory leak.
> 
> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> ---
>  src/time/__tz.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/time/__tz.c b/src/time/__tz.c
> index c34b3eb7..917740ea 100644
> --- a/src/time/__tz.c
> +++ b/src/time/__tz.c
> @@ -151,6 +151,7 @@ static void do_tzset()
>  		old_tz_size *= 2;
>  		if (i >= old_tz_size) old_tz_size = i+1;
>  		if (old_tz_size > PATH_MAX+2) old_tz_size = PATH_MAX+2;
> +		if (old_tz != old_tz_buf) free(old_tz);
>  		old_tz = malloc(old_tz_size);
>  	}
>  	if (old_tz) memcpy(old_tz, s, i+1);
> -- 
> 2.27.0

The whole purpose of the code block you're modifying is to grow the
size of the buffer geometrically so that the total allocation is well
bounded (on the order of 2x PATH_MAX) regardless of how many times you
change the TZ or how long a TZ string is, so that it does not have to
be freed. On top of this, a normal program that's not doing anything
unsafe (modifying own environment is unsafe without constraints like
knowing you'll always be single-threaded) only allocates once.

In general, musl avoids having libc components depend on malloc, and
when that's inevitable, avoids having them depend on free unless the
interface contract requires it. This way, only programs that actually
use malloc need to link malloc.

Rich