[PATCH 01/11] config: make sequencer.c's git_config_string_dup() public
- From
Jeff King <peff@peff.net>
- Date
- Apr 7, 2024, 00:58 UTC
- Message-ID
- <20240407005831.GA868358@coredump.intra.peff.net>
- In-Reply-To
- <20240407005656.GA436890@coredump.intra.peff.net>
Just about every caller of git_config_string() has a possible leak in it: if we parse a config variable twice, it will overwrite the pointer that was allocated the first time, leaving the memory unreferenced.
Unfortunately we can't just fix this directly in git_config_string(). Some callers do something like:
const char *foo = "default_value"; ... git_config_string(&foo, var, value);
And we must _not_ free that initial value, as it's a string literal. We can't help those cases easily, as there's no way to distinguish a heap-allocated variable from the default one. But let's start by at least providing a helper that avoids the leak. That will let us convert some cases right away, and give us one potential path forward for the more complex ones.
It turns out we already have such a helper, courtesy of 03a4e260e2 (sequencer: plug memory leaks for the option values, 2016-10-21). The problem is more acute in sequencer.c, which may load config multiple times. Hence the solution was limited to that file back then. But this really is a more general problem within our config callbacks.
Note that the new helper takes a "char *" rather than a const pointer. This is more appropriate, since we tend to use "const" as a signal for a lack of memory ownership (and this function is most certainly asserting ownership over the pointed-to memory).
Signed-off-by: Jeff King <peff@peff.net> --- config.c | 9 +++++++++ config.h | 12 ++++++++++++ sequencer.c | 10 ---------- 3 files changed, 21 insertions(+), 10 deletions(-)
diff --git a/config.c b/config.c index eebce8c7e0..2194fb078a 100644 --- a/config.c +++ b/config.c @@ -1345,6 +1345,15 @@ int git_config_string(const char **dest, const char *var, const char *value) return 0; } +int git_config_string_dup(char **dest, const char *var, const char *value) +{ + if (!value) + return config_error_nonbool(var); + free(*dest); + *dest = xstrdup(value); + return 0; +} + int git_config_pathname(const char **dest, const char *var, const char *value) { if (!value) diff --git a/config.h b/config.h index f4966e3749..cdffc14ccf 100644 --- a/config.h +++ b/config.h @@ -279,9 +279,21 @@ int git_config_bool(const char *, const char *); /** * Allocates and copies the value string into the `dest` parameter; if no * string is given, prints an error message and returns -1. + * + * Note that this function does _not_ free the memory referenced by the + * destination pointer. This makes it safe to use on a variable that initially + * points to a string literal, but it also means that it leaks if the config + * option is seen multiple times. */ int git_config_string(const char **, const char *, const char *); +/** + * Like git_config_string(), but frees any previously-allocated + * string at the destination pointer, avoiding a leak when a + * config variable is seen multiple times. + */ +int git_config_string_dup(char **, const char *, const char *); + /** * Similar to `git_config_string`, but expands `~` or `~user` into the * user's home directory when found at the beginning of the path. diff --git a/sequencer.c b/sequencer.c index 2c19846385..3e5d82e0e5 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2920,16 +2920,6 @@ static int read_populate_todo(struct repository *r, return 0; } -static int git_config_string_dup(char **dest, - const char *var, const char *value) -{ - if (!value) - return config_error_nonbool(var); - free(*dest); - *dest = xstrdup(value); - return 0; -} - static int populate_opts_cb(const char *key, const char *value, const struct config_context *ctx, void *data)
-- 2.44.0.872.g288abe5b5b