From 58d66a3946486ecde00c1666104f659abdc141d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Kuzn=C3=ADk?= Date: Mon, 29 Oct 2018 14:00:24 +0000 Subject: [PATCH] Fix race between unlinking a client and processing incoming data --- servers/lloadd/backend.c | 3 +++ servers/lloadd/bind.c | 25 ++++++++++++++++++++++++- servers/lloadd/client.c | 37 +++++++++++++++++++++++++++++-------- servers/lloadd/connection.c | 10 ++-------- servers/lloadd/lload.h | 3 +-- servers/lloadd/operation.c | 10 +++++++--- servers/lloadd/upstream.c | 4 +++- 7 files changed, 69 insertions(+), 23 deletions(-) diff --git a/servers/lloadd/backend.c b/servers/lloadd/backend.c index a2cff0c525..e564decb6c 100644 --- 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 537ea093c0..bb2534ed02 100644 --- 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 64a756f78e..3673bddd5e 100644 --- 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 886a2b6a83..b1bbff86f3 100644 --- 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 5a96fb1782..8c58e3dfb0 100644 --- 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 dd845a3379..e6bde8c8fc 100644 --- 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 118d3a29c9..458af0c1e4 100644 --- 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;