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 <jamill@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/cache.h b/cache.h
index c1c7e9e65f2c827105f3b4976fc8bc21c8ec9225..a3334a71cafa519e2e6f2edf915d78d7e23d6377 100644 (file)
--- 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);
 
  */
 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
 /*
  * 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 c2f48d53dd4aaba0752aa5f2e6633242d320b248..010898ba6d15e5ed051288ee6c46bf1ed2132bd8 100644 (file)
--- 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");
 
 
        trace_argv_printf(argv, "trace: built-in: git");
 

+       validate_cache_entries(&the_index);

        status = p->fn(argc, argv, prefix);
        status = p->fn(argc, argv, prefix);

+       validate_cache_entries(&the_index);
+

        if (status)
                return status;
 
        if (status)
                return status;
 

diff --git a/mem-pool.c b/mem-pool.c
index 139617cb233720964b33d9b39b87bf39cc2f3705..a2841a4a9ad7e4871282031f1b18fd62f18e9638 100644 (file)
--- 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;
 }
 
        *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;
 
 {
        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;
        {
                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);
        }
 
                free(block_to_free);
        }
 

diff --git a/mem-pool.h b/mem-pool.h
index adeefdcb2863f95f2a98c05593466b756b85b73d..999d3c3a52cc49b6dec896fc1e6b31ea8ac078d5 100644 (file)
--- 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.
  */
 /*
  * 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.
 
 /*
  * Alloc memory from the mem_pool.

diff --git a/read-cache.c b/read-cache.c
index b07369660baa64961c08ab7bac41e7d3f75c3710..fd67e2e8a48b4f28d923b4ba7354b02f203d8d7f 100644 (file)
--- 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 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;
 
        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) {
        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;
 }
 
                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;
 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)
 {
 
 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);
 }
        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;
+}