Re: [PATCH 3/7] builtin/gc: fix leaking config values
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Aug 15, 2024, 13:50 UTC
- 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:
Show 11 quoted lines
> 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.
Show 6 quoted lines
> - 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;
Show 6 quoted lines
> - .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.
Show 19 quoted lines
> 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".
Show 9 quoted lines
> 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