Re: [PATCH 3/7] builtin/gc: fix leaking config values

Derrick Stolee
2024-08-15T13:50:40Z

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