Re: [PATCH 11/12] builtin/rebase: fix options.strategy memory lifecycle
From: Phillip Wood <hidden>
Date: 2021-06-20 18:14:44
Hi Andrzej Thanks for working on removing memory leaks from git. On 20/06/2021 16:12, andrzej@ahunt.org wrote:
From: Andrzej Hunt <redacted> 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
quoted hunk ↗ jump to hunk
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 <redacted> --- 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: