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
