From 9529cfc0de18f2f77024a3ab7889be6b2539f3e6 Mon Sep 17 00:00:00 2001
From: William Brown <william@blackhats.net.au>
Date: Jul 15 2019 23:08:45 +0000
Subject: Ticket 50493 - connection_is_free to trylock


Bug Description: Due to the nature of the connection table
being single threaded, in connection_is_free, we would iterate
over the CT attempting to lock and check connection free states.
However, because this required the lock, if the connection was
currently in io, or other operations, the ct would delay behind
the c_mutex until it was released, then we would check the free
state.

Fix Description: Change the connection_is_free to use trylock
instead of lock - this means if the connection is locked it's
probably inuse and we can skip over it directly. We also change the
fn to iterate over the ct twice to check for possible connections
incase something frees up.

https://pagure.io/389-ds-base/pull-request/50493

Author: William Brown <william@blackhats.net.au>

Review by: tbordaz (Thanks!)

---

diff --git a/ldap/servers/slapd/connection.c b/ldap/servers/slapd/connection.c
index 6136d72..e6ce0f0 100644
--- a/ldap/servers/slapd/connection.c
+++ b/ldap/servers/slapd/connection.c
@@ -749,10 +749,13 @@ connection_acquire_nolock(Connection *conn)
 int
 connection_is_free(Connection *conn, int use_lock)
 {
-    int rc;
+    int rc = 0;
 
     if (use_lock) {
-        pthread_mutex_lock(&(conn->c_mutex));
+        /* If the lock is held, someone owns this! */
+        if (pthread_mutex_trylock(&(conn->c_mutex)) != 0) {
+            return 0;
+        }
     }
     rc = conn->c_sd == SLAPD_INVALID_SOCKET && conn->c_refcnt == 0 &&
          !(conn->c_flags & CONN_FLAG_CLOSING);
diff --git a/ldap/servers/slapd/conntable.c b/ldap/servers/slapd/conntable.c
index a513486..cadcfa9 100644
--- a/ldap/servers/slapd/conntable.c
+++ b/ldap/servers/slapd/conntable.c
@@ -115,12 +115,24 @@ Connection *
 connection_table_get_connection(Connection_Table *ct, int sd)
 {
     Connection *c = NULL;
-    int index, count;
+    size_t index = 0;
+    size_t count = 0;
 
     PR_Lock(ct->table_mutex);
 
+    /*
+     * We attempt to loop over the ct twice, because connection_is_free uses trylock
+     * and some resources *might* free in the time needed to loop around.
+     */
+    size_t ct_max_loops = ct->size * 2;
+
+    /*
+     * This uses sd as entropy to randomly start inside the ct rather than
+     * always head-loading the list. Not my idea, but it's probably okay ...
+     */
     index = sd % ct->size;
-    for (count = 0; count < ct->size; count++, index = (index + 1) % ct->size) {
+
+    for (count = 0; count < ct_max_loops; count++, index = (index + 1) % ct->size) {
         /* Do not use slot 0, slot 0 is head of the list of active connections */
         if (index == 0) {
             continue;
@@ -132,7 +144,8 @@ connection_table_get_connection(Connection_Table *ct, int sd)
         }
     }
 
-    if (count < ct->size) {
+    /* If count exceeds max loops, we didn't find something into index. */
+    if (count < ct_max_loops) {
         /* Found an available Connection */
         c = &(ct->c[index]);
         PR_ASSERT(c->c_next == NULL);