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

[PATCH 10/10] built-ins: plug memory leaks with unpack_trees_options_release()

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Oct 4, 2021, 00:46 UTC
Message-ID
<patch-10.10-06ad6baf566-20211004T002226Z-avarab@gmail.com>
In-Reply-To
<cover-00.10-00000000000-20211004T002226Z-avarab@gmail.com>

Plug memory leaks in various built-ins that were missing unpack_trees_options_release() calls. In an earlier commit these functions were all made to use the "UNPACK_TREES_OPTIONS_INIT" macro, now let's have the ones that didn't clean up their memory do so.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 archive.c           |  9 +++++++--
 builtin/am.c        | 17 ++++++++++++-----
 builtin/checkout.c  |  9 +++++++--
 builtin/clone.c     |  1 +
 builtin/commit.c    |  6 +++++-
 builtin/merge.c     |  6 ++++--
 builtin/read-tree.c | 14 ++++++++++----
 builtin/reset.c     | 13 +++++++++----
 builtin/stash.c     | 14 ++++++++++----
 diff-lib.c          |  5 ++++-
 10 files changed, 69 insertions(+), 25 deletions(-)
diff --git a/archive.c b/archive.c
index 210d7235c5a..003db7d355d 100644
--- a/archive.c
+++ b/archive.c
@@ -306,8 +306,10 @@ int write_archive_entries(struct archiver_args *args,
 		opts.dst_index = args->repo->index;
 		opts.fn = oneway_merge;
 		init_tree_desc(&t, args->tree->buffer, args->tree->size);
-		if (unpack_trees(1, &t, &opts))
-			return -1;
+		if (unpack_trees(1, &t, &opts)) {
+			err = -1;
+			goto cleanup;
+		}
 		git_attr_set_direction(GIT_ATTR_INDEX);
 	}
 
@@ -346,8 +348,11 @@ int write_archive_entries(struct archiver_args *args,
 		if (err)
 			break;
 	}
+
+cleanup:
 	strbuf_release(&path_in_archive);
 	strbuf_release(&content);
+	unpack_trees_options_release(&opts);
 
 	return err;
 }
diff --git a/builtin/am.c b/builtin/am.c
index 82641ce68ec..4d4bb473c0f 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1903,6 +1903,7 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
 	struct lock_file lock_file = LOCK_INIT;
 	struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
 	struct tree_desc t[2];
+	int ret = 0;
 
 	if (parse_tree(head) || parse_tree(remote))
 		return -1;
@@ -1923,13 +1924,15 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
 
 	if (unpack_trees(2, t, &opts)) {
 		rollback_lock_file(&lock_file);
-		return -1;
+		ret = -1;
+		goto cleanup;
 	}
 
 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
 		die(_("unable to write new index file"));
-
-	return 0;
+cleanup:
+	unpack_trees_options_release(&opts);
+	return ret;
 }
 
 /**
@@ -1941,6 +1944,7 @@ static int merge_tree(struct tree *tree)
 	struct lock_file lock_file = LOCK_INIT;
 	struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
 	struct tree_desc t[1];
+	int ret = 0;
 
 	if (parse_tree(tree))
 		return -1;
@@ -1956,13 +1960,16 @@ static int merge_tree(struct tree *tree)
 
 	if (unpack_trees(1, t, &opts)) {
 		rollback_lock_file(&lock_file);
-		return -1;
+		ret = -1;
+		goto cleanup;
 	}
 
 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
 		die(_("unable to write new index file"));
 
-	return 0;
+cleanup:
+	unpack_trees_options_release(&opts);
+	return ret;
 }
 
 /**
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 482d17676a0..fd76b504861 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -641,6 +641,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
 {
 	struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
 	struct tree_desc tree_desc;
+	int ret;
 
 	opts.head_idx = -1;
 	opts.update = worktree;
@@ -667,10 +668,14 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
 		 */
 		/* fallthrough */
 	case 0:
-		return 0;
+		ret = 0;
+		break;
 	default:
-		return 128;
+		ret = 128;
 	}
+
+	unpack_trees_options_release(&opts);
+	return ret;
 }
 
 static void setup_branch_path(struct branch_info *branch)
diff --git a/builtin/clone.c b/builtin/clone.c
index 0df820c5970..df3bb9a7884 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -697,6 +697,7 @@ static int checkout(int submodule_progress)
 	init_tree_desc(&t, tree->buffer, tree->size);
 	if (unpack_trees(1, &t, &opts) < 0)
 		die(_("unable to checkout working tree"));
+	unpack_trees_options_release(&opts);
 
 	free(head);
 
diff --git a/builtin/commit.c b/builtin/commit.c
index 6cc7313bad8..84c79ecb5a5 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -305,6 +305,7 @@ static void create_base_index(const struct commit *current_head)
 	struct tree *tree;
 	struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
 	struct tree_desc t;
+	int exit_early = 0;
 
 	if (!current_head) {
 		discard_cache();
@@ -324,7 +325,10 @@ static void create_base_index(const struct commit *current_head)
 	parse_tree(tree);
 	init_tree_desc(&t, tree->buffer, tree->size);
 	if (unpack_trees(1, &t, &opts))
-		exit(128); /* We've already reported the error, finish dying */
+		exit_early = 1; /* We've already reported the error, finish dying */
+	unpack_trees_options_release(&opts);
+	if (exit_early)
+		exit(128);
 }
 
 static void refresh_cache_or_die(int refresh_flags)
diff --git a/builtin/merge.c b/builtin/merge.c
index 73290a07fcc..28089e2c5ed 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -671,6 +671,7 @@ static int read_tree_trivial(struct object_id *common, struct object_id *head,
 	struct tree *trees[MAX_UNPACK_TREES];
 	struct tree_desc t[MAX_UNPACK_TREES];
 	struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
+	int ret = 0;
 
 	opts.head_idx = 2;
 	opts.src_index = &the_index;
@@ -695,8 +696,9 @@ static int read_tree_trivial(struct object_id *common, struct object_id *head,
 		init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
 	}
 	if (unpack_trees(nr_trees, t, &opts))
-		return -1;
-	return 0;
+		ret = -1;
+	unpack_trees_options_release(&opts);
+	return ret;
 }
 
 static void write_tree_trivial(struct object_id *oid)
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 06f3b97ac05..8f1b8a7e74c 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -154,6 +154,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
 		OPT__QUIET(&opts.quiet, N_("suppress feedback messages")),
 		OPT_END()
 	};
+	int ret = 0;
 
 	opts.head_idx = -1;
 	opts.src_index = &the_index;
@@ -243,11 +244,13 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
 		parse_tree(tree);
 		init_tree_desc(t+i, tree->buffer, tree->size);
 	}
-	if (unpack_trees(nr_trees, t, &opts))
-		return 128;
+	if (unpack_trees(nr_trees, t, &opts)) {
+		ret = 128;
+		goto cleanup;
+	}
 
 	if (opts.debug_unpack || opts.dry_run)
-		return 0; /* do not write the index out */
+		goto cleanup; /* do not write the index out */
 
 	/*
 	 * When reading only one tree (either the most basic form,
@@ -262,5 +265,8 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
 
 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
 		die("unable to write new index file");
-	return 0;
+
+cleanup:
+	unpack_trees_options_release(&opts);
+	return ret;
 }
diff --git a/builtin/reset.c b/builtin/reset.c
index 86c604b21e9..713d084c3eb 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -78,10 +78,14 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
 
 	if (reset_type == KEEP) {
 		struct object_id head_oid;
-		if (get_oid("HEAD", &head_oid))
-			return error(_("You do not have a valid HEAD."));
-		if (!fill_tree_descriptor(the_repository, desc + nr, &head_oid))
-			return error(_("Failed to find tree of HEAD."));
+		if (get_oid("HEAD", &head_oid)) {
+			error(_("You do not have a valid HEAD."));
+			goto out;
+		}
+		if (!fill_tree_descriptor(the_repository, desc + nr, &head_oid)) {
+			error(_("Failed to find tree of HEAD."));
+			goto out;
+		}
 		nr++;
 		opts.fn = twoway_merge;
 	}
@@ -103,6 +107,7 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
 	ret = 0;
 
 out:
+	unpack_trees_options_release(&opts);
 	for (i = 0; i < nr; i++)
 		free((void *)desc[i].buffer);
 	return ret;
diff --git a/builtin/stash.c b/builtin/stash.c
index 1137e5fcbe8..be6ecb1ae11 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -237,6 +237,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
 	struct tree_desc t[MAX_UNPACK_TREES];
 	struct tree *tree;
 	struct lock_file lock_file = LOCK_INIT;
+	int ret = 0;
 
 	read_cache_preload(NULL);
 	if (refresh_cache(REFRESH_QUIET))
@@ -258,13 +259,17 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
 	opts.update = update;
 	opts.fn = oneway_merge;
 
-	if (unpack_trees(nr_trees, t, &opts))
-		return -1;
+	if (unpack_trees(nr_trees, t, &opts)) {
+		ret = -1;
+		goto cleanup;
+	}
 
 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
-		return error(_("unable to write new index file"));
+		ret = error(_("unable to write new index file"));
 
-	return 0;
+cleanup:
+	unpack_trees_options_release(&opts);
+	return ret;
 }
 
 static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)
@@ -815,6 +820,7 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op
 
 	if (unpack_trees(ARRAY_SIZE(tree_desc), tree_desc, &unpack_tree_opt))
 		die(_("failed to unpack trees"));
+	unpack_trees_options_release(&unpack_tree_opt);
 
 	do_diff_cache(&info->b_commit, diff_opt);
 }
diff --git a/diff-lib.c b/diff-lib.c
index 8a08d9af4eb..2d8a90a51b2 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -527,6 +527,7 @@ static int diff_cache(struct rev_info *revs,
 	struct tree *tree;
 	struct tree_desc t;
 	struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
+	int ret;
 
 	tree = parse_tree_indirect(tree_oid);
 	if (!tree)
@@ -545,7 +546,9 @@ static int diff_cache(struct rev_info *revs,
 	opts.pathspec->recursive = 1;
 
 	init_tree_desc(&t, tree->buffer, tree->size);
-	return unpack_trees(1, &t, &opts);
+	ret = unpack_trees(1, &t, &opts);
+	unpack_trees_options_release(&opts);
+	return ret;
 }
 
 void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)

-- 
2.33.0.1404.g83021034c5d
Previous: Elijah NewrenNext: Elijah Newren
Message 28 of 31 in “unpack-trees & dir APIs: fix memory leaks”
  1. 00/10 unpack-trees & dir APIs: fix memory leaksÆvar Arnfjörð Bjarmason, Oct 4, 2021
  2. 02/10 merge-recursive.c: call a new unpack_trees_options_init() functionÆvar Arnfjörð Bjarmason, Oct 4, 2021
  3. Elijah NewrenOct 4, 2021
  4. Ævar Arnfjörð BjarmasonOct 4, 2021
  5. Elijah NewrenOct 4, 2021
  6. 04/10 unpack-trees API: don't have clear_unpack_trees_porcelain() resetÆvar Arnfjörð Bjarmason, Oct 4, 2021
  7. Phillip WoodOct 4, 2021
  8. Ævar Arnfjörð BjarmasonOct 4, 2021
  9. Elijah NewrenOct 4, 2021
  10. Ævar Arnfjörð BjarmasonOct 4, 2021
  11. Elijah NewrenOct 4, 2021
  12. 09/10 merge.c: avoid duplicate unpack_trees_options_release() codeÆvar Arnfjörð Bjarmason, Oct 4, 2021
  13. Elijah NewrenOct 4, 2021
  14. Ævar Arnfjörð BjarmasonOct 4, 2021
  15. 03/10 unpack-trees.[ch]: embed "dir" in "struct unpack_trees_options"Ævar Arnfjörð Bjarmason, Oct 4, 2021
  16. Elijah NewrenOct 4, 2021
  17. 01/10 unpack-trees.[ch]: define and use a UNPACK_TREES_OPTIONS_INITÆvar Arnfjörð Bjarmason, Oct 4, 2021
  18. 05/10 dir.[ch]: make DIR_INIT mandatoryÆvar Arnfjörð Bjarmason, Oct 4, 2021
  19. Elijah NewrenOct 4, 2021
  20. 06/10 dir.c: get rid of lazy initializationÆvar Arnfjörð Bjarmason, Oct 4, 2021
  21. Elijah NewrenOct 4, 2021
  22. 07/10 unpack-trees API: rename clear_unpack_trees_porcelain()Ævar Arnfjörð Bjarmason, Oct 4, 2021
  23. Phillip WoodOct 4, 2021
  24. Ævar Arnfjörð BjarmasonOct 4, 2021
  25. Elijah NewrenOct 4, 2021
  26. 08/10 unpack-trees: don't leak memory in verify_clean_subdirectory()Ævar Arnfjörð Bjarmason, Oct 4, 2021
  27. Elijah NewrenOct 4, 2021
  28. 10/10 built-ins: plug memory leaks with unpack_trees_options_release()Ævar Arnfjörð Bjarmason, Oct 4, 2021
  29. Elijah NewrenOct 4, 2021
  30. Ævar Arnfjörð BjarmasonOct 4, 2021
  31. Elijah NewrenOct 4, 2021

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.