From a9a746364bd26d333c7229c6f7e851b507cd284a Mon Sep 17 00:00:00 2001 From: Nicolas Pitre Date: Wed, 24 Mar 2010 16:22:34 -0400 Subject: [PATCH 1/2] Make xmalloc and xrealloc thread-safe By providing a hook for the routine responsible for trying to free some memory on malloc failure, we can ensure that the called routine is protected by the appropriate locks when threads are in play. The obvious offender here was pack-objects which was calling xmalloc() within threads while release_pack_memory() is not thread safe. Signed-off-by: Nicolas Pitre Signed-off-by: Junio C Hamano --- builtin-pack-objects.c | 9 +++++++++ git-compat-util.h | 2 ++ wrapper.c | 20 ++++++++++++++++---- 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c index 539e75d56f..0ecc198422 100644 --- a/builtin-pack-objects.c +++ b/builtin-pack-objects.c @@ -1549,6 +1549,13 @@ static void find_deltas(struct object_entry **list, unsigned *list_size, #ifndef NO_PTHREADS +static void try_to_free_from_threads(size_t size) +{ + read_lock(); + release_pack_memory(size, -1); + read_unlock(); +} + /* * The main thread waits on the condition that (at least) one of the workers * has stopped working (which is indicated in the .working member of @@ -1583,10 +1590,12 @@ static void init_threaded_search(void) pthread_mutex_init(&cache_mutex, NULL); pthread_mutex_init(&progress_mutex, NULL); pthread_cond_init(&progress_cond, NULL); + set_try_to_free_routine(try_to_free_from_threads); } static void cleanup_threaded_search(void) { + set_try_to_free_routine(NULL); pthread_cond_destroy(&progress_cond); pthread_mutex_destroy(&read_mutex); pthread_mutex_destroy(&cache_mutex); diff --git a/git-compat-util.h b/git-compat-util.h index a3c4537366..1c171db8b1 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -346,6 +346,8 @@ static inline char *gitstrchrnul(const char *s, int c) extern void release_pack_memory(size_t, int); +extern void set_try_to_free_routine(void (*routine)(size_t)); + extern char *xstrdup(const char *str); extern void *xmalloc(size_t size); extern void *xmallocz(size_t size); diff --git a/wrapper.c b/wrapper.c index 9c71b21242..62edb57338 100644 --- a/wrapper.c +++ b/wrapper.c @@ -3,11 +3,23 @@ */ #include "cache.h" +static void try_to_free_builtin(size_t size) +{ + release_pack_memory(size, -1); +} + +static void (*try_to_free_routine)(size_t size) = try_to_free_builtin; + +void set_try_to_free_routine(void (*routine)(size_t)) +{ + try_to_free_routine = (routine) ? routine : try_to_free_builtin; +} + char *xstrdup(const char *str) { char *ret = strdup(str); if (!ret) { - release_pack_memory(strlen(str) + 1, -1); + try_to_free_routine(strlen(str) + 1); ret = strdup(str); if (!ret) die("Out of memory, strdup failed"); @@ -21,7 +33,7 @@ void *xmalloc(size_t size) if (!ret && !size) ret = malloc(1); if (!ret) { - release_pack_memory(size, -1); + try_to_free_routine(size); ret = malloc(size); if (!ret && !size) ret = malloc(1); @@ -67,7 +79,7 @@ void *xrealloc(void *ptr, size_t size) if (!ret && !size) ret = realloc(ptr, 1); if (!ret) { - release_pack_memory(size, -1); + try_to_free_routine(size); ret = realloc(ptr, size); if (!ret && !size) ret = realloc(ptr, 1); @@ -83,7 +95,7 @@ void *xcalloc(size_t nmemb, size_t size) if (!ret && (!nmemb || !size)) ret = calloc(1, 1); if (!ret) { - release_pack_memory(nmemb * size, -1); + try_to_free_routine(nmemb * size); ret = calloc(nmemb, size); if (!ret && (!nmemb || !size)) ret = calloc(1, 1); From 937491944292fa3303b565b9bd8914c6b644ab13 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Thu, 8 Apr 2010 09:15:39 +0200 Subject: [PATCH 2/2] Thread-safe xmalloc and xrealloc needs a recursive mutex The mutex used to protect object access (read_mutex) may need to be acquired recursively. Introduce init_recursive_mutex() helper function in thread-utils.c that constructs a mutex with the PHREAD_MUTEX_RECURSIVE attribute. pthread_mutex_init() emulation on Win32 is already recursive as it is implemented on top of the CRITICAL_SECTION type, which is recursive. http://msdn.microsoft.com/en-us/library/ms682530%28VS.85%29.aspx Add do-nothing compatibility wrappers for pthread_mutexattr* functions. Initial-version-by: Fredrik Kuivinen Signed-off-by: Johannes Sixt Signed-off-by: Junio C Hamano --- builtin-grep.c | 2 +- builtin-pack-objects.c | 4 ++-- compat/win32/pthread.h | 8 +++++++- thread-utils.c | 16 ++++++++++++++++ thread-utils.h | 1 + 5 files changed, 27 insertions(+), 4 deletions(-) diff --git a/builtin-grep.c b/builtin-grep.c index 371db0aa9e..52137f4ae4 100644 --- a/builtin-grep.c +++ b/builtin-grep.c @@ -16,8 +16,8 @@ #include "quote.h" #ifndef NO_PTHREADS -#include "thread-utils.h" #include +#include "thread-utils.h" #endif static char const * const grep_usage[] = { diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c index 0ecc198422..26fc7cd5a6 100644 --- a/builtin-pack-objects.c +++ b/builtin-pack-objects.c @@ -18,8 +18,8 @@ #include "refs.h" #ifndef NO_PTHREADS -#include "thread-utils.h" #include +#include "thread-utils.h" #endif static const char pack_usage[] = @@ -1586,7 +1586,7 @@ static pthread_cond_t progress_cond; */ static void init_threaded_search(void) { - pthread_mutex_init(&read_mutex, NULL); + init_recursive_mutex(&read_mutex); pthread_mutex_init(&cache_mutex, NULL); pthread_mutex_init(&progress_mutex, NULL); pthread_cond_init(&progress_cond, NULL); diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h index c72f100f40..a45f8d66df 100644 --- a/compat/win32/pthread.h +++ b/compat/win32/pthread.h @@ -18,11 +18,17 @@ */ #define pthread_mutex_t CRITICAL_SECTION -#define pthread_mutex_init(a,b) InitializeCriticalSection((a)) +#define pthread_mutex_init(a,b) (InitializeCriticalSection((a)), 0) #define pthread_mutex_destroy(a) DeleteCriticalSection((a)) #define pthread_mutex_lock EnterCriticalSection #define pthread_mutex_unlock LeaveCriticalSection +typedef int pthread_mutexattr_t; +#define pthread_mutexattr_init(a) (*(a) = 0) +#define pthread_mutexattr_destroy(a) do {} while (0) +#define pthread_mutexattr_settype(a, t) 0 +#define PTHREAD_MUTEX_RECURSIVE 0 + /* * Implement simple condition variable for Windows threads, based on ACE * implementation. diff --git a/thread-utils.c b/thread-utils.c index 4f9c829c2d..589f838f82 100644 --- a/thread-utils.c +++ b/thread-utils.c @@ -1,4 +1,5 @@ #include "cache.h" +#include #if defined(hpux) || defined(__hpux) || defined(_hpux) # include @@ -43,3 +44,18 @@ int online_cpus(void) return 1; } + +int init_recursive_mutex(pthread_mutex_t *m) +{ + pthread_mutexattr_t a; + int ret; + + ret = pthread_mutexattr_init(&a); + if (!ret) { + ret = pthread_mutexattr_settype(&a, PTHREAD_MUTEX_RECURSIVE); + if (!ret) + ret = pthread_mutex_init(m, &a); + pthread_mutexattr_destroy(&a); + } + return ret; +} diff --git a/thread-utils.h b/thread-utils.h index cce4b77bd6..1727a03333 100644 --- a/thread-utils.h +++ b/thread-utils.h @@ -2,5 +2,6 @@ #define THREAD_COMPAT_H extern int online_cpus(void); +extern int init_recursive_mutex(pthread_mutex_t*); #endif /* THREAD_COMPAT_H */