[PATCHv2 1/6] threads: check for failure to set thread-local value

[Eric Blake <eblake@xxxxxxxxxx>]

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.5

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list