Hi Andrzej
Thanks for working on removing memory leaks from git.
On 20/06/2021 16:12, andrzej@xxxxxxxxx wrote:
From: Andrzej Hunt <ajrhunt@xxxxxxxxxx>
This change:
- xstrdup()'s all string being used for replace_opts.strategy, to
I think you mean replay_opts rather than replace_opts.
guarantee that replace_opts owns these strings. This is needed because
sequencer_remove_state() will free replace_opts.strategy, and it's
usually called as part of the usage of replace_opts.
- Removes xstrdup()'s being used to populate options.strategy in
cmd_rebase(), which avoids leaking options.strategy, even in the
case where strategy is never moved/copied into replace_opts.
These changes are needed because:
- We would always create a new string for options.strategy if we either
get a strategy via options (OPT_STRING(...strategy...), or via
GIT_TEST_MERGE_ALGORITHM.
- But only sometimes is this string copied into replace_opts - in which
case it did get free()'d in sequencer_remove_state().
- The rest of the time, the newly allocated string would remain unused,
causing a leak. But we can't just add a free because that can result
in a double-free in those cases where replace_opts was populated.
An alternative approach would be to set options.strategy to NULL when
moving the pointer to replace_opts.strategy, combined with always
free()'ing options.strategy, but that seems like a more
complicated and wasteful approach.
read_basic_state() contains
if (file_exists(state_dir_path("strategy", opts))) {
strbuf_reset(&buf);
if (!read_oneliner(&buf, state_dir_path("strategy", opts),
READ_ONELINER_WARN_MISSING))
return -1;
free(opts->strategy);
opts->strategy = xstrdup(buf.buf);
}
So we do try to free opts->strategy when reading the state from disc and
we allocate a new string. I suspect that opts->strategy is actually NULL
in when this function is called but I haven't checked. Given that we are
allocating a copy above I think maybe your alternative approach of
always freeing opts->strategy would be better.
Best Wishes
Phillip
This was first seen when running t0021 with LSAN, but t2012 helped catch
the fact that we can't just free(options.strategy) at the end of
cmd_rebase (as that can cause a double-free). LSAN output from t0021:
LSAN output from t0021:
Direct leak of 4 byte(s) in 1 object(s) allocated from:
#0 0x486804 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
#1 0xa71eb8 in xstrdup wrapper.c:29:14
#2 0x61b1cc in cmd_rebase builtin/rebase.c:1779:22
#3 0x4ce83e in run_builtin git.c:475:11
#4 0x4ccafe in handle_builtin git.c:729:3
#5 0x4cb01c in run_argv git.c:818:4
#6 0x4cb01c in cmd_main git.c:949:19
#7 0x6b3fad in main common-main.c:52:11
#8 0x7f267b512349 in __libc_start_main (/lib64/libc.so.6+0x24349)
SUMMARY: AddressSanitizer: 4 byte(s) leaked in 1 allocation(s).
Signed-off-by: Andrzej Hunt <andrzej@xxxxxxxxx>
---
builtin/rebase.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 12f093121d..9d81db0f3a 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -139,7 +139,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
replay.ignore_date = opts->ignore_date;
replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
if (opts->strategy)
- replay.strategy = opts->strategy;
+ replay.strategy = xstrdup_or_null(opts->strategy);
else if (!replay.strategy && replay.default_strategy) {
replay.strategy = replay.default_strategy;
replay.default_strategy = NULL;
@@ -1723,7 +1723,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
}
if (options.strategy) {
- options.strategy = xstrdup(options.strategy);
switch (options.type) {
case REBASE_APPLY:
die(_("--strategy requires --merge or --interactive"));
@@ -1776,7 +1775,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (options.type == REBASE_MERGE &&
!options.strategy &&
getenv("GIT_TEST_MERGE_ALGORITHM"))
- options.strategy = xstrdup(getenv("GIT_TEST_MERGE_ALGORITHM"));
+ options.strategy = getenv("GIT_TEST_MERGE_ALGORITHM");
switch (options.type) {
case REBASE_MERGE:
