Assume mutex locking cannot fail

Locking and unlocking a non-recursive mutex is a simple memory
operation and should not fail on any reasonable platform with correct
usage.  A pthread mutex can return EDEADLK on lock or EPERM on unlock,
or EINVAL if the mutex is uninitialized, but all of these conditions
would reflect serious bugs in the calling code.

Change the k5_mutex_lock and k5_mutex_unlock wrappers to return void
and adjust all call sites.  Propagate this change through
k5_cc_mutex_lock and k5_cc_mutex_unlock as well.

2 files changed, 47 insertions, 59 deletions

diff --git a/src/util/support/errors.c b/src/util/support/errors.c
index 1c13a4aa63..d820873e28 100644
--- a/src/util/support/errors.c
+++ b/src/util/support/errors.c
@@ -108,9 +108,10 @@ k5_get_error(struct errinfo *ep, long code)
     if (code == ep->code && ep->msg != NULL)
         return oom_check(strdup(ep->msg));
 
-    if (initialize() || lock())
+    if (initialize())
         return oom_check(strdup(_("Kerberos library initialization failure")));
 
+    lock();
     if (fptr == NULL) {
         unlock();
 #ifdef HAVE_STRERROR_R
@@ -153,8 +154,7 @@ void
 k5_set_error_info_callout_fn(const char *(KRB5_CALLCONV *f)(long))
 {
     initialize();
-    if (lock() == 0) {
-        fptr = f;
-        unlock();
-    }
+    lock();
+    fptr = f;
+    unlock();
 }
diff --git a/src/util/support/threads.c b/src/util/support/threads.c
index 4370c0589d..a97789624e 100644
--- a/src/util/support/threads.c
+++ b/src/util/support/threads.c
@@ -180,35 +180,36 @@ static void thread_termination(void *);
 
 static void thread_termination (void *tptr)
 {
-    int err = k5_mutex_lock(&key_lock);
-    if (err == 0) {
-        int i, pass, none_found;
-        struct tsd_block *t = tptr;
-
-        /* Make multiple passes in case, for example, a libkrb5 cleanup
-           function wants to print out an error message, which causes
-           com_err to allocate a thread-specific buffer, after we just
-           freed up the old one.
-
-           Shouldn't actually happen, if we're careful, but check just in
-           case.  */
-
-        pass = 0;
-        none_found = 0;
-        while (pass < 4 && !none_found) {
-            none_found = 1;
-            for (i = 0; i < K5_KEY_MAX; i++) {
-                if (destructors_set[i] && destructors[i] && t->values[i]) {
-                    void *v = t->values[i];
-                    t->values[i] = 0;
-                    (*destructors[i])(v);
-                    none_found = 0;
-                }
+    int i, pass, none_found;
+    struct tsd_block *t = tptr;
+
+    k5_mutex_lock(&key_lock);
+
+    /*
+     * Make multiple passes in case, for example, a libkrb5 cleanup
+     * function wants to print out an error message, which causes
+     * com_err to allocate a thread-specific buffer, after we just
+     * freed up the old one.
+     *
+     * Shouldn't actually happen, if we're careful, but check just in
+     * case.
+     */
+
+    pass = 0;
+    none_found = 0;
+    while (pass < 4 && !none_found) {
+        none_found = 1;
+        for (i = 0; i < K5_KEY_MAX; i++) {
+            if (destructors_set[i] && destructors[i] && t->values[i]) {
+                void *v = t->values[i];
+                t->values[i] = 0;
+                (*destructors[i])(v);
+                none_found = 0;
             }
         }
-        free (t);
-        err = k5_mutex_unlock(&key_lock);
     }
+    free (t);
+    k5_mutex_unlock(&key_lock);
 
     /* remove thread from global linked list */
 }
@@ -328,7 +329,6 @@ int k5_key_register (k5_key_t keynum, void (*destructor)(void *))
     assert(destructors_set[keynum] == 0);
     destructors[keynum] = destructor;
     destructors_set[keynum] = 1;
-    err = 0;
 
 #elif defined(_WIN32)
 
@@ -338,17 +338,14 @@ int k5_key_register (k5_key_t keynum, void (*destructor)(void *))
     destructors_set[keynum] = 1;
     destructors[keynum] = destructor;
     LeaveCriticalSection(&key_lock);
-    err = 0;
 
 #else /* POSIX */
 
-    err = k5_mutex_lock(&key_lock);
-    if (err == 0) {
-        assert(destructors_set[keynum] == 0);
-        destructors_set[keynum] = 1;
-        destructors[keynum] = destructor;
-        err = k5_mutex_unlock(&key_lock);
-    }
+    k5_mutex_lock(&key_lock);
+    assert(destructors_set[keynum] == 0);
+    destructors_set[keynum] = 1;
+    destructors[keynum] = destructor;
+    k5_mutex_unlock(&key_lock);
 
 #endif
     return 0;
@@ -381,21 +378,12 @@ int k5_key_delete (k5_key_t keynum)
 
 #else /* POSIX */
 
-    {
-        int err;
-
-        /* XXX RESOURCE LEAK:
-
-           Need to destroy the allocated objects first!  */
-
-        err = k5_mutex_lock(&key_lock);
-        if (err == 0) {
-            assert(destructors_set[keynum] == 1);
-            destructors_set[keynum] = 0;
-            destructors[keynum] = NULL;
-            k5_mutex_unlock(&key_lock);
-        }
-    }
+    /* XXX RESOURCE LEAK: Need to destroy the allocated objects first!  */
+    k5_mutex_lock(&key_lock);
+    assert(destructors_set[keynum] == 1);
+    destructors_set[keynum] = 0;
+    destructors[keynum] = NULL;
+    k5_mutex_unlock(&key_lock);
 
 #endif
 
@@ -512,13 +500,13 @@ krb5int_mutex_free (k5_mutex_t *m)
 }
 
 /* Callable versions of the various macros.  */
-int KRB5_CALLCONV
+void KRB5_CALLCONV
 krb5int_mutex_lock (k5_mutex_t *m)
 {
-    return k5_mutex_lock (m);
+    k5_mutex_lock (m);
 }
-int KRB5_CALLCONV
+void KRB5_CALLCONV
 krb5int_mutex_unlock (k5_mutex_t *m)
 {
-    return k5_mutex_unlock (m);
+    k5_mutex_unlock (m);
 }