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

[PATCH 20/23] pack-write: fix return parameter of `write_rev_file_order()`

From
Patrick Steinhardt <ps@pks.im>
Date
Sep 30, 2024, 09:14 UTC
Message-ID
<2f3e17915d750c0ded8bf4f2f57106b81e9d8b3b.1727687410.git.ps@pks.im>
In-Reply-To
<cover.1727687410.git.ps@pks.im>

While the return parameter of `write_rev_file_order()` is a string constant, the function may indeed return an allocated string when its first parameter is a `NULL` pointer. This makes for a confusing calling convention, where callers need to be aware of these intricate ownership rules and cast away the constness to free the string in some cases.

Adapt the function and its caller `write_rev_file()` to always return an allocated string and adapt callers to always free the return value.

Note that this requires us to also adapt `rename_tmp_packfile()`, which compares the pointers to packfile data with each other. Now that the path of the reverse index file gets allocated unconditionally the check will always fail. This is fixed by using strcmp(3P) instead, which also feels way less fragile.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/index-pack.c              |  7 +++---
 midx-write.c                      |  3 ++-
 pack-write.c                      | 42 +++++++++++++++++--------------
 pack.h                            |  4 +--
 t/t5327-multi-pack-bitmaps-rev.sh |  1 +
 5 files changed, 31 insertions(+), 26 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index e228c56ff2..9d23b41b3a 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1505,7 +1505,7 @@ static void rename_tmp_packfile(const char **final_name,
 				struct strbuf *name, unsigned char *hash,
 				const char *ext, int make_read_only_if_same)
 {
-	if (*final_name != curr_name) {
+	if (!*final_name || strcmp(*final_name, curr_name)) {
 		if (!*final_name)
 			*final_name = odb_pack_name(name, hash, ext);
 		if (finalize_object_file(curr_name, *final_name))
@@ -1726,7 +1726,7 @@ int cmd_index_pack(int argc,
 {
 	int i, fix_thin_pack = 0, verify = 0, stat_only = 0, rev_index;
 	const char *curr_index;
-	const char *curr_rev_index = NULL;
+	char *curr_rev_index = NULL;
 	const char *index_name = NULL, *pack_name = NULL, *rev_index_name = NULL;
 	const char *keep_msg = NULL;
 	const char *promisor_msg = NULL;
@@ -1968,8 +1968,7 @@ int cmd_index_pack(int argc,
 		free((void *) curr_pack);
 	if (!index_name)
 		free((void *) curr_index);
-	if (!rev_index_name)
-		free((void *) curr_rev_index);
+	free(curr_rev_index);
 
 	/*
 	 * Let the caller know this pack is not self contained
diff --git a/midx-write.c b/midx-write.c
index 625c7d3137..b3a5f6c516 100644
--- a/midx-write.c
+++ b/midx-write.c
@@ -649,7 +649,7 @@ static void write_midx_reverse_index(char *midx_name, unsigned char *midx_hash,
 				     struct write_midx_context *ctx)
 {
 	struct strbuf buf = STRBUF_INIT;
-	const char *tmp_file;
+	char *tmp_file;
 
 	trace2_region_enter("midx", "write_midx_reverse_index", the_repository);
 
@@ -662,6 +662,7 @@ static void write_midx_reverse_index(char *midx_name, unsigned char *midx_hash,
 		die(_("cannot store reverse index file"));
 
 	strbuf_release(&buf);
+	free(tmp_file);
 
 	trace2_region_leave("midx", "write_midx_reverse_index", the_repository);
 }
diff --git a/pack-write.c b/pack-write.c
index 27965672f1..9961149e65 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -212,15 +212,15 @@ static void write_rev_trailer(struct hashfile *f, const unsigned char *hash)
 	hashwrite(f, hash, the_hash_algo->rawsz);
 }
 
-const char *write_rev_file(const char *rev_name,
-			   struct pack_idx_entry **objects,
-			   uint32_t nr_objects,
-			   const unsigned char *hash,
-			   unsigned flags)
+char *write_rev_file(const char *rev_name,
+		     struct pack_idx_entry **objects,
+		     uint32_t nr_objects,
+		     const unsigned char *hash,
+		     unsigned flags)
 {
 	uint32_t *pack_order;
 	uint32_t i;
-	const char *ret;
+	char *ret;
 
 	if (!(flags & WRITE_REV) && !(flags & WRITE_REV_VERIFY))
 		return NULL;
@@ -238,13 +238,14 @@ const char *write_rev_file(const char *rev_name,
 	return ret;
 }
 
-const char *write_rev_file_order(const char *rev_name,
-				 uint32_t *pack_order,
-				 uint32_t nr_objects,
-				 const unsigned char *hash,
-				 unsigned flags)
+char *write_rev_file_order(const char *rev_name,
+			   uint32_t *pack_order,
+			   uint32_t nr_objects,
+			   const unsigned char *hash,
+			   unsigned flags)
 {
 	struct hashfile *f;
+	char *path;
 	int fd;
 
 	if ((flags & WRITE_REV) && (flags & WRITE_REV_VERIFY))
@@ -254,12 +255,13 @@ const char *write_rev_file_order(const char *rev_name,
 		if (!rev_name) {
 			struct strbuf tmp_file = STRBUF_INIT;
 			fd = odb_mkstemp(&tmp_file, "pack/tmp_rev_XXXXXX");
-			rev_name = strbuf_detach(&tmp_file, NULL);
+			path = strbuf_detach(&tmp_file, NULL);
 		} else {
 			unlink(rev_name);
 			fd = xopen(rev_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
+			path = xstrdup(rev_name);
 		}
-		f = hashfd(fd, rev_name);
+		f = hashfd(fd, path);
 	} else if (flags & WRITE_REV_VERIFY) {
 		struct stat statbuf;
 		if (stat(rev_name, &statbuf)) {
@@ -270,22 +272,24 @@ const char *write_rev_file_order(const char *rev_name,
 				die_errno(_("could not stat: %s"), rev_name);
 		}
 		f = hashfd_check(rev_name);
-	} else
+		path = xstrdup(rev_name);
+	} else {
 		return NULL;
+	}
 
 	write_rev_header(f);
 
 	write_rev_index_positions(f, pack_order, nr_objects);
 	write_rev_trailer(f, hash);
 
-	if (rev_name && adjust_shared_perm(rev_name) < 0)
-		die(_("failed to make %s readable"), rev_name);
+	if (adjust_shared_perm(path) < 0)
+		die(_("failed to make %s readable"), path);
 
 	finalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA,
 			  CSUM_HASH_IN_STREAM | CSUM_CLOSE |
 			  ((flags & WRITE_IDX_VERIFY) ? 0 : CSUM_FSYNC));
 
-	return rev_name;
+	return path;
 }
 
 static void write_mtimes_header(struct hashfile *f)
@@ -549,7 +553,7 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,
 			 unsigned char hash[],
 			 char **idx_tmp_name)
 {
-	const char *rev_tmp_name = NULL;
+	char *rev_tmp_name = NULL;
 	char *mtimes_tmp_name = NULL;
 
 	if (adjust_shared_perm(pack_tmp_name))
@@ -575,7 +579,7 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,
 	if (mtimes_tmp_name)
 		rename_tmp_packfile(name_buffer, mtimes_tmp_name, "mtimes");
 
-	free((char *)rev_tmp_name);
+	free(rev_tmp_name);
 	free(mtimes_tmp_name);
 }
 
diff --git a/pack.h b/pack.h
index 3ab9e3f60c..02bbdfb19c 100644
--- a/pack.h
+++ b/pack.h
@@ -96,8 +96,8 @@ struct ref;
 
 void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought);
 
-const char *write_rev_file(const char *rev_name, struct pack_idx_entry **objects, uint32_t nr_objects, const unsigned char *hash, unsigned flags);
-const char *write_rev_file_order(const char *rev_name, uint32_t *pack_order, uint32_t nr_objects, const unsigned char *hash, unsigned flags);
+char *write_rev_file(const char *rev_name, struct pack_idx_entry **objects, uint32_t nr_objects, const unsigned char *hash, unsigned flags);
+char *write_rev_file_order(const char *rev_name, uint32_t *pack_order, uint32_t nr_objects, const unsigned char *hash, unsigned flags);
 
 /*
  * The "hdr" output buffer should be at least this big, which will handle sizes
diff --git a/t/t5327-multi-pack-bitmaps-rev.sh b/t/t5327-multi-pack-bitmaps-rev.sh
index 9cac03a94b..994a8e6be4 100755
--- a/t/t5327-multi-pack-bitmaps-rev.sh
+++ b/t/t5327-multi-pack-bitmaps-rev.sh
@@ -2,6 +2,7 @@
 
 test_description='exercise basic multi-pack bitmap functionality (.rev files)'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "${TEST_DIRECTORY}/lib-bitmap.sh"
 

-- 
2.46.2.852.g229c0bf0e5.dirty
Previous: Patrick SteinhardtNext: Patrick Steinhardt
Message 30 of 33 in “Memory leak fixes (pt.8)”
  1. 00/23 Memory leak fixes (pt.8)Patrick Steinhardt, Sep 30, 2024
  2. 01/23 builtin/annotate: fix leaking args vectorPatrick Steinhardt, Sep 30, 2024
  3. 02/23 read-cache: fix leaking hash context in `do_write_index()`Patrick Steinhardt, Sep 30, 2024
  4. Derrick StoleeSep 30, 2024
  5. 03/23 scalar: fix leaking repositoriesPatrick Steinhardt, Sep 30, 2024
  6. 04/23 shell: fix leaking stringsPatrick Steinhardt, Sep 30, 2024
  7. 05/23 wt-status: fix leaking buffer with sparse directoriesPatrick Steinhardt, Sep 30, 2024
  8. 06/23 submodule: fix leaking submodule entry listPatrick Steinhardt, Sep 30, 2024
  9. 07/23 builtin/stash: fix leaking `pathspec_from_file`Patrick Steinhardt, Sep 30, 2024
  10. 08/23 builtin/pack-redundant: fix various memory leaksPatrick Steinhardt, Sep 30, 2024
  11. 09/23 builtin/clone: fix leaking repo state when cloning with bundle URIsPatrick Steinhardt, Sep 30, 2024
  12. 10/23 t/helper: fix leaking repository in partial-clone helperPatrick Steinhardt, Sep 30, 2024
  13. 11/23 builtin/revert: fix leaking `gpg_sign` and `strategy` configPatrick Steinhardt, Sep 30, 2024
  14. 12/23 diff: improve lifecycle management of diff queuesPatrick Steinhardt, Sep 30, 2024
  15. 13/23 line-log: fix several memory leaksPatrick Steinhardt, Sep 30, 2024
  16. 14/23 pseudo-merge: fix various memory leaksPatrick Steinhardt, Sep 30, 2024
  17. Taylor BlauSep 30, 2024
  18. 15/23 pseudo-merge: fix leaking strmap keysPatrick Steinhardt, Sep 30, 2024
  19. Taylor BlauSep 30, 2024
  20. Patrick SteinhardtOct 7, 2024
  21. Jeff KingOct 8, 2024
  22. Patrick SteinhardtOct 9, 2024
  23. 16/23 pack-bitmap-write: fix leaking OID arrayPatrick Steinhardt, Sep 30, 2024
  24. Taylor BlauSep 30, 2024
  25. 17/23 midx-write: fix leaking bufferPatrick Steinhardt, Sep 30, 2024
  26. Taylor BlauSep 30, 2024
  27. Patrick SteinhardtOct 7, 2024
  28. 18/23 revision: fix memory leaks when rewriting parentsPatrick Steinhardt, Sep 30, 2024
  29. 19/23 revision: fix leaking saved parentsPatrick Steinhardt, Sep 30, 2024
  30. 20/23 pack-write: fix return parameter of `write_rev_file_order()`Patrick Steinhardt, Sep 30, 2024
  31. 21/23 t/helper: fix leaks in proc-receive helperPatrick Steinhardt, Sep 30, 2024
  32. 22/23 remote: fix leaking push reportsPatrick Steinhardt, Sep 30, 2024
  33. 23/23 builtin/send-pack: fix leaking list of push optionsPatrick Steinhardt, Sep 30, 2024

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.