From: Derrick Stolee <stolee@gmail.com>
Date: Thu, 15 Aug 2024 13:50:38 GMT
Subject: Re: [PATCH 3/7] builtin/gc: fix leaking config values
Message-ID: <23188674-a465-4af8-ae5a-2404ab6d4c1e@gmail.com>
In-Reply-To: <310e361371efc156c3aaac94439bdbeaa965155f.1723533091.git.ps@pks.im>

On 8/13/24 3:17 AM, Patrick Steinhardt wrote:
> We're leaking config values in git-gc(1) when those values are tracked
> as strings. Introduce a new `gc_config_release()` function that releases
> this memory to plug those leaks and release old values before populating
> the config fields via `git_config_string()` et al.
> 
> Note that there is one small gotcha here with the "--prune" option. Next
> to passing a string, this option also accepts the "--no-prune" option
> that overrides the default or configured value. We thus need to discern
> between the option not having been passed by the user and the negative
> variant of it. This is done by using a simple sentinel value that lets
> us discern these cases.

I'm glad to see that you are correcting this in the very next patch
where I pointed out my concern about it. Excellent.

> -	const char *gc_log_expire;
> -	const char *prune_expire;
> -	const char *prune_worktrees_expire;
> +	char *gc_log_expire;
> +	char *prune_expire;
> +	char *prune_worktrees_expire;

> -	.gc_log_expire = "1.day.ago", \
> -	.prune_expire = "2.weeks.ago", \
> -	.prune_worktrees_expire = "3.months.ago", \
> +	.gc_log_expire = xstrdup("1.day.ago"), \
> +	.prune_expire = xstrdup("2.weeks.ago"), \
> +	.prune_worktrees_expire = xstrdup("3.months.ago"), \

Using xstrdup() is now possible that these aren't globals. Good.

>   static void gc_config(struct gc_config *cfg)
>   {
>   	const char *value;
> +	char *owned = NULL;
>   
>   	if (!git_config_get_value("gc.packrefs", &value)) {
>   		if (value && !strcmp(value, "notbare"))
> @@ -185,15 +195,34 @@ static void gc_config(struct gc_config *cfg)
>   	git_config_get_bool("gc.autodetach", &cfg->detach_auto);
>   	git_config_get_bool("gc.cruftpacks", &cfg->cruft_packs);
>   	git_config_get_ulong("gc.maxcruftsize", &cfg->max_cruft_size);
> -	git_config_get_expiry("gc.pruneexpire", (char **) &cfg->prune_expire);
> -	git_config_get_expiry("gc.worktreepruneexpire", (char **) &cfg->prune_worktrees_expire);
> -	git_config_get_expiry("gc.logexpiry", (char **) &cfg->gc_log_expire);
> +
> +	if (!git_config_get_expiry("gc.pruneexpire", &owned)) {
> +		free(cfg->prune_expire);
> +		cfg->prune_expire = owned;
> +	}

Ah. Good logic of "if I get a (possibly NULL) result, then free the old
value before setting the new one".

> diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
> index 1f1f664871..e641df0116 100755
> --- a/t/t5304-prune.sh
> +++ b/t/t5304-prune.sh
> @@ -7,6 +7,7 @@ test_description='prune'
>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>   
> +TEST_PASSES_SANITIZE_LEAK=true

Fantastic.

Thanks,
-Stolee