From 927cfaf467a1eb5d2a0fea3a262ea9b212440c91 Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@redhat.com>
Date: Tue, 20 Dec 2011 15:06:47 -0700
Subject: [PATCH] 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.
---
 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 5b8fd5b9ef..ea64887225 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 63f699b2fe..1c3382662b 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 e52f3a9dea..e5000ea7a6 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 380dc56bed..8205516b04 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;
 
-- 
GitLab