We had a memory leak on a very arcane OOM situation (unlikely to ever hit in practice, but who knows if libvirt.so would ever be linked into some other program that exhausts all thread-local storage keys?). I found it by code inspection, while analyzing a valgrind report generated by Alex Jia. * src/util/threads.h (virThreadLocalSet): Alter signature. * src/util/threads-pthread.c (virThreadHelper): Reduce allocation lifetime. (virThreadLocalSet): Detect failure. * src/util/threads-win32.c (virThreadLocalSet): Likewise. (virCondWait): Fix caller. * src/util/virterror.c (virLastErrorObject): Likewise. --- src/util/threads-pthread.c | 14 +++++++++++--- src/util/threads-win32.c | 9 ++++++--- src/util/threads.h | 2 +- src/util/virterror.c | 5 +++-- 4 files changed, 21 insertions(+), 9 deletions(-) diff --git a/src/util/threads-pthread.c b/src/util/threads-pthread.c index 5b8fd5b..ea64887 100644 --- a/src/util/threads-pthread.c +++ b/src/util/threads-pthread.c @@ -154,8 +154,11 @@ struct virThreadArgs { static void *virThreadHelper(void *data) { struct virThreadArgs *args = data; - args->func(args->opaque); + struct virThreadArgs local = *args; + + /* Free args early, rather than tying it up during the entire thread. */ VIR_FREE(args); + local.func(local.opaque); return NULL; } @@ -249,7 +252,12 @@ void *virThreadLocalGet(virThreadLocalPtr l) return pthread_getspecific(l->key); } -void virThreadLocalSet(virThreadLocalPtr l, void *val) +int virThreadLocalSet(virThreadLocalPtr l, void *val) { - pthread_setspecific(l->key, val); + int err = pthread_setspecific(l->key, val); + if (err) { + errno = err; + return -1; + } + return 0; } diff --git a/src/util/threads-win32.c b/src/util/threads-win32.c index 63f699b..1c33826 100644 --- a/src/util/threads-win32.c +++ b/src/util/threads-win32.c @@ -155,7 +155,10 @@ int virCondWait(virCondPtr c, virMutexPtr m) if (!event) { return -1; } - virThreadLocalSet(&virCondEvent, event); + if (virThreadLocalSet(&virCondEvent, event) < 0) { + CloseHandle(event); + return -1; + } } virMutexLock(&c->lock); @@ -376,7 +379,7 @@ void *virThreadLocalGet(virThreadLocalPtr l) return TlsGetValue(l->key); } -void virThreadLocalSet(virThreadLocalPtr l, void *val) +int virThreadLocalSet(virThreadLocalPtr l, void *val) { - TlsSetValue(l->key, val); + return TlsSetValue(l->key, val) == 0 ? -1 : 0; } diff --git a/src/util/threads.h b/src/util/threads.h index e52f3a9..e5000ea 100644 --- a/src/util/threads.h +++ b/src/util/threads.h @@ -104,7 +104,7 @@ typedef void (*virThreadLocalCleanup)(void *); int virThreadLocalInit(virThreadLocalPtr l, virThreadLocalCleanup c) ATTRIBUTE_RETURN_CHECK; void *virThreadLocalGet(virThreadLocalPtr l); -void virThreadLocalSet(virThreadLocalPtr l, void*); +int virThreadLocalSet(virThreadLocalPtr l, void*) ATTRIBUTE_RETURN_CHECK; # ifdef WIN32 # include "threads-win32.h" diff --git a/src/util/virterror.c b/src/util/virterror.c index 380dc56..8205516 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -267,7 +267,8 @@ virLastErrorObject(void) if (!err) { if (VIR_ALLOC(err) < 0) return NULL; - virThreadLocalSet(&virLastErr, err); + if (virThreadLocalSet(&virLastErr, err) < 0) + VIR_FREE(err); } return err; } @@ -612,7 +613,7 @@ virDispatchError(virConnectPtr conn) virErrorFunc handler = virErrorHandler; void *userData = virUserData; - /* Should never happen, but doesn't hurt to check */ + /* Can only happen on OOM. */ if (!err) return; -- 1.7.7.4