From 8616a2d0cb57865540f1c00ac2e5385a6cc5d84e Mon Sep 17 00:00:00 2001 From: Jameson Miller Date: Mon, 2 Jul 2018 19:49:39 +0000 Subject: [PATCH] block alloc: add validations around cache_entry lifecyle Add an option (controlled by an environment variable) perform extra validations on mem_pool allocated cache entries. When set: 1) Invalidate cache_entry memory when discarding cache_entry. 2) When discarding index_state struct, verify that all cache_entries were allocated from expected mem_pool. 3) When discarding mem_pools, invalidate mem_pool memory. This should provide extra checks that mem_pools and their allocated cache_entries are being used as expected. Signed-off-by: Jameson Miller Signed-off-by: Junio C Hamano --- cache.h | 6 ++++++ git.c | 3 +++ mem-pool.c | 6 +++++- mem-pool.h | 2 +- read-cache.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 5 files changed, 68 insertions(+), 4 deletions(-) diff --git a/cache.h b/cache.h index c1c7e9e65f..a3334a71ca 100644 --- a/cache.h +++ b/cache.h @@ -380,6 +380,12 @@ struct cache_entry *make_empty_transient_cache_entry(size_t name_len); */ void discard_cache_entry(struct cache_entry *ce); +/* + * Check configuration if we should perform extra validation on cache + * entries. + */ +int should_validate_cache_entries(void); + /* * Duplicate a cache_entry. Allocate memory for the new entry from a * memory_pool. Takes into account cache_entry fields that are meant diff --git a/git.c b/git.c index c2f48d53dd..010898ba6d 100644 --- a/git.c +++ b/git.c @@ -414,7 +414,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) trace_argv_printf(argv, "trace: built-in: git"); + validate_cache_entries(&the_index); status = p->fn(argc, argv, prefix); + validate_cache_entries(&the_index); + if (status) return status; diff --git a/mem-pool.c b/mem-pool.c index 139617cb23..a2841a4a9a 100644 --- a/mem-pool.c +++ b/mem-pool.c @@ -50,7 +50,7 @@ void mem_pool_init(struct mem_pool **mem_pool, size_t initial_size) *mem_pool = pool; } -void mem_pool_discard(struct mem_pool *mem_pool) +void mem_pool_discard(struct mem_pool *mem_pool, int invalidate_memory) { struct mp_block *block, *block_to_free; @@ -59,6 +59,10 @@ void mem_pool_discard(struct mem_pool *mem_pool) { block_to_free = block; block = block->next_block; + + if (invalidate_memory) + memset(block_to_free->space, 0xDD, ((char *)block_to_free->end) - ((char *)block_to_free->space)); + free(block_to_free); } diff --git a/mem-pool.h b/mem-pool.h index adeefdcb28..999d3c3a52 100644 --- a/mem-pool.h +++ b/mem-pool.h @@ -29,7 +29,7 @@ void mem_pool_init(struct mem_pool **mem_pool, size_t initial_size); /* * Discard a memory pool and free all the memory it is responsible for. */ -void mem_pool_discard(struct mem_pool *mem_pool); +void mem_pool_discard(struct mem_pool *mem_pool, int invalidate_memory); /* * Alloc memory from the mem_pool. diff --git a/read-cache.c b/read-cache.c index b07369660b..fd67e2e8a4 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2050,8 +2050,10 @@ int discard_index(struct index_state *istate) * Cache entries in istate->cache[] should have been allocated * from the memory pool associated with this index, or from an * associated split_index. There is no need to free individual - * cache entries. + * cache entries. validate_cache_entries can detect when this + * assertion does not hold. */ + validate_cache_entries(istate); resolve_undo_clear_index(istate); istate->cache_nr = 0; @@ -2068,13 +2070,45 @@ int discard_index(struct index_state *istate) istate->untracked = NULL; if (istate->ce_mem_pool) { - mem_pool_discard(istate->ce_mem_pool); + mem_pool_discard(istate->ce_mem_pool, should_validate_cache_entries()); istate->ce_mem_pool = NULL; } return 0; } +/* + * Validate the cache entries of this index. + * All cache entries associated with this index + * should have been allocated by the memory pool + * associated with this index, or by a referenced + * split index. + */ +void validate_cache_entries(const struct index_state *istate) +{ + int i; + + if (!should_validate_cache_entries() ||!istate || !istate->initialized) + return; + + for (i = 0; i < istate->cache_nr; i++) { + if (!istate) { + die("internal error: cache entry is not allocated from expected memory pool"); + } else if (!istate->ce_mem_pool || + !mem_pool_contains(istate->ce_mem_pool, istate->cache[i])) { + if (!istate->split_index || + !istate->split_index->base || + !istate->split_index->base->ce_mem_pool || + !mem_pool_contains(istate->split_index->base->ce_mem_pool, istate->cache[i])) { + die("internal error: cache entry is not allocated from expected memory pool"); + } + } + } + + if (istate->split_index) + validate_cache_entries(istate->split_index->base); +} + int unmerged_index(const struct index_state *istate) { int i; @@ -2878,8 +2912,25 @@ struct cache_entry *dup_cache_entry(const struct cache_entry *ce, void discard_cache_entry(struct cache_entry *ce) { + if (ce && should_validate_cache_entries()) + memset(ce, 0xCD, cache_entry_size(ce->ce_namelen)); + if (ce && ce->mem_pool_allocated) return; free(ce); } + +int should_validate_cache_entries(void) +{ + static int validate_index_cache_entries = -1; + + if (validate_index_cache_entries < 0) { + if (getenv("GIT_TEST_VALIDATE_INDEX_CACHE_ENTRIES")) + validate_index_cache_entries = 1; + else + validate_index_cache_entries = 0; + } + + return validate_index_cache_entries; +}