git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 17/17] built-ins: use free() not UNLEAK() if trivial, rm dead code

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Nov 3, 2022, 17:06 UTC
Message-ID
<patch-17.17-33a5753cc3a-20221103T164632Z-avarab@gmail.com>
In-Reply-To
<cover-00.17-00000000000-20221103T164632Z-avarab@gmail.com>

For a lot of uses of UNLEAK() it would be quite tricky to release the memory involved, or we're missing the relevant *_(release|clear)() functions. But in these cases we have them already, and can just invoke them on the variable(s) involved, instead of UNLEAK().

For "builtin/worktree.c" the UNLEAK() was also added in [1], but the struct member it's unleaking was removed in [2]. The only non-"int" member of that structure is "const char *keep_locked", which comes to us via "argv" or a string literal[3].

We have good visibility via the compiler and tooling (e.g. SANITIZE=address) on bad free()-ing, but none on UNLEAK() we don't need anymore. So let's prefer releasing the memory when it's easy.

For "bugreport", "worktree" and "config" we need to start using a "ret = ..." return pattern. For "builtin/bugreport.c" these UNLEAK() were added in [4], and for "builtin/config.c" in [1].

For "config" the code seen here was the only user of the "value" variable. For "ACTION_{RENAME,REMOVE}_SECTION" we need to be sure to return the right exit code in the cases where we were relying on falling through to the top-level.

I think there's still a use-case for UNLEAK(), but hat it's changed since then. Using it so that "we can see the real leaks" is counter-productive in these cases.

It's more useful to have UNLEAK() be a marker of the remaining odd cases where it's hard to free() the memory for whatever reason. With this change less than 20 of them remain in-tree.

1. 0e5bba53af7 (add UNLEAK annotation for reducing leak false
   positives, 2017-09-08)
2. d861d34a6ed (worktree: remove extra members from struct add_opts,
   2018-04-24)
3. 0db4961c49b (worktree: teach `add` to accept --reason <string> with
  --lock, 2021-07-15)
4. 0e5bba53af7 and 00d8c311050 (commit: fix "author_ident" leak,
   2022-05-12).
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/add.c       |  2 +-
 builtin/bugreport.c |  9 ++++++---
 builtin/commit.c    |  6 +++---
 builtin/config.c    | 42 ++++++++++++++++++++----------------------
 builtin/diff.c      |  2 +-
 builtin/worktree.c  |  7 ++++---
 6 files changed, 35 insertions(+), 33 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index f84372964c8..c68ebafed5e 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -695,6 +695,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		die(_("Unable to write new index file"));
 
 	dir_clear(&dir);
-	UNLEAK(pathspec);
+	clear_pathspec(&pathspec);
 	return exit_status;
 }
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 96052541cbf..5bc254be80f 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -106,6 +106,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 	const char *user_relative_path = NULL;
 	char *prefixed_filename;
 	size_t output_path_len;
+	int ret;
 
 	const struct option bugreport_options[] = {
 		OPT_CALLBACK_F(0, "diagnose", &diagnose, N_("mode"),
@@ -182,7 +183,9 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 		user_relative_path);
 
 	free(prefixed_filename);
-	UNLEAK(buffer);
-	UNLEAK(report_path);
-	return !!launch_editor(report_path.buf, NULL, NULL);
+	strbuf_release(&buffer);
+
+	ret = !!launch_editor(report_path.buf, NULL, NULL);
+	strbuf_release(&report_path);
+	return ret;
 }
diff --git a/builtin/commit.c b/builtin/commit.c
index c291199b704..f88a29167f4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1874,8 +1874,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	apply_autostash(git_path_merge_autostash(the_repository));
 
 cleanup:
-	UNLEAK(author_ident);
-	UNLEAK(err);
-	UNLEAK(sb);
+	strbuf_release(&author_ident);
+	strbuf_release(&err);
+	strbuf_release(&sb);
 	return ret;
 }
diff --git a/builtin/config.c b/builtin/config.c
index 753e5fac297..060cf9f3e05 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -639,8 +639,9 @@ static char *default_user_config(void)
 int cmd_config(int argc, const char **argv, const char *prefix)
 {
 	int nongit = !startup_info->have_repository;
-	char *value;
+	char *value = NULL;
 	int flags = 0;
+	int ret = 0;
 
 	given_config_source.file = xstrdup_or_null(getenv(CONFIG_ENVIRONMENT));
 
@@ -856,44 +857,38 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		free(config_file);
 	}
 	else if (actions == ACTION_SET) {
-		int ret;
 		check_write();
 		check_argc(argc, 2, 2);
 		value = normalize_value(argv[0], argv[1]);
-		UNLEAK(value);
 		ret = git_config_set_in_file_gently(given_config_source.file, argv[0], value);
 		if (ret == CONFIG_NOTHING_SET)
 			error(_("cannot overwrite multiple values with a single value\n"
 			"       Use a regexp, --add or --replace-all to change %s."), argv[0]);
-		return ret;
 	}
 	else if (actions == ACTION_SET_ALL) {
 		check_write();
 		check_argc(argc, 2, 3);
 		value = normalize_value(argv[0], argv[1]);
-		UNLEAK(value);
-		return git_config_set_multivar_in_file_gently(given_config_source.file,
-							      argv[0], value, argv[2],
-							      flags);
+		ret = git_config_set_multivar_in_file_gently(given_config_source.file,
+							     argv[0], value, argv[2],
+							     flags);
 	}
 	else if (actions == ACTION_ADD) {
 		check_write();
 		check_argc(argc, 2, 2);
 		value = normalize_value(argv[0], argv[1]);
-		UNLEAK(value);
-		return git_config_set_multivar_in_file_gently(given_config_source.file,
-							      argv[0], value,
-							      CONFIG_REGEX_NONE,
-							      flags);
+		ret = git_config_set_multivar_in_file_gently(given_config_source.file,
+							     argv[0], value,
+							     CONFIG_REGEX_NONE,
+							     flags);
 	}
 	else if (actions == ACTION_REPLACE_ALL) {
 		check_write();
 		check_argc(argc, 2, 3);
 		value = normalize_value(argv[0], argv[1]);
-		UNLEAK(value);
-		return git_config_set_multivar_in_file_gently(given_config_source.file,
-							      argv[0], value, argv[2],
-							      flags | CONFIG_FLAGS_MULTI_REPLACE);
+		ret = git_config_set_multivar_in_file_gently(given_config_source.file,
+							     argv[0], value, argv[2],
+							     flags | CONFIG_FLAGS_MULTI_REPLACE);
 	}
 	else if (actions == ACTION_GET) {
 		check_argc(argc, 1, 2);
@@ -934,26 +929,28 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 							      flags | CONFIG_FLAGS_MULTI_REPLACE);
 	}
 	else if (actions == ACTION_RENAME_SECTION) {
-		int ret;
 		check_write();
 		check_argc(argc, 2, 2);
 		ret = git_config_rename_section_in_file(given_config_source.file,
 							argv[0], argv[1]);
 		if (ret < 0)
 			return ret;
-		if (ret == 0)
+		else if (!ret)
 			die(_("no such section: %s"), argv[0]);
+		else
+			ret = 0;
 	}
 	else if (actions == ACTION_REMOVE_SECTION) {
-		int ret;
 		check_write();
 		check_argc(argc, 1, 1);
 		ret = git_config_rename_section_in_file(given_config_source.file,
 							argv[0], NULL);
 		if (ret < 0)
 			return ret;
-		if (ret == 0)
+		else if (!ret)
 			die(_("no such section: %s"), argv[0]);
+		else
+			ret = 0;
 	}
 	else if (actions == ACTION_GET_COLOR) {
 		check_argc(argc, 1, 2);
@@ -966,5 +963,6 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		return get_colorbool(argv[0], argc == 2);
 	}
 
-	return 0;
+	free(value);
+	return ret;
 }
diff --git a/builtin/diff.c b/builtin/diff.c
index 854d2c5a5c4..cb63f157dd1 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -609,7 +609,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	if (1 < rev.diffopt.skip_stat_unmatch)
 		refresh_index_quietly();
 	release_revisions(&rev);
-	UNLEAK(ent);
+	object_array_clear(&ent);
 	UNLEAK(blob);
 	return result;
 }
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 4a24d53be15..591d659faea 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -629,6 +629,7 @@ static int add(int ac, const char **av, const char *prefix)
 			 N_("try to match the new branch name with a remote-tracking branch")),
 		OPT_END()
 	};
+	int ret;
 
 	memset(&opts, 0, sizeof(opts));
 	opts.checkout = 1;
@@ -705,9 +706,9 @@ static int add(int ac, const char **av, const char *prefix)
 		die(_("--[no-]track can only be used if a new branch is created"));
 	}
 
-	UNLEAK(path);
-	UNLEAK(opts);
-	return add_worktree(path, branch, &opts);
+	ret = add_worktree(path, branch, &opts);
+	free(path);
+	return ret;
 }
 
 static void show_worktree_porcelain(struct worktree *wt, int line_terminator)

-- 
2.38.0.1451.g86b35f4140a
Previous: Ævar Arnfjörð BjarmasonNext: Phillip Wood
Message 25 of 45 in “leak fixes: use existing constructors & other trivia”
  1. 00/17 leak fixes: use existing constructors & other triviaÆvar Arnfjörð Bjarmason, Nov 3, 2022
  2. 02/17 {reset,merge}: call discard_index() before returningÆvar Arnfjörð Bjarmason, Nov 3, 2022
  3. 01/17 tests: mark tests as passing with SANITIZE=leakÆvar Arnfjörð Bjarmason, Nov 3, 2022
  4. 04/17 read-cache.c: clear and free "sparse_checkout_patterns"Ævar Arnfjörð Bjarmason, Nov 3, 2022
  5. 05/17 dir.c: free "ident" and "exclude_per_dir" in "struct untracked_cache"Ævar Arnfjörð Bjarmason, Nov 3, 2022
  6. 06/17 built-ins & libs & helpers: add/move destructors, fix leaksÆvar Arnfjörð Bjarmason, Nov 3, 2022
  7. 07/17 unpack-file: fix ancient leak in create_temp_file()Ævar Arnfjörð Bjarmason, Nov 3, 2022
  8. 09/17 ls-files: fix a --with-tree memory leakÆvar Arnfjörð Bjarmason, Nov 3, 2022
  9. 10/17 sequencer.c: fix "opts->strategy" leak in read_strategy_opts()Ævar Arnfjörð Bjarmason, Nov 3, 2022
  10. Phillip WoodNov 4, 2022
  11. Taylor BlauNov 4, 2022
  12. Ævar Arnfjörð BjarmasonNov 5, 2022
  13. Phillip WoodNov 8, 2022
  14. Ævar Arnfjörð BjarmasonNov 8, 2022
  15. 03/17 commit: discard partial cache before (re-)reading itÆvar Arnfjörð Bjarmason, Nov 3, 2022
  16. 08/17 revision API: call graph_clear() in release_revisions()Ævar Arnfjörð Bjarmason, Nov 3, 2022
  17. 12/17 sequencer.c: fix a pick_commits() leakÆvar Arnfjörð Bjarmason, Nov 3, 2022
  18. 11/17 connected.c: free the "struct packed_git"Ævar Arnfjörð Bjarmason, Nov 3, 2022
  19. 13/17 rebase: don't leak on "--abort"Ævar Arnfjörð Bjarmason, Nov 3, 2022
  20. Phillip WoodNov 4, 2022
  21. Ævar Arnfjörð BjarmasonNov 5, 2022
  22. 16/17 revert: fix parse_options_concat() leakÆvar Arnfjörð Bjarmason, Nov 3, 2022
  23. 14/17 sequencer.c: fix sequencer_continue() leakÆvar Arnfjörð Bjarmason, Nov 3, 2022
  24. 15/17 cherry-pick: free "struct replay_opts" membersÆvar Arnfjörð Bjarmason, Nov 3, 2022
  25. 17/17 built-ins: use free() not UNLEAK() if trivial, rm dead codeÆvar Arnfjörð Bjarmason, Nov 3, 2022
  26. Phillip WoodNov 4, 2022
  27. Ævar Arnfjörð BjarmasonNov 5, 2022
  28. Phillip WoodNov 7, 2022
  29. 00/15 leak fixes: use existing constructors & other triviaÆvar Arnfjörð Bjarmason, Nov 8, 2022
  30. 01/15 tests: mark tests as passing with SANITIZE=leakÆvar Arnfjörð Bjarmason, Nov 8, 2022
  31. 02/15 {reset,merge}: call discard_index() before returningÆvar Arnfjörð Bjarmason, Nov 8, 2022
  32. 04/15 read-cache.c: clear and free "sparse_checkout_patterns"Ævar Arnfjörð Bjarmason, Nov 8, 2022
  33. 03/15 commit: discard partial cache before (re-)reading itÆvar Arnfjörð Bjarmason, Nov 8, 2022
  34. 05/15 dir.c: free "ident" and "exclude_per_dir" in "struct untracked_cache"Ævar Arnfjörð Bjarmason, Nov 8, 2022
  35. 06/15 built-ins & libs & helpers: add/move destructors, fix leaksÆvar Arnfjörð Bjarmason, Nov 8, 2022
  36. 08/15 revision API: call graph_clear() in release_revisions()Ævar Arnfjörð Bjarmason, Nov 8, 2022
  37. 07/15 unpack-file: fix ancient leak in create_temp_file()Ævar Arnfjörð Bjarmason, Nov 8, 2022
  38. 09/15 ls-files: fix a --with-tree memory leakÆvar Arnfjörð Bjarmason, Nov 8, 2022
  39. 10/15 sequencer.c: fix "opts->strategy" leak in read_strategy_opts()Ævar Arnfjörð Bjarmason, Nov 8, 2022
  40. 11/15 connected.c: free the "struct packed_git"Ævar Arnfjörð Bjarmason, Nov 8, 2022
  41. 13/15 cherry-pick: free "struct replay_opts" membersÆvar Arnfjörð Bjarmason, Nov 8, 2022
  42. 12/15 rebase: don't leak on "--abort"Ævar Arnfjörð Bjarmason, Nov 8, 2022
  43. 15/15 built-ins: use free() not UNLEAK() if trivial, rm dead codeÆvar Arnfjörð Bjarmason, Nov 8, 2022
  44. 14/15 revert: fix parse_options_concat() leakÆvar Arnfjörð Bjarmason, Nov 8, 2022
  45. Taylor BlauNov 8, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.