diff mbox series

[01/23] builtin/replay: plug leaking `advance_name` variable

Message ID dd044eacc2b20c3e426f5825fdc30f4db6618052.1721995576.git.ps@pks.im (mailing list archive)
State Superseded
Headers show
Series Memory leak fixes (pt.3) | expand

Commit Message

Patrick Steinhardt July 26, 2024, 12:13 p.m. UTC
The `advance_name` variable can either contain a static string when
parsed via the `--advance` command line option or it may be an allocated
string when set via `determine_replay_mode()`. Because we cannot be sure
whether it is allocated or not we just didn't free it at all, resulting
in a memory leak.

Split up the variables such that we can track the static and allocated
strings separately and then free the allocated one to fix the memory
leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/replay.c         | 20 ++++++++++++++------
 t/t3650-replay-basics.sh |  1 +
 2 files changed, 15 insertions(+), 6 deletions(-)

Comments

Taylor Blau July 31, 2024, 4:22 p.m. UTC | #1
On Fri, Jul 26, 2024 at 02:13:59PM +0200, Patrick Steinhardt wrote:
> The `advance_name` variable can either contain a static string when
> parsed via the `--advance` command line option or it may be an allocated
> string when set via `determine_replay_mode()`. Because we cannot be sure
> whether it is allocated or not we just didn't free it at all, resulting
> in a memory leak.
>
> Split up the variables such that we can track the static and allocated
> strings separately and then free the allocated one to fix the memory
> leak.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/replay.c         | 20 ++++++++++++++------
>  t/t3650-replay-basics.sh |  1 +
>  2 files changed, 15 insertions(+), 6 deletions(-)

I had to read this patch a couple of times to make sure that I
understood the flow into and out of determine_replay_mode(). But after
reading it a couple of times, I agree with the change that you made
here.

Thanks,
Taylor
diff mbox series

Patch

diff --git a/builtin/replay.c b/builtin/replay.c
index 0448326636..27b40eda98 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -151,7 +151,7 @@  static void get_ref_information(struct rev_cmdline_info *cmd_info,
 
 static void determine_replay_mode(struct rev_cmdline_info *cmd_info,
 				  const char *onto_name,
-				  const char **advance_name,
+				  char **advance_name,
 				  struct commit **onto,
 				  struct strset **update_refs)
 {
@@ -174,6 +174,7 @@  static void determine_replay_mode(struct rev_cmdline_info *cmd_info,
 		*onto = peel_committish(*advance_name);
 		if (repo_dwim_ref(the_repository, *advance_name, strlen(*advance_name),
 			     &oid, &fullname, 0) == 1) {
+			free(*advance_name);
 			*advance_name = fullname;
 		} else {
 			die(_("argument to --advance must be a reference"));
@@ -197,6 +198,7 @@  static void determine_replay_mode(struct rev_cmdline_info *cmd_info,
 		if (negative_refs_complete) {
 			struct hashmap_iter iter;
 			struct strmap_entry *entry;
+			const char *last_key = NULL;
 
 			if (rinfo.negative_refexprs == 0)
 				die(_("all positive revisions given must be references"));
@@ -208,8 +210,11 @@  static void determine_replay_mode(struct rev_cmdline_info *cmd_info,
 			/* Only one entry, but we have to loop to get it */
 			strset_for_each_entry(&rinfo.negative_refs,
 					      &iter, entry) {
-				*advance_name = entry->key;
+				last_key = entry->key;
 			}
+
+			free(*advance_name);
+			*advance_name = xstrdup_or_null(last_key);
 		} else { /* positive_refs_complete */
 			if (rinfo.negative_refexprs > 1)
 				die(_("cannot implicitly determine correct base for --onto"));
@@ -271,7 +276,8 @@  static struct commit *pick_regular_commit(struct commit *pickme,
 
 int cmd_replay(int argc, const char **argv, const char *prefix)
 {
-	const char *advance_name = NULL;
+	const char *advance_name_opt = NULL;
+	char *advance_name = NULL;
 	struct commit *onto = NULL;
 	const char *onto_name = NULL;
 	int contained = 0;
@@ -292,7 +298,7 @@  int cmd_replay(int argc, const char **argv, const char *prefix)
 		NULL
 	};
 	struct option replay_options[] = {
-		OPT_STRING(0, "advance", &advance_name,
+		OPT_STRING(0, "advance", &advance_name_opt,
 			   N_("branch"),
 			   N_("make replay advance given branch")),
 		OPT_STRING(0, "onto", &onto_name,
@@ -306,14 +312,15 @@  int cmd_replay(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, replay_options, replay_usage,
 			     PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN_OPT);
 
-	if (!onto_name && !advance_name) {
+	if (!onto_name && !advance_name_opt) {
 		error(_("option --onto or --advance is mandatory"));
 		usage_with_options(replay_usage, replay_options);
 	}
 
-	if (advance_name && contained)
+	if (advance_name_opt && contained)
 		die(_("options '%s' and '%s' cannot be used together"),
 		    "--advance", "--contained");
+	advance_name = xstrdup_or_null(advance_name_opt);
 
 	repo_init_revisions(the_repository, &revs, prefix);
 
@@ -441,6 +448,7 @@  int cmd_replay(int argc, const char **argv, const char *prefix)
 
 cleanup:
 	release_revisions(&revs);
+	free(advance_name);
 
 	/* Return */
 	if (ret < 0)
diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
index 389670262e..12bd3db4cb 100755
--- a/t/t3650-replay-basics.sh
+++ b/t/t3650-replay-basics.sh
@@ -5,6 +5,7 @@  test_description='basic git replay tests'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 GIT_AUTHOR_NAME=author@name