path: root/split-index.c
diff options
authorElijah Newren <>2020-08-15 17:37:56 (GMT)
committerJunio C Hamano <>2020-08-18 19:16:06 (GMT)
commit44c7e1a7e08c0863c4156869364cb5751a23784e (patch)
treea0e8f23bd18a49bfe9a92421dc5de8bb6df20afd /split-index.c
parenta762c8c1e1e5b6352f027db80be4ca1c0077403d (diff)
mem-pool: use more standard initialization and finalization
A typical memory type, such as strbuf, hashmap, or string_list can be stored on the stack or embedded within another structure. mem_pool cannot be, because of how mem_pool_init() and mem_pool_discard() are written. mem_pool_init() does essentially the following (simplified for purposes of explanation here): void mem_pool_init(struct mem_pool **pool...) { *pool = xcalloc(1, sizeof(*pool)); It seems weird to require that mem_pools can only be accessed through a pointer. It also seems slightly dangerous: unlike strbuf_release() or strbuf_reset() or string_list_clear(), all of which put the data structure into a state where it can be re-used after the call, mem_pool_discard(pool) will leave pool pointing at free'd memory. read-cache (and split-index) are the only current users of mem_pools, and they haven't fallen into a use-after-free mistake here, but it seems likely to be problematic for future users especially since several of the current callers of mem_pool_init() will only call it when the mem_pool* is not already allocated (i.e. is NULL). This type of mechanism also prevents finding synchronization points where one can free existing memory and then resume more operations. It would be natural at such points to run something like mem_pool_discard(pool...); and, if necessary, mem_pool_init(&pool...); and then carry on continuing to use the pool. However, this fails badly if several objects had a copy of the value of pool from before these commands; in such a case, those objects won't get the updated value of pool that mem_pool_init() overwrites pool with and they'll all instead be reading and writing from free'd memory. Modify mem_pool_init()/mem_pool_discard() to behave more like strbuf_init()/strbuf_release() or string_list_init()/string_list_clear() In particular: (1) make mem_pool_init() just take a mem_pool* and have it only worry about allocating struct mp_blocks, not the struct mem_pool itself, (2) make mem_pool_discard() free the memory that the pool was responsible for, but leave it in a state where it can be used to allocate more memory afterward (without the need to call mem_pool_init() again). Signed-off-by: Elijah Newren <> Signed-off-by: Junio C Hamano <>
Diffstat (limited to 'split-index.c')
1 files changed, 4 insertions, 2 deletions
diff --git a/split-index.c b/split-index.c
index e6154e4..c0e8ad6 100644
--- a/split-index.c
+++ b/split-index.c
@@ -79,8 +79,10 @@ void move_cache_to_base_index(struct index_state *istate)
if (si->base &&
si->base->ce_mem_pool) {
- if (!istate->ce_mem_pool)
- mem_pool_init(&istate->ce_mem_pool, 0);
+ if (!istate->ce_mem_pool) {
+ istate->ce_mem_pool = xmalloc(sizeof(struct mem_pool));
+ mem_pool_init(istate->ce_mem_pool, 0);
+ }
mem_pool_combine(istate->ce_mem_pool, istate->split_index->base->ce_mem_pool);