read-cache: two small leak fixes

I found these two small memory leaks while investigating things involving the index. I thought they were small enough to send independently.

I initially wrote this as two patches because the second patch was more involved to work around the "cleanup" label, and my branch was based on 2.46.1. This code has changed recently due to d1c53f6703 (read-cache: fix leaking hashfile when writing index fails, 2024-08-14), so now the second patch is simpler and these could probably be merged into one.

Thanks, -Stolee

Derrick Stolee (2):
read-cache: free threaded memory pool
read-cache: free hash context in do_write_index()

 read-cache.c | 2 ++
 1 file changed, 2 insertions(+)

base-commit: 6258f68c3c1092c901337895c864073dcdea9213
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1801%2Fderrickstolee%2Fleaks-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1801/derrickstolee/leaks-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1801

gitgitgadget

From: Derrick Stolee <[email protected]>

In load_cache_entries_threaded(), each thread is allocated its own memory pool. This pool needs to be cleaned up while closing the threads down, or it will be leaked.

---
 read-cache.c | 1 +
 1 file changed, 1 insertion(+)

index 764fdfec465..3c078afadbc 100644
@@ -2188,6 +2188,7 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con
 		if (err)
 			die(_("unable to join load_cache_entries thread: %s"), strerror(err));
 		mem_pool_combine(istate->ce_mem_pool, p->ce_mem_pool);
+		free(p->ce_mem_pool);
 		consumed += p->consumed;
 	}
 

gitgitgadget

From: Derrick Stolee <[email protected]>

While writing an index, a 'git_hash_ctx' is allocated for hashing the file contents. This should be freed as the method exits.

---
 read-cache.c | 1 +
 1 file changed, 1 insertion(+)

index 3c078afadbc..51845c2e611 100644
@@ -3126,6 +3126,7 @@ out:
 		free_hashfile(f);
 	strbuf_release(&sb);
 	free(ieot);
+	free(eoie_c);
 	return ret;
 }
 

gitgitgadget

s/allocated/allocating/

Sorry for the collision. I had checked when prepping the GGG PR on Friday but forgot to check again before submitting. Your patch is better in substance and context.

Thanks,
-Stolee

Nothing to be sorry about, both series were sent out quite close to one another. Quite on the contrary, thanks for fixing this leak :)

I don't mind much which of these versions lands in the tree, and the resulting conflict is trivial to solve anyway.

Patrick

You're right that the wording is awkward but I'm not thrilled with the suggested alternative.

Perhaps "each thread allocates its own"

I took a look at what it would take to replace the pointer with an inline struct but found complications with situations such as the find_mem_pool() method. While we could replace some of the logic to recognize the new type, the existing logic seems to depend on using the NULL pointer as an indicator that the pool should be lazily initialized.

If we were to pull the struct inline, we would either need another boolean to indicate initialization or lose lazy initialization.

I'm leaning towards the simpler leak fix over the disruption of that change.

Thanks,
-Stolee

Sure, works for me :)

Fair enough, no complaint from my side. I thought it would've been easy, but didn't dive deep. So if you say it is harder than I made it out to be with my shallow understanding I'm going to trust your judgement. After all, the leak fix is a strict improvement by itself.

Patrick

From: Derrick Stolee <[email protected]>

In load_cache_entries_threaded(), each thread allocates its own memory pool. This pool needs to be cleaned up while closing the threads down, or it will be leaked.

This ce_mem_pool pointer could theoretically be converted to an inline copy of the struct, but the use of a pointer helps with existing lazy- initialization logic. Adjusting that behavior only to avoid this pointer would be a much bigger change.

---
read-cache: two small leak fixes

This v2 removes the duplicate patch and updates the commit message.

Thanks, -Stolee

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1801%2Fderrickstolee%2Fleaks-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1801/derrickstolee/leaks-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1801

Range-diff vs v1:

1: 9a45b15ea4b ! 1: 220a098c93c read-cache: free threaded memory pool
@@ Metadata
## Commit message ##
read-cache: free threaded memory pool

- In load_cache_entries_threaded(), each thread is allocated its own
- memory pool. This pool needs to be cleaned up while closing the threads
- down, or it will be leaked.
+ In load_cache_entries_threaded(), each thread allocates its own memory
+ pool. This pool needs to be cleaned up while closing the threads down,
+ or it will be leaked.
+
+ This ce_mem_pool pointer could theoretically be converted to an inline
+ copy of the struct, but the use of a pointer helps with existing lazy-
+ initialization logic. Adjusting that behavior only to avoid this pointer
+ would be a much bigger change.

2: b6fe5b3ef7e < -: ----------- read-cache: free hash context in do_write_index()

 read-cache.c | 1 +
 1 file changed, 1 insertion(+)

index 764fdfec465..3c078afadbc 100644
@@ -2188,6 +2188,7 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con
 		if (err)
 			die(_("unable to join load_cache_entries thread: %s"), strerror(err));
 		mem_pool_combine(istate->ce_mem_pool, p->ce_mem_pool);
+		free(p->ce_mem_pool);
 		consumed += p->consumed;
 	}
 

base-commit: 6258f68c3c1092c901337895c864073dcdea9213

gitgitgadget

Thanks, this looks good to me.

Patrick