threads: check for failure to set thread-local value

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.

diff --git a/src/util/threads-pthread.c b/src/util/threads-pthread.c
index 5b8fd5b9ef9cad1c5e77706160b81681cb966b3e..ea6488722578462e66d519df93501e17d0291a80 100644 (file)
--- 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 63f699b2fe4a0375018d9c3689d01fd1365aaf58..1c3382662b435e1f8cee2ff9a0c1326532fa3b44 100644 (file)
--- 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 e52f3a9deaf81949b81463025eefebc60dc04b3b..e5000ea7a64dbfbd58006dac9951ae1af27ad7cd 100644 (file)
--- 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 380dc56bed95d47a5495c480b263db44cc2d25d1..8205516b0433c00fed675c04fcf054d167b41f0a 100644 (file)
--- 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;