[PATCH] malloc: add locking to thread cache

[Joern Engel <joern at purestorage dot com>]

With signals we can reenter the thread-cache.  Protect against that with
a lock.  Will almost never happen in practice, it took the company five
years to reproduce a similar race in the existing malloc.  But easy to
trigger with a targeted test.

JIRA: PURE-27597
---
 tpc/malloc2.13/malloc-machine.h |  3 +++
 tpc/malloc2.13/tcache.h         | 32 +++++++++++++++++++++++++++-----
 2 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/tpc/malloc2.13/malloc-machine.h b/tpc/malloc2.13/malloc-machine.h
index 07072f5d5e11..a03b78bf3c1a 100644
--- a/tpc/malloc2.13/malloc-machine.h
+++ b/tpc/malloc2.13/malloc-machine.h
@@ -65,6 +65,9 @@ static inline int mutex_lock(mutex_t *m) {
     }
   }
 }
+/* Returns 0 on success, 1 on error
+   XXX: This is the opposite of the Linux kernel's mutex_trylock
+   primitive, making it confusing and error-prone. */
 static inline int mutex_trylock(mutex_t *m) {
   int r;
 
diff --git a/tpc/malloc2.13/tcache.h b/tpc/malloc2.13/tcache.h
index e267ce905ed0..ece03fc464cd 100644
--- a/tpc/malloc2.13/tcache.h
+++ b/tpc/malloc2.13/tcache.h
@@ -90,6 +90,13 @@ static inline int get_bit(unsigned long *bitmap, int i)
 }
 
 struct thread_cache {
+	/*
+	 * Since the cache is per-thread, it should not need a lock.
+	 * The reason we have one anyway is signal handlers, which can
+	 * cause the same thread to enter this code twice concurrently.
+	 */
+	mutex_t mutex;
+
 	/* Bytes in cache */
 	uint32_t tc_size;
 
@@ -264,9 +271,13 @@ static void *tcache_malloc(size_t size)
 		if (!cache)
 			return NULL;
 		memset(cache, 0, sizeof(*cache));
+		mutex_init(&cache->mutex);
 		tsd_setspecific(cache_key, cache);
 	}
 
+	if (mutex_trylock(&cache->mutex))
+		return NULL;
+
 	bin_no = cache_bin(nb);
 	assert(bin_no < CACHE_NO_BINS);
 	set_accessed(cache, bin_no);
@@ -281,6 +292,7 @@ static void *tcache_malloc(size_t size)
 		}
 		if (cache_bin(chunksize(*bin)) != bin_no) {
 			malloc_printerr(check_action, "invalid tcache entry", victim);
+			mutex_unlock(&cache->mutex);
 			return NULL;
 		}
 		*bin = victim->fd;
@@ -288,6 +300,7 @@ static void *tcache_malloc(size_t size)
 		cache->tc_size -= chunksize(victim);
 		cache->tc_count--;
 		alloc_perturb(p, size);
+		mutex_unlock(&cache->mutex);
 		return p;
 	}
 
@@ -301,8 +314,10 @@ static void *tcache_malloc(size_t size)
 		tcache_gc(cache);
 
 	arena = arena_get(size);
-	if (!arena)
+	if (!arena) {
+		mutex_unlock(&cache->mutex);
 		return NULL;
+	}
 	free_atomic_list(arena);
 	/* TODO: _int_malloc does checked_request2size() again, which is silly */
 	victim = _int_malloc(arena, size);
@@ -335,6 +350,7 @@ static void *tcache_malloc(size_t size)
 	arena_unlock(arena);
 	assert(!victim || arena == arena_for_chunk(mem2chunk(victim)));
 	alloc_perturb(victim, size);
+	mutex_unlock(&cache->mutex);
 	return victim;
 }
 
@@ -346,12 +362,16 @@ static void tcache_free(mchunkptr p)
 	size_t size;
 	int bin_no;
 
+	size = chunksize(p);
+	if (size > MAX_CACHED_SIZE)
+		goto uncached_free;
+
 	tsd_getspecific(cache_key, cache);
 	if (!cache)
 		goto uncached_free;
-	size = chunksize(p);
-	if (size > MAX_CACHED_SIZE)
+	if (mutex_trylock(&cache->mutex))
 		goto uncached_free;
+
 	bin_no = cache_bin(size);
 	assert(bin_no < CACHE_NO_BINS);
 
@@ -360,16 +380,18 @@ static void tcache_free(mchunkptr p)
 	bin = &cache->tc_bin[bin_no];
 	if (*bin == p) {
 		malloc_printerr(check_action, "double free or corruption (tcache)", chunk2mem(p));
-		return;
+		goto out;
 	}
 	if (*bin && cache_bin(chunksize(*bin)) != bin_no) {
 		malloc_printerr(check_action, "invalid tcache entry", chunk2mem(p));
-		return;
+		goto out;
 	}
 	free_perturb(p, size - 2 * SIZE_SZ);
 	add_to_bin(bin, p);
 	if (cache->tc_size > CACHE_SIZE)
 		tcache_gc(cache);
+ out:
+	mutex_unlock(&cache->mutex);
 	return;
 
  uncached_free:
-- 
2.7.0.rc3