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

Re: [PATCH 1/7] pack-write.c: plug a leak in stage_tmp_packfiles()

From
Derrick Stolee <derrickstolee@github.com>
Date
Apr 11, 2023, 13:23 UTC
Message-ID
<423b5c5c-ef86-198a-16f7-c836ed78928c@github.com>
In-Reply-To
<18be29c3988295cd58521f8cc4a729897df074c8.1681166596.git.me@ttaylorr.com>
On 4/10/2023 6:53 PM, Taylor Blau wrote:
> Instead, simply free() `rev_tmp_name` at the end of
> `stage_tmp_packfiles()`.
Show 6 quoted lines
> @@ -568,6 +568,8 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,
>  		rename_tmp_packfile(name_buffer, rev_tmp_name, "rev");
>  	if (mtimes_tmp_name)
>  		rename_tmp_packfile(name_buffer, mtimes_tmp_name, "mtimes");
> +
> +	free((char *)rev_tmp_name);

Just cut off from the context is a "if (rev_tmp_name)", so it might be good to group this into that block, since we have the condition, anyway.

But I was also thinking about how we like to use "const" as an indicator as "I am not responsible for free()ing this". And this comes from the public write_rev_file() method. Based on the API prototype, we could think that this string is held by a static strbuf (making the method not reentrant, but that happens sometimes in our methods). But generally, I wanted to inspect what it would take to make the API reflect the fact that it can return a "new" string.

But there are two issues:
 1. The actual logic is inside write_rev_file_order(), so that API
    needs to change, too.
 2. The "new" string is created only if the rev_name parameter is
    NULL, which is somewhat understandable but still requires
    inside knowledge about the implementation to make that choice.
 3. If we inspect the callers to these methods, only one caller
    passes a non-null name: builtin/index-pack.c. The rest pass NULL,
    including write_midx_reverse_index() (which then leaks the name).

The below diff includes my attempt to change the API to return a non-const string that must be freed by the callers.

Thanks, -Stolee

--- >8 ---
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index b17e79cd40f..6d2fa52f9c4 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1725,7 +1725,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 {
 	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;
@@ -1956,8 +1956,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 		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.c b/midx.c
index 9af3e5de889..85154bedd73 100644
--- a/midx.c
+++ b/midx.c
@@ -945,7 +945,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);
 
@@ -958,6 +958,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 f1714054951..73850c061d9 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -207,15 +207,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;
@@ -233,12 +233,13 @@ 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)
 {
+	char *ret_name;
 	struct hashfile *f;
 	int fd;
 
@@ -249,10 +250,11 @@ 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);
+			rev_name = ret_name = strbuf_detach(&tmp_file, NULL);
 		} else {
 			unlink(rev_name);
 			fd = xopen(rev_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
+			ret_name = xstrdup(rev_name);
 		}
 		f = hashfd(fd, rev_name);
 	} else if (flags & WRITE_REV_VERIFY) {
@@ -264,6 +266,7 @@ const char *write_rev_file_order(const char *rev_name,
 			} else
 				die_errno(_("could not stat: %s"), rev_name);
 		}
+		ret_name = xstrdup(rev_name);
 		f = hashfd_check(rev_name);
 	} else
 		return NULL;
@@ -280,7 +283,7 @@ const char *write_rev_file_order(const char *rev_name,
 			  CSUM_HASH_IN_STREAM | CSUM_CLOSE |
 			  ((flags & WRITE_IDX_VERIFY) ? 0 : CSUM_FSYNC));
 
-	return rev_name;
+	return ret_name;
 }
 
 static void write_mtimes_header(struct hashfile *f)
@@ -543,7 +546,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;
 	const char *mtimes_tmp_name = NULL;
 
 	if (adjust_shared_perm(pack_tmp_name))
@@ -564,8 +567,10 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,
 	}
 
 	rename_tmp_packfile(name_buffer, pack_tmp_name, "pack");
-	if (rev_tmp_name)
+	if (rev_tmp_name) {
 		rename_tmp_packfile(name_buffer, rev_tmp_name, "rev");
+		free(rev_tmp_name);
+	}
 	if (mtimes_tmp_name)
 		rename_tmp_packfile(name_buffer, mtimes_tmp_name, "mtimes");
 }
diff --git a/pack.h b/pack.h
index 3ab9e3f60c0..02bbdfb19cc 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
Previous: Taylor BlauNext: Taylor Blau
Message 3 of 29 in “pack-revindex: enable on-disk reverse indexes by default”
  1. 0/7 pack-revindex: enable on-disk reverse indexes by defaultTaylor Blau, Apr 10, 2023
  2. 1/7 pack-write.c: plug a leak in stage_tmp_packfiles()Taylor Blau, Apr 10, 2023
  3. Derrick StoleeApr 11, 2023
  4. Taylor BlauApr 11, 2023
  5. 2/7 t5325: mark as leak-freeTaylor Blau, Apr 10, 2023
  6. 3/7 pack-revindex: make `load_pack_revindex` take a repositoryTaylor Blau, Apr 10, 2023
  7. Derrick StoleeApr 11, 2023
  8. Taylor BlauApr 11, 2023
  9. Derrick StoleeApr 12, 2023
  10. 4/7 pack-revindex: introduce GIT_TEST_REV_INDEX_DIE_ON_DISKTaylor Blau, Apr 10, 2023
  11. 5/7 pack-revindex: introduce `pack.readReverseIndex`Taylor Blau, Apr 10, 2023
  12. 6/7 config: enable `pack.writeReverseIndex` by defaultTaylor Blau, Apr 10, 2023
  13. Junio C HamanoApr 13, 2023
  14. 7/7 t: invert `GIT_TEST_WRITE_REV_INDEX`Taylor Blau, Apr 10, 2023
  15. Derrick StoleeApr 11, 2023
  16. Taylor BlauApr 11, 2023
  17. Derrick StoleeApr 12, 2023
  18. Derrick StoleeApr 11, 2023
  19. Taylor BlauApr 11, 2023
  20. Derrick StoleeApr 12, 2023
  21. 0/7 pack-revindex: enable on-disk reverse indexes by defaultTaylor Blau, Apr 12, 2023
  22. 1/7 pack-write.c: plug a leak in stage_tmp_packfiles()Taylor Blau, Apr 12, 2023
  23. 2/7 t5325: mark as leak-freeTaylor Blau, Apr 12, 2023
  24. 3/7 pack-revindex: make `load_pack_revindex` take a repositoryTaylor Blau, Apr 12, 2023
  25. 4/7 pack-revindex: introduce GIT_TEST_REV_INDEX_DIE_ON_DISKTaylor Blau, Apr 12, 2023
  26. 5/7 pack-revindex: introduce `pack.readReverseIndex`Taylor Blau, Apr 12, 2023
  27. 7/7 t: invert `GIT_TEST_WRITE_REV_INDEX`Taylor Blau, Apr 12, 2023
  28. 6/7 config: enable `pack.writeReverseIndex` by defaultTaylor Blau, Apr 12, 2023
  29. Derrick StoleeApr 13, 2023

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.