Thread safety and locking checks.

7 files changed, 232 insertions, 125 deletions

diff --git a/daemon/bd.h b/daemon/bd.h
index a59b1e1..5199615 100644
--- a/daemon/bd.h
+++ b/daemon/bd.h
@@ -44,16 +44,18 @@ typedef void (*bd_escape_value)(const ha_request_t* rq, ha_buffer_t* buf,
  */
 typedef struct bd_context
 {
-    hsh_t* cache;            /* Some cached records or basic */
-
+    /* Readonly ---------------------------------------------*/
     bd_validate_digest  f_validate_digest;
     bd_validate_basic   f_validate_basic;
     bd_escape_value     f_escape_value;
+
+    /* Require locking --------------------------------------*/
+    hsh_t* cache;            /* Some cached records or basic */
 }
 bd_context_t;
 
-#define BD_CALLBACKS(a, b, c) { NULL, (a), (b), (c) }
-#define BD_DEFAULTS { NULL, NULL, NULL }
+#define BD_CALLBACKS(a, b, c) {(a), (b), (c), NULL }
+#define BD_DEFAULTS { NULL, NULL, NULL, NULL }
 
 /* ----------------------------------------------------------------------------------
  *  Base Handler Functions
diff --git a/daemon/ldap.c b/daemon/ldap.c
index 956c464..f04879a 100644
--- a/daemon/ldap.c
+++ b/daemon/ldap.c
@@ -49,7 +49,7 @@ typedef struct ldap_context
     /* Base Handler ------------------------------------------------------ */
     bd_context_t bd;
 
-    /* Settings ---------------------------------------------------------- */
+    /* Read Only --------------------------------------------------------- */
     const char* servers;      /* Servers to authenticate against (required) */
     const char* filter;       /* Filter (either this or dnmap must be set) */
     const char* base;         /* Base for the filter */
@@ -65,7 +65,7 @@ typedef struct ldap_context
     int ldap_max;             /* Number of open connections allowed */
     int ldap_timeout;         /* Maximum amount of time to dedicate to an ldap query */
 
-    /* Context ----------------------------------------------------------- */
+    /* Require Locking --------------------------------------------------- */
     LDAP** pool;            /* Pool of available connections */
     int pool_mark;          /* Amount of connections allocated */
 }
@@ -421,27 +421,40 @@ static int validate_ldap_ha1(const ha_request_t* rq, ldap_context_t* ctx, LDAP*
 static LDAP* get_ldap_connection(const ha_request_t* rq, ldap_context_t* ctx)
 {
     LDAP* ld;
-    int i, r;
+    int i, r, create = 1;
 
     ASSERT(ctx);
 
-    for(i = 0; i < ctx->ldap_max; i++)
-    {
-        /* An open connection in the pool */
-        if(ctx->pool[i])
+    /*
+     * Note that below there maybe a race condition between the two locks
+     * but this will only allow a few extra connections to open at best
+     * and as such really isn't a big issue.
+     */
+
+    ha_lock(NULL);
+
+        for(i = 0; i < ctx->ldap_max; i++)
         {
-            ha_messagex(rq, LOG_DEBUG, "using cached LDAP connection");
-            ld = ctx->pool[i];
-            ctx->pool[i] = NULL;
-            return ld;
+            /* An open connection in the pool */
+            if(ctx->pool[i])
+            {
+                ha_messagex(rq, LOG_DEBUG, "using cached LDAP connection");
+                ld = ctx->pool[i];
+                ctx->pool[i] = NULL;
+                break;
+            }
         }
-    }
 
-    if(ctx->pool_mark >= ctx->ldap_max)
-    {
-        ha_messagex(rq, LOG_ERR, "too many open LDAP connections");
-        return NULL;
-    }
+        if(ld == NULL && ctx->pool_mark >= ctx->ldap_max)
+        {
+            ha_messagex(rq, LOG_ERR, "too many open LDAP connections");
+            create = 0;
+        }
+
+    ha_unlock(NULL);
+
+    if(ld != NULL || create == 0)
+        return ld;
 
     ld = ldap_init(ctx->servers, ctx->port);
     if(!ld)
@@ -462,16 +475,26 @@ static LDAP* get_ldap_connection(const ha_request_t* rq, ldap_context_t* ctx)
         }
     }
 
-    ctx->pool_mark++;
-    ha_messagex(rq, LOG_DEBUG, "opened new connection (total %d)", ctx->pool_mark);
+    ha_lock(NULL);
+
+        ctx->pool_mark++;
+        ha_messagex(rq, LOG_DEBUG, "opened new connection (total %d)", ctx->pool_mark);
+
+    ha_unlock(NULL);
+
     return ld;
 }
 
 static void discard_ldap_connection(const ha_request_t* rq, ldap_context_t* ctx, LDAP* ld)
 {
     ldap_unbind_s(ld);
-    ctx->pool_mark--;
-    ha_messagex(rq, LOG_DEBUG, "discarding connection (total %d)", ctx->pool_mark);
+
+    ha_lock(NULL);
+
+        ctx->pool_mark--;
+        ha_messagex(rq, LOG_DEBUG, "discarding connection (total %d)", ctx->pool_mark);
+
+    ha_unlock(NULL);
 }
 
 static void save_ldap_connection(const ha_request_t* rq, ldap_context_t* ctx, LDAP* ld)
@@ -494,17 +517,21 @@ static void save_ldap_connection(const ha_request_t* rq, ldap_context_t* ctx, LD
         break;
 
     default:
-        for(i = 0; i < ctx->ldap_max; i++)
-        {
-            /* An open connection in the pool */
-            if(!ctx->pool[i])
+        ha_lock(NULL);
+
+            for(i = 0; i < ctx->ldap_max; i++)
             {
-                ha_messagex(rq, LOG_DEBUG, "caching ldap connection for later use");
-                ctx->pool[i] = ld;
-                ld = NULL;
-                break;
+                /* An open connection in the pool */
+                if(!ctx->pool[i])
+                {
+                    ha_messagex(rq, LOG_DEBUG, "caching ldap connection for later use");
+                    ctx->pool[i] = ld;
+                    ld = NULL;
+                    break;
+                }
             }
-        }
+
+        ha_unlock(NULL);
         break;
     };
 
diff --git a/daemon/mysql.c b/daemon/mysql.c
index 0ed8f35..1a75374 100644
--- a/daemon/mysql.c
+++ b/daemon/mysql.c
@@ -26,7 +26,7 @@ typedef struct mysql_context
     /* Base Handler ------------------------------------------------------ */
     bd_context_t bd;
 
-    /* Settings ---------------------------------------------------------- */
+    /* Readonly Settings ------------------------------------------------- */
     const char* host;         /* The connection host or path */
     unsigned int port;        /* The connection port */
     const char* user;         /* The pgsql user name */
@@ -41,7 +41,7 @@ typedef struct mysql_context
     int mysql_timeout;        /* Maximum amount of time to dedicate to a query */
     int use_unix_socket;
 
-    /* Context ----------------------------------------------------------- */
+    /* Require Locking --------------------------------------------------- */
     MYSQL** pool;            /* Pool of available connections */
     int pool_mark;          /* Amount of connections allocated */
 }
@@ -73,6 +73,11 @@ static const mysql_context_t mysql_defaults =
     0                   /* pool_mark */
 };
 
+/* mysql_real_connect is not always thread-safe :( */
+static pthread_mutex_t g_mysql_mutex;
+static pthread_mutexattr_t g_mysql_mutexattr;
+
+
 
 /* -------------------------------------------------------------------------------
  * Internal Functions
@@ -235,27 +240,40 @@ static MYSQL* get_mysql_connection(const ha_request_t* rq, mysql_context_t* ctx)
 {
     MYSQL* my;
     MYSQL* r;
-    int i;
+    int i, create = 1;
 
     ASSERT(ctx);
 
-    for(i = 0; i < ctx->mysql_max; i++)
-    {
-        /* An open connection in the pool */
-        if(ctx->pool[i])
+    /*
+     * Note that below there maybe a race condition between the two locks
+     * but this will only allow a few extra connections to open at best
+     * and as such really isn't a big issue.
+     */
+
+    ha_lock(&g_mysql_mutex);
+
+        for(i = 0; i < ctx->mysql_max; i++)
         {
-            ha_messagex(rq, LOG_DEBUG, "using cached mysql connection");
-            my = ctx->pool[i];
-            ctx->pool[i] = NULL;
-            return my;
+            /* An open connection in the pool */
+            if(ctx->pool[i])
+            {
+                ha_messagex(rq, LOG_DEBUG, "using cached mysql connection");
+                my = ctx->pool[i];
+                ctx->pool[i] = NULL;
+            }
         }
-    }
 
-    if(ctx->pool_mark >= ctx->mysql_max)
-    {
-        ha_messagex(rq, LOG_ERR, "too many open mysql connections");
-        return NULL;
-    }
+
+        if(my == NULL && ctx->pool_mark >= ctx->mysql_max)
+        {
+            ha_messagex(rq, LOG_ERR, "too many open mysql connections");
+            create = 0;
+        }
+
+    ha_unlock(&g_mysql_mutex);
+
+    if(my != NULL || create == 0)
+        return my;
 
     my = mysql_init(NULL);
     if(!my)
@@ -269,29 +287,39 @@ static MYSQL* get_mysql_connection(const ha_request_t* rq, mysql_context_t* ctx)
     mysql_options(my, MYSQL_OPT_WRITE_TIMEOUT, (char*)&(ctx->mysql_timeout)); */
 
     /* Apparently mysql_real_connect is not thread safe :( */
-    ha_lock(NULL);
+    ha_lock(&g_mysql_mutex);
+
         r = mysql_real_connect(my, ctx->use_unix_socket ? NULL : ctx->host,
                                ctx->user, ctx->password, ctx->database, ctx->port,
                                ctx->use_unix_socket ? ctx->host : NULL, 0);
-    ha_unlock(NULL);
 
-    if(!r)
-    {
-        ha_messagex(rq, LOG_ERR, "error opening mysql connection: %s", mysql_error(my));
-        mysql_close(my);
-        return NULL;
-    }
+        if(!r)
+        {
+            ha_messagex(rq, LOG_ERR, "error opening mysql connection: %s", mysql_error(my));
+            mysql_close(my);
+            my = NULL;
+        }
+        else
+        {
+            ctx->pool_mark++;
+            ha_messagex(rq, LOG_DEBUG, "opened new mysql connection (total %d)", ctx->pool_mark);
+        }
+
+    ha_unlock(&g_mysql_mutex);
 
-    ctx->pool_mark++;
-    ha_messagex(rq, LOG_DEBUG, "opened new mysql connection (total %d)", ctx->pool_mark);
     return my;
 }
 
 static void discard_mysql_connection(const ha_request_t* rq, mysql_context_t* ctx, MYSQL* my)
 {
     mysql_close(my);
-    ctx->pool_mark--;
-    ha_messagex(rq, LOG_DEBUG, "discarding mysql connection (total %d)", ctx->pool_mark);
+
+    ha_lock(&g_mysql_mutex);
+
+        ctx->pool_mark--;
+        ha_messagex(rq, LOG_DEBUG, "discarding mysql connection (total %d)", ctx->pool_mark);
+
+    ha_unlock(&g_mysql_mutex);
 }
 
 static void save_mysql_connection(const ha_request_t* rq, mysql_context_t* ctx, MYSQL* my)
@@ -322,17 +350,21 @@ static void save_mysql_connection(const ha_request_t* rq, mysql_context_t* ctx,
 
     /* Make sure it's worth saving */
     default:
-        for(i = 0; i < ctx->mysql_max; i++)
-        {
-            /* An open connection in the pool */
-            if(!ctx->pool[i])
+        ha_lock(&g_mysql_mutex);
+
+            for(i = 0; i < ctx->mysql_max; i++)
             {
-                ha_messagex(rq, LOG_DEBUG, "caching mysql connection for later use");
-                ctx->pool[i] = my;
-                my = NULL;
-                break;
+                /* An open connection in the pool */
+                if(!ctx->pool[i])
+                {
+                    ha_messagex(rq, LOG_DEBUG, "caching mysql connection for later use");
+                    ctx->pool[i] = my;
+                    my = NULL;
+                    break;
+                }
             }
-        }
+
+        ha_unlock(&g_mysql_mutex);
         break;
     };
 
@@ -701,6 +733,19 @@ int mysql_initialize(ha_context_t* context)
         ha_messagex(NULL, LOG_INFO, "initialized mysql handler");
     }
 
+    /* Global Initialization */
+    else
+    {
+        /* Create the smblib mutex */
+        if(pthread_mutexattr_init(&g_mysql_mutexattr) != 0 ||
+           pthread_mutexattr_settype(&g_mysql_mutexattr, HA_MUTEX_TYPE) ||
+           pthread_mutex_init(&g_mysql_mutex, &g_mysql_mutexattr) != 0)
+        {
+            ha_messagex(NULL, LOG_CRIT, "threading problem. can't create mutex");
+            return HA_CRITERROR;
+        }
+    }
+
     return HA_OK;
 }
 
@@ -727,6 +772,12 @@ void mysql_destroy(ha_context_t* context)
             free(ctx->pool);
         }
     }
+    else
+    {
+        /* Close the mutex */
+        pthread_mutex_destroy(&g_mysql_mutex);
+        pthread_mutexattr_destroy(&g_mysql_mutexattr);
+    }
 
     bd_destroy(context);
     ha_messagex(NULL, LOG_INFO, "uninitialized mysql handler");
diff --git a/daemon/ntlm.c b/daemon/ntlm.c
index 8e1aa20..fba4624 100644
--- a/daemon/ntlm.c
+++ b/daemon/ntlm.c
@@ -39,14 +39,14 @@ ntlm_connection_t;
 /* The main context */
 typedef struct ntlm_context
 {
-    /* Settings ---------------------------------------------------------- */
+    /* Read Only --------------------------------------------------------- */
     const char* server;             /* Server to authenticate against */
     const char* domain;             /* NTLM domain to authenticate against */
     const char* backup;             /* Backup server if primary is down */
     int pending_max;                    /* Maximum number of connections at once */
     int pending_timeout;            /* Timeout for authentication (in seconds) */
 
-    /* Context ----------------------------------------------------------- */
+    /* Require Locking --------------------------------------------------- */
     hsh_t* pending;                /* Pending connections */
     hsh_t* established;        /* Established connections */
 }
@@ -149,19 +149,19 @@ static int putpending(ntlm_context_t* ctx, const void* key, ntlm_connection_t* c
     ASSERT(ctx && key && conn);
     ASSERT(conn->handle);
 
-    if(!hsh_get(ctx->pending, key))
-    {
-        ha_lock(NULL);
+    ha_lock(NULL);
 
+        if(!hsh_get(ctx->pending, key))
+        {
             if(!hsh_set(ctx->pending, key, (void*)conn))
             {
                 free_hash_object(NULL, conn);
                 ha_messagex(NULL, LOG_ERR, "out of memory");
                 r = -1;
             }
+        }
 
-        ha_unlock(NULL);
-    }
+    ha_unlock(NULL);
 
     return r;
 }
@@ -633,7 +633,7 @@ void ntlm_destroy(ha_context_t* context)
         ntlm_context_t* ctx = (ntlm_context_t*)(context->ctx_data);
 
         if(ctx->pending)
-        hsh_free(ctx->pending);
+            hsh_free(ctx->pending);
 
         if(ctx->established)
             hsh_free(ctx->established);
@@ -721,18 +721,18 @@ int ntlm_process(ha_request_t* rq)
     {
         ha_lock(NULL);
 
-        /*
-         * NTLM trusts a connection after it's been authenticated
-         * so just pass success for those. Note that we do this
-         * in the absence of a authorization header so that we
-         * allow connections to be re-authenticated.
-         */
+            /*
+             * NTLM trusts a connection after it's been authenticated
+             * so just pass success for those. Note that we do this
+             * in the absence of a authorization header so that we
+             * allow connections to be re-authenticated.
+             */
 
-        if(hsh_get(ctx->established, key) == NTLM_ESTABLISHED)
-        {
-            hsh_touch(ctx->established, key);
-            rq->resp_code = HA_SERVER_OK;
-        }
+            if(hsh_get(ctx->established, key) == NTLM_ESTABLISHED)
+            {
+                hsh_touch(ctx->established, key);
+                rq->resp_code = HA_SERVER_OK;
+            }
 
         ha_unlock(NULL);
 
diff --git a/daemon/pgsql.c b/daemon/pgsql.c
index 6bdb158..56eebd1 100644
--- a/daemon/pgsql.c
+++ b/daemon/pgsql.c
@@ -25,7 +25,7 @@ typedef struct pgsql_context
     /* Base Handler ------------------------------------------------------ */
     bd_context_t bd;
 
-    /* Settings ---------------------------------------------------------- */
+    /* Read Only --------------------------------------------------------- */
     const char* host;         /* The connection host or path */
     const char* port;         /* The connection port */
     const char* user;         /* The pgsql user name */
@@ -39,7 +39,7 @@ typedef struct pgsql_context
     int pgsql_max;            /* Number of open connections allowed */
     int pgsql_timeout;        /* Maximum amount of time to dedicate to a query */
 
-    /* Context ----------------------------------------------------------- */
+    /* Require Locking --------------------------------------------------- */
     PGconn** pool;            /* Pool of available connections */
     int pool_mark;          /* Amount of connections allocated */
 }
@@ -282,29 +282,42 @@ static const char* make_pgsql_connstring(const ha_request_t* rq, pgsql_context_t
 
 static PGconn* get_pgsql_connection(const ha_request_t* rq, pgsql_context_t* ctx)
 {
-    PGconn* pg;
+    PGconn* pg = NULL;
     const char* connstring;
-    int i;
+    int i, create = 1;
 
     ASSERT(ctx);
 
-    for(i = 0; i < ctx->pgsql_max; i++)
-    {
-        /* An open connection in the pool */
-        if(ctx->pool[i])
+    /*
+     * Note that below there maybe a race condition between the two locks
+     * but this will only allow a few extra connections to open at best
+     * and as such really isn't a big issue.
+     */
+
+    ha_lock(NULL);
+
+        for(i = 0; i < ctx->pgsql_max; i++)
         {
-            ha_messagex(rq, LOG_DEBUG, "using cached pgsql connection");
-            pg = ctx->pool[i];
-            ctx->pool[i] = NULL;
-            return pg;
+            /* An open connection in the pool */
+            if(ctx->pool[i])
+            {
+                ha_messagex(rq, LOG_DEBUG, "using cached pgsql connection");
+                pg = ctx->pool[i];
+                ctx->pool[i] = NULL;
+                break;
+            }
         }
-    }
 
-    if(ctx->pool_mark >= ctx->pgsql_max)
-    {
-        ha_messagex(rq, LOG_ERR, "too many open pgsql connections");
-        return NULL;
-    }
+        if(pg == NULL && ctx->pool_mark >= ctx->pgsql_max)
+        {
+            ha_messagex(rq, LOG_ERR, "too many open pgsql connections");
+            create = 0;
+        }
+
+    ha_unlock(NULL);
+
+    if(pg != NULL || create == 0)
+        return pg;
 
     connstring = make_pgsql_connstring(rq, ctx);
     if(!connstring)
@@ -326,16 +339,26 @@ static PGconn* get_pgsql_connection(const ha_request_t* rq, pgsql_context_t* ctx
         return NULL;
     }
 
-    ctx->pool_mark++;
-    ha_messagex(rq, LOG_DEBUG, "opened new pgsql connection (total %d)", ctx->pool_mark);
+    ha_lock(NULL);
+
+        ctx->pool_mark++;
+        ha_messagex(rq, LOG_DEBUG, "opened new pgsql connection (total %d)", ctx->pool_mark);
+
+    ha_unlock(NULL);
+
     return pg;
 }
 
 static void discard_pgsql_connection(const ha_request_t* rq, pgsql_context_t* ctx, PGconn* pg)
 {
     PQfinish(pg);
-    ctx->pool_mark--;
-    ha_messagex(rq, LOG_DEBUG, "discarding pgsql connection (total %d)", ctx->pool_mark);
+
+    ha_lock(NULL);
+
+        ctx->pool_mark--;
+        ha_messagex(rq, LOG_DEBUG, "discarding pgsql connection (total %d)", ctx->pool_mark);
+
+    ha_unlock(NULL);
 }
 
 static void save_pgsql_connection(const ha_request_t* rq, pgsql_context_t* ctx, PGconn* pg)
@@ -350,17 +373,21 @@ static void save_pgsql_connection(const ha_request_t* rq, pgsql_context_t* ctx,
     /* Make sure it's worth saving */
     if(PQstatus(pg) != CONNECTION_BAD)
     {
-        for(i = 0; i < ctx->pgsql_max; i++)
-        {
-            /* An open connection in the pool */
-            if(!ctx->pool[i])
+        ha_lock(NULL);
+
+            for(i = 0; i < ctx->pgsql_max; i++)
             {
-                ha_messagex(rq, LOG_DEBUG, "caching pgsql connection for later use");
-                ctx->pool[i] = pg;
-                pg = NULL;
-                break;
+                /* An open connection in the pool */
+                if(!ctx->pool[i])
+                {
+                    ha_messagex(rq, LOG_DEBUG, "caching pgsql connection for later use");
+                    ctx->pool[i] = pg;
+                    pg = NULL;
+                    break;
+                }
             }
-        }
+
+        ha_unlock(NULL);
     }
 
     if(pg != NULL)
diff --git a/daemon/simple.c b/daemon/simple.c
index b37922e..5b56138 100644
--- a/daemon/simple.c
+++ b/daemon/simple.c
@@ -23,7 +23,7 @@ typedef struct simple_context
     /* Base Handler ------------------------------------------------------ */
     bd_context_t bd;
 
-    /* Settings ---------------------------------------------------------- */
+    /* Read Only --------------------------------------------------------- */
     const char* filename;           /* The file name with the user names */
 }
 simple_context_t;
diff --git a/sample/httpauthd.conf b/sample/httpauthd.conf
index 19b168b..9e4b0e1 100644
--- a/sample/httpauthd.conf
+++ b/sample/httpauthd.conf
@@ -28,7 +28,7 @@ LDAPFilter: (cn=%u)
 LDAPBase: dc=fam
 LDAPScope: sub
 
-[PGSQL:UDBpg]
+[PGSQL:UDB]
 Realm: blah
 DBServer: sean-2
 DBUser: testuser
@@ -38,7 +38,7 @@ DBQuery: SELECT * from testauth WHERE username = '%u';
 # DBPWType: md5
 DBHA1Column: pw
 
-[MYSQL:UDB]
+[MYSQL:UDBmy]
 Realm: blah
 DBServer: var/lib/mysql/mysql.sock
 DBUser: testuser