Fix race between unlinking a client and processing incoming data

diff --git a/servers/lloadd/backend.c b/servers/lloadd/backend.c
index a2cff0c5251f99f8b72de7c1d6027e9037d2e8d7..e564decb6c412c9d05e718502f523ff410b6750b 100644 (file)
--- a/servers/lloadd/backend.c
+++ b/servers/lloadd/backend.c
@@ -259,6 +259,9 @@ backend_select( LloadOperation *op, int *res )
                         "connid=%lu msgid=%d\n",
                         c->c_connid, op->o_client_connid, op->o_client_msgid );
 
+                /* c_state is DYING if we're about to be unlinked */
+                assert( IS_ALIVE( c, c_live ) );
+
                 /*
                  * Round-robin step:
                  * Rotate the queue to put this connection at the end, same for

diff --git a/servers/lloadd/bind.c b/servers/lloadd/bind.c
index 537ea093c04a04ff2e34372a41b2294a0dd95bc3..bb2534ed026446fa1e2b9bdf44f3ade2ef06cfd6 100644 (file)
--- a/servers/lloadd/bind.c
+++ b/servers/lloadd/bind.c
@@ -338,7 +338,7 @@ request_bind( LloadConnection *client, LloadOperation *op )
         if ( upstream ) {
             ldap_pvt_thread_mutex_lock( &upstream->c_io_mutex );
             CONNECTION_LOCK(upstream);
-            if ( !upstream->c_live ) {
+            if ( !IS_ALIVE( upstream, c_live ) ) {
                 CONNECTION_UNLOCK(upstream);
                 ldap_pvt_thread_mutex_unlock( &upstream->c_io_mutex );
                 upstream = NULL;
@@ -430,6 +430,29 @@ request_bind( LloadConnection *client, LloadOperation *op )
     op->o_upstream_msgid = upstream->c_next_msgid++;
     op->o_res = LLOAD_OP_FAILED;
 
+    /* Was it unlinked in the meantime? No need to send a response since the
+     * client is dead */
+    if ( !IS_ALIVE( op, o_refcnt ) ) {
+        LloadBackend *b = upstream->c_private;
+
+        upstream->c_n_ops_executing--;
+        ldap_pvt_thread_mutex_unlock( &upstream->c_io_mutex );
+        CONNECTION_UNLOCK(upstream);
+
+        ldap_pvt_thread_mutex_lock( &b->b_mutex );
+        b->b_n_ops_executing--;
+        ldap_pvt_thread_mutex_unlock( &b->b_mutex );
+
+        assert( !IS_ALIVE( client, c_live ) );
+        ldap_pvt_thread_mutex_lock( &op->o_link_mutex );
+        if ( op->o_upstream ) {
+            op->o_upstream = NULL;
+        }
+        ldap_pvt_thread_mutex_unlock( &op->o_link_mutex );
+        rc = -1;
+        goto done;
+    }
+
     if ( BER_BVISNULL( &mech ) ) {
         if ( !BER_BVISNULL( &upstream->c_sasl_bind_mech ) ) {
             ber_memfree( upstream->c_sasl_bind_mech.bv_val );

diff --git a/servers/lloadd/client.c b/servers/lloadd/client.c
index 64a756f78e3351dd1a0d7299c5bd6c60b427ad05..3673bddd5eb9ada787fe56d871ba2f0c6927885c 100644 (file)
--- a/servers/lloadd/client.c
+++ b/servers/lloadd/client.c
@@ -106,6 +106,28 @@ request_process( LloadConnection *client, LloadOperation *op )
     op->o_upstream_connid = upstream->c_connid;
     op->o_res = LLOAD_OP_FAILED;
 
+    /* Was it unlinked in the meantime? No need to send a response since the
+     * client is dead */
+    if ( !IS_ALIVE( op, o_refcnt ) ) {
+        LloadBackend *b = upstream->c_private;
+
+        upstream->c_n_ops_executing--;
+        ldap_pvt_thread_mutex_unlock( &upstream->c_io_mutex );
+        CONNECTION_UNLOCK(upstream);
+
+        ldap_pvt_thread_mutex_lock( &b->b_mutex );
+        b->b_n_ops_executing--;
+        ldap_pvt_thread_mutex_unlock( &b->b_mutex );
+
+        assert( !IS_ALIVE( client, c_live ) );
+        ldap_pvt_thread_mutex_lock( &op->o_link_mutex );
+        if ( op->o_upstream ) {
+            op->o_upstream = NULL;
+        }
+        ldap_pvt_thread_mutex_unlock( &op->o_link_mutex );
+        return -1;
+    }
+
     output = upstream->c_pendingber;
     if ( output == NULL && (output = ber_alloc()) == NULL ) {
         LloadBackend *b = upstream->c_private;
@@ -286,12 +308,9 @@ client_tls_handshake_cb( evutil_socket_t s, short what, void *arg )
         ldap_pvt_thread_mutex_unlock( &c->c_io_mutex );
         connection_write_cb( s, what, arg );
 
-        CONNECTION_LOCK(c);
-        if ( !c->c_live ) {
-            CONNECTION_UNLOCK(c);
+        if ( !IS_ALIVE( c, c_live ) ) {
             goto fail;
         }
-        CONNECTION_UNLOCK(c);
 
         /* Do we still have data pending? If so, connection_write_cb would
          * already have arranged the write callback to trigger again */
@@ -324,7 +343,7 @@ client_tls_handshake_cb( evutil_socket_t s, short what, void *arg )
         c->c_read_timeout = NULL;
         event_assign( c->c_read_event, base, c->c_fd, EV_READ|EV_PERSIST,
                 connection_read_cb, c );
-        if ( c->c_live ) {
+        if ( IS_ALIVE( c, c_live ) ) {
             event_add( c->c_read_event, c->c_read_timeout );
         }
 
@@ -338,11 +357,11 @@ client_tls_handshake_cb( evutil_socket_t s, short what, void *arg )
         CONNECTION_UNLOCK(c);
         return;
     } else if ( ber_sockbuf_ctrl( c->c_sb, LBER_SB_OPT_NEEDS_WRITE, NULL ) ) {
-        CONNECTION_LOCK(c);
-        if ( c->c_live ) {
+        if ( IS_ALIVE( c, c_live ) ) {
+            CONNECTION_LOCK(c);
             event_add( c->c_write_event, lload_write_timeout );
+            CONNECTION_UNLOCK(c);
         }
-        CONNECTION_UNLOCK(c);
         Debug( LDAP_DEBUG_CONNS, "client_tls_handshake_cb: "
                 "connid=%lu need write rc=%d\n",
                 c->c_connid, rc );
@@ -537,6 +556,8 @@ client_destroy( LloadConnection *c )
     assert( c->c_state == LLOAD_C_DYING );
     c->c_state = LLOAD_C_INVALID;
 
+    assert( c->c_ops == NULL );
+
     if ( c->c_read_event ) {
         event_free( c->c_read_event );
         c->c_read_event = NULL;

diff --git a/servers/lloadd/connection.c b/servers/lloadd/connection.c
index 886a2b6a83079855fde49b60a24fba7700b13c8d..b1bbff86f36d3c28f664538aadbb879d95972c12 100644 (file)
--- a/servers/lloadd/connection.c
+++ b/servers/lloadd/connection.c
@@ -161,16 +161,13 @@ connection_read_cb( evutil_socket_t s, short what, void *arg )
     ber_len_t len;
     epoch_t epoch;
 
-    CONNECTION_LOCK(c);
-    if ( !c->c_live ) {
+    if ( !IS_ALIVE( c, c_live ) ) {
         event_del( c->c_read_event );
-        CONNECTION_UNLOCK(c);
         Debug( LDAP_DEBUG_CONNS, "connection_read_cb: "
                 "suspended read event on a dead connid=%lu\n",
                 c->c_connid );
         return;
     }
-    CONNECTION_UNLOCK(c);
 
     if ( what & EV_TIMEOUT ) {
         Debug( LDAP_DEBUG_CONNS, "connection_read_cb: "
@@ -271,15 +268,12 @@ connection_write_cb( evutil_socket_t s, short what, void *arg )
     LloadConnection *c = arg;
     epoch_t epoch;
 
-    CONNECTION_LOCK(c);
     Debug( LDAP_DEBUG_CONNS, "connection_write_cb: "
             "considering writing to%s connid=%lu what=%hd\n",
             c->c_live ? " live" : " dead", c->c_connid, what );
-    if ( !c->c_live ) {
-        CONNECTION_UNLOCK(c);
+    if ( !IS_ALIVE( c, c_live ) ) {
         return;
     }
-    CONNECTION_UNLOCK(c);
 
     if ( what & EV_TIMEOUT ) {
         Debug( LDAP_DEBUG_CONNS, "connection_write_cb: "

diff --git a/servers/lloadd/lload.h b/servers/lloadd/lload.h
index 5a96fb1782c86d003da295d28880aa36849e9f5f..8c58e3dfb0d4d1cca4e15e3d702c15cfb74ca9ac 100644 (file)
--- a/servers/lloadd/lload.h
+++ b/servers/lloadd/lload.h
@@ -301,8 +301,7 @@ struct LloadConnection {
 #define CONNECTION_UNLOCK(c) ldap_pvt_thread_mutex_unlock( &(c)->c_mutex )
 #define CONNECTION_UNLINK_(c) \
     do { \
-        if ( (c)->c_live ) { \
-            (c)->c_live = 0; \
+        if ( __atomic_exchange_n( &(c)->c_live, 0, __ATOMIC_ACQ_REL ) ) { \
             RELEASE_REF( (c), c_refcnt, c->c_destroy ); \
             (c)->c_unlink( (c) ); \
         } \

diff --git a/servers/lloadd/operation.c b/servers/lloadd/operation.c
index dd845a337907968974af26603b94e779db31c875..e6bde8c8fcff51bf0132ca0c969e9726a20f3f0e 100644 (file)
--- a/servers/lloadd/operation.c
+++ b/servers/lloadd/operation.c
@@ -127,6 +127,10 @@ operation_init( LloadConnection *c, BerElement *ber )
     ber_len_t len;
     int rc;
 
+    if ( !IS_ALIVE( c, c_live ) ) {
+        return NULL;
+    }
+
     op = ch_calloc( 1, sizeof(LloadOperation) );
     op->o_client = c;
     op->o_client_connid = c->c_connid;
@@ -343,7 +347,7 @@ operation_send_abandon( LloadOperation *op, LloadConnection *upstream )
     BerElement *ber;
     int rc = -1;
 
-    if ( !IS_ALIVE( upstream, c_refcnt ) ) {
+    if ( !IS_ALIVE( upstream, c_live ) ) {
         return rc;
     }
 
@@ -400,7 +404,7 @@ operation_abandon( LloadOperation *op )
     ldap_pvt_thread_mutex_lock( &op->o_link_mutex );
     c = op->o_upstream;
     ldap_pvt_thread_mutex_unlock( &op->o_link_mutex );
-    if ( !c || !IS_ALIVE( c, c_refcnt ) ) {
+    if ( !c || !IS_ALIVE( c, c_live ) ) {
         goto done;
     }
 
@@ -443,7 +447,7 @@ operation_send_reject(
     ldap_pvt_thread_mutex_lock( &op->o_link_mutex );
     c = op->o_client;
     ldap_pvt_thread_mutex_unlock( &op->o_link_mutex );
-    if ( !c || !IS_ALIVE( c, c_refcnt ) ) {
+    if ( !c || !IS_ALIVE( c, c_live ) ) {
         Debug( LDAP_DEBUG_TRACE, "operation_send_reject: "
                 "not sending msgid=%d, client connid=%lu is dead\n",
                 op->o_client_msgid, op->o_client_connid );

diff --git a/servers/lloadd/upstream.c b/servers/lloadd/upstream.c
index 118d3a29c926813fe52dc9edba224c939363c259..458af0c1e4331343bf45ae69cb612b373b13c15f 100644 (file)
--- a/servers/lloadd/upstream.c
+++ b/servers/lloadd/upstream.c
@@ -247,7 +247,7 @@ handle_one_response( LloadConnection *c )
         ldap_pvt_thread_mutex_lock( &op->o_link_mutex );
         client = op->o_client;
         ldap_pvt_thread_mutex_unlock( &op->o_link_mutex );
-        if ( client && IS_ALIVE( client, c_refcnt ) ) {
+        if ( client && IS_ALIVE( client, c_live ) ) {
             rc = handler( client, op, ber );
         } else {
             ber_free( ber, 1 );
@@ -1054,6 +1054,8 @@ upstream_destroy( LloadConnection *c )
     assert( c->c_state == LLOAD_C_DYING );
     c->c_state = LLOAD_C_INVALID;
 
+    assert( c->c_ops == NULL );
+
     if ( c->c_read_event ) {
         event_free( c->c_read_event );
         c->c_read_event = NULL;