From: andrzej@ahunt.org
To: git@vger.kernel.org
Cc: andrzej@ahunt.org
Subject: [PATCH 11/12] builtin/rebase: fix options.strategy memory lifecycle
Date: Sun, 20 Jun 2021 17:12:03 +0200 [thread overview]
Message-ID: <20210620151204.19260-12-andrzej@ahunt.org> (raw)
In-Reply-To: <20210620151204.19260-1-andrzej@ahunt.org>
From: Andrzej Hunt <ajrhunt@google.com>
This change:
- xstrdup()'s all string being used for replace_opts.strategy, to
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.
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@ahunt.org>
---
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:
--
2.26.2
next prev parent reply other threads:[~2021-06-20 15:12 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-20 15:11 [PATCH 00/12] Fix all leaks in tests t0002-t0099: Part 2 andrzej
2021-06-20 15:11 ` [PATCH 01/12] fmt-merge-msg: free newly allocated temporary strings when done andrzej
2021-06-21 20:34 ` Elijah Newren
2021-06-20 15:11 ` [PATCH 02/12] environment: move strbuf into block to plug leak andrzej
2021-06-21 20:49 ` Elijah Newren
2021-06-26 8:27 ` René Scharfe
2021-06-20 15:11 ` [PATCH 03/12] builtin/submodule--helper: release unused strbuf to avoid leak andrzej
2021-06-20 15:11 ` [PATCH 04/12] builtin/for-each-repo: remove unnecessary argv copy to plug leak andrzej
2021-06-21 20:55 ` Elijah Newren
2021-06-20 15:11 ` [PATCH 05/12] diffcore-rename: move old_dir/new_dir definition " andrzej
2021-06-21 14:01 ` Elijah Newren
2021-06-20 15:11 ` [PATCH 06/12] ref-filter: also free head for ATOM_HEAD to avoid leak andrzej
2021-06-21 21:10 ` Elijah Newren
2021-06-20 15:11 ` [PATCH 07/12] read-cache: call diff_setup_done " andrzej
2021-06-21 21:17 ` Elijah Newren
2021-06-20 15:12 ` [PATCH 08/12] convert: release strbuf " andrzej
2021-06-21 20:31 ` Elijah Newren
2021-06-20 15:12 ` [PATCH 09/12] builtin/mv: free or UNLEAK multiple pointers at end of cmd_mv andrzej
2021-06-20 15:12 ` [PATCH 10/12] builtin/merge: free found_ref when done andrzej
2021-06-21 21:27 ` Elijah Newren
2021-06-20 15:12 ` andrzej [this message]
2021-06-20 18:14 ` [PATCH 11/12] builtin/rebase: fix options.strategy memory lifecycle Phillip Wood
2021-06-21 21:39 ` Elijah Newren
2021-06-22 9:02 ` Phillip Wood
2021-07-25 13:03 ` Andrzej Hunt
2021-07-27 19:34 ` Phillip Wood
2021-06-20 15:12 ` [PATCH 12/12] reset: clear_unpack_trees_porcelain to plug leak andrzej
2021-06-21 21:44 ` Elijah Newren
2021-06-21 21:54 ` [PATCH 00/12] Fix all leaks in tests t0002-t0099: Part 2 Elijah Newren
2021-07-25 13:05 ` Andrzej Hunt
2021-07-26 8:01 ` Christian Couder
2021-07-25 13:08 ` [PATCH v2 " andrzej
2021-07-25 13:08 ` [PATCH v2 01/12] fmt-merge-msg: free newly allocated temporary strings when done andrzej
2021-07-26 19:20 ` Junio C Hamano
2021-07-25 13:08 ` [PATCH v2 02/12] environment: move strbuf into block to plug leak andrzej
2021-07-25 13:08 ` [PATCH v2 03/12] builtin/submodule--helper: release unused strbuf to avoid leak andrzej
2021-07-25 13:08 ` [PATCH v2 04/12] builtin/for-each-repo: remove unnecessary argv copy to plug leak andrzej
2021-07-26 20:02 ` Junio C Hamano
2021-07-25 13:08 ` [PATCH v2 05/12] diffcore-rename: move old_dir/new_dir definition " andrzej
2021-07-26 20:02 ` Junio C Hamano
2021-07-25 13:08 ` [PATCH v2 06/12] ref-filter: also free head for ATOM_HEAD to avoid leak andrzej
2021-07-26 20:04 ` Junio C Hamano
2021-07-25 13:08 ` [PATCH v2 07/12] read-cache: call diff_setup_done " andrzej
2021-07-26 20:10 ` Junio C Hamano
2021-07-25 13:08 ` [PATCH v2 08/12] convert: release strbuf " andrzej
2021-07-26 20:15 ` Junio C Hamano
2021-07-25 13:08 ` [PATCH v2 09/12] builtin/mv: free or UNLEAK multiple pointers at end of cmd_mv andrzej
2021-07-25 13:08 ` [PATCH v2 10/12] builtin/merge: free found_ref when done andrzej
2021-07-25 13:08 ` [PATCH v2 11/12] builtin/rebase: fix options.strategy memory lifecycle andrzej
2021-07-25 13:08 ` [PATCH v2 12/12] reset: clear_unpack_trees_porcelain to plug leak andrzej
2021-07-26 20:20 ` [PATCH v2 00/12] Fix all leaks in tests t0002-t0099: Part 2 Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210620151204.19260-12-andrzej@ahunt.org \
--to=andrzej@ahunt.org \
--cc=git@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).