Skip to content

Commit 2335ebf

Browse files
newrenGit for Windows Build Agent
authored andcommitted
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 <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent 72b93c2 commit 2335ebf

File tree

4 files changed

+23
-23
lines changed

4 files changed

+23
-23
lines changed

mem-pool.c

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -33,21 +33,13 @@ static struct mp_block *mem_pool_alloc_block(struct mem_pool *mem_pool, size_t b
3333
return p;
3434
}
3535

36-
void mem_pool_init(struct mem_pool **mem_pool, size_t initial_size)
36+
void mem_pool_init(struct mem_pool *pool, size_t initial_size)
3737
{
38-
struct mem_pool *pool;
39-
40-
if (*mem_pool)
41-
return;
42-
43-
pool = xcalloc(1, sizeof(*pool));
44-
38+
memset(pool, 0, sizeof(*pool));
4539
pool->block_alloc = BLOCK_GROWTH_SIZE;
4640

4741
if (initial_size > 0)
4842
mem_pool_alloc_block(pool, initial_size, NULL);
49-
50-
*mem_pool = pool;
5143
}
5244

5345
void mem_pool_discard(struct mem_pool *mem_pool, int invalidate_memory)
@@ -66,7 +58,8 @@ void mem_pool_discard(struct mem_pool *mem_pool, int invalidate_memory)
6658
free(block_to_free);
6759
}
6860

69-
free(mem_pool);
61+
mem_pool->mp_block = NULL;
62+
mem_pool->pool_alloc = 0;
7063
}
7164

7265
void *mem_pool_alloc(struct mem_pool *mem_pool, size_t len)

mem-pool.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,10 @@ struct mem_pool {
2424
/*
2525
* Initialize mem_pool with specified initial size.
2626
*/
27-
void mem_pool_init(struct mem_pool **mem_pool, size_t initial_size);
27+
void mem_pool_init(struct mem_pool *pool, size_t initial_size);
2828

2929
/*
30-
* Discard a memory pool and free all the memory it is responsible for.
30+
* Discard all the memory the memory pool is responsible for.
3131
*/
3232
void mem_pool_discard(struct mem_pool *mem_pool, int invalidate_memory);
3333

read-cache.c

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -89,8 +89,10 @@ static struct mem_pool *find_mem_pool(struct index_state *istate)
8989
else
9090
pool_ptr = &istate->ce_mem_pool;
9191

92-
if (!*pool_ptr)
93-
mem_pool_init(pool_ptr, 0);
92+
if (!*pool_ptr) {
93+
*pool_ptr = xmalloc(sizeof(**pool_ptr));
94+
mem_pool_init(*pool_ptr, 0);
95+
}
9496

9597
return *pool_ptr;
9698
}
@@ -2020,11 +2022,12 @@ static unsigned long load_all_cache_entries(struct index_state *istate,
20202022
{
20212023
unsigned long consumed;
20222024

2025+
istate->ce_mem_pool = xmalloc(sizeof(*istate->ce_mem_pool));
20232026
if (istate->version == 4) {
2024-
mem_pool_init(&istate->ce_mem_pool,
2027+
mem_pool_init(istate->ce_mem_pool,
20252028
estimate_cache_size_from_compressed(istate->cache_nr));
20262029
} else {
2027-
mem_pool_init(&istate->ce_mem_pool,
2030+
mem_pool_init(istate->ce_mem_pool,
20282031
estimate_cache_size(mmap_size, istate->cache_nr));
20292032
}
20302033

@@ -2084,7 +2087,8 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con
20842087
if (istate->name_hash_initialized)
20852088
BUG("the name hash isn't thread safe");
20862089

2087-
mem_pool_init(&istate->ce_mem_pool, 0);
2090+
istate->ce_mem_pool = xmalloc(sizeof(*istate->ce_mem_pool));
2091+
mem_pool_init(istate->ce_mem_pool, 0);
20882092

20892093
/* ensure we have no more threads than we have blocks to process */
20902094
if (nr_threads > ieot->nr)
@@ -2111,11 +2115,12 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con
21112115
nr = 0;
21122116
for (j = p->ieot_start; j < p->ieot_start + p->ieot_blocks; j++)
21132117
nr += p->ieot->entries[j].nr;
2118+
istate->ce_mem_pool = xmalloc(sizeof(*istate->ce_mem_pool));
21142119
if (istate->version == 4) {
2115-
mem_pool_init(&p->ce_mem_pool,
2120+
mem_pool_init(p->ce_mem_pool,
21162121
estimate_cache_size_from_compressed(nr));
21172122
} else {
2118-
mem_pool_init(&p->ce_mem_pool,
2123+
mem_pool_init(p->ce_mem_pool,
21192124
estimate_cache_size(mmap_size, nr));
21202125
}
21212126

@@ -2372,7 +2377,7 @@ int discard_index(struct index_state *istate)
23722377

23732378
if (istate->ce_mem_pool) {
23742379
mem_pool_discard(istate->ce_mem_pool, should_validate_cache_entries());
2375-
istate->ce_mem_pool = NULL;
2380+
FREE_AND_NULL(istate->ce_mem_pool);
23762381
}
23772382

23782383
return 0;

split-index.c

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,8 +79,10 @@ void move_cache_to_base_index(struct index_state *istate)
7979
if (si->base &&
8080
si->base->ce_mem_pool) {
8181

82-
if (!istate->ce_mem_pool)
83-
mem_pool_init(&istate->ce_mem_pool, 0);
82+
if (!istate->ce_mem_pool) {
83+
istate->ce_mem_pool = xmalloc(sizeof(struct mem_pool));
84+
mem_pool_init(istate->ce_mem_pool, 0);
85+
}
8486

8587
mem_pool_combine(istate->ce_mem_pool, istate->split_index->base->ce_mem_pool);
8688
}

0 commit comments

Comments
 (0)