Commit 58d66a39 authored by Ondřej Kuzník's avatar Ondřej Kuzník
Browse files

Fix race between unlinking a client and processing incoming data

parent 959ff079
...@@ -259,6 +259,9 @@ backend_select( LloadOperation *op, int *res ) ...@@ -259,6 +259,9 @@ backend_select( LloadOperation *op, int *res )
"connid=%lu msgid=%d\n", "connid=%lu msgid=%d\n",
c->c_connid, op->o_client_connid, op->o_client_msgid ); 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: * Round-robin step:
* Rotate the queue to put this connection at the end, same for * Rotate the queue to put this connection at the end, same for
......
...@@ -338,7 +338,7 @@ request_bind( LloadConnection *client, LloadOperation *op ) ...@@ -338,7 +338,7 @@ request_bind( LloadConnection *client, LloadOperation *op )
if ( upstream ) { if ( upstream ) {
ldap_pvt_thread_mutex_lock( &upstream->c_io_mutex ); ldap_pvt_thread_mutex_lock( &upstream->c_io_mutex );
CONNECTION_LOCK(upstream); CONNECTION_LOCK(upstream);
if ( !upstream->c_live ) { if ( !IS_ALIVE( upstream, c_live ) ) {
CONNECTION_UNLOCK(upstream); CONNECTION_UNLOCK(upstream);
ldap_pvt_thread_mutex_unlock( &upstream->c_io_mutex ); ldap_pvt_thread_mutex_unlock( &upstream->c_io_mutex );
upstream = NULL; upstream = NULL;
...@@ -430,6 +430,29 @@ request_bind( LloadConnection *client, LloadOperation *op ) ...@@ -430,6 +430,29 @@ request_bind( LloadConnection *client, LloadOperation *op )
op->o_upstream_msgid = upstream->c_next_msgid++; op->o_upstream_msgid = upstream->c_next_msgid++;
op->o_res = LLOAD_OP_FAILED; 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( &mech ) ) {
if ( !BER_BVISNULL( &upstream->c_sasl_bind_mech ) ) { if ( !BER_BVISNULL( &upstream->c_sasl_bind_mech ) ) {
ber_memfree( upstream->c_sasl_bind_mech.bv_val ); ber_memfree( upstream->c_sasl_bind_mech.bv_val );
......
...@@ -106,6 +106,28 @@ request_process( LloadConnection *client, LloadOperation *op ) ...@@ -106,6 +106,28 @@ request_process( LloadConnection *client, LloadOperation *op )
op->o_upstream_connid = upstream->c_connid; op->o_upstream_connid = upstream->c_connid;
op->o_res = LLOAD_OP_FAILED; 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; output = upstream->c_pendingber;
if ( output == NULL && (output = ber_alloc()) == NULL ) { if ( output == NULL && (output = ber_alloc()) == NULL ) {
LloadBackend *b = upstream->c_private; LloadBackend *b = upstream->c_private;
...@@ -286,12 +308,9 @@ client_tls_handshake_cb( evutil_socket_t s, short what, void *arg ) ...@@ -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 ); ldap_pvt_thread_mutex_unlock( &c->c_io_mutex );
connection_write_cb( s, what, arg ); connection_write_cb( s, what, arg );
CONNECTION_LOCK(c); if ( !IS_ALIVE( c, c_live ) ) {
if ( !c->c_live ) {
CONNECTION_UNLOCK(c);
goto fail; goto fail;
} }
CONNECTION_UNLOCK(c);
/* Do we still have data pending? If so, connection_write_cb would /* Do we still have data pending? If so, connection_write_cb would
* already have arranged the write callback to trigger again */ * 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 ) ...@@ -324,7 +343,7 @@ client_tls_handshake_cb( evutil_socket_t s, short what, void *arg )
c->c_read_timeout = NULL; c->c_read_timeout = NULL;
event_assign( c->c_read_event, base, c->c_fd, EV_READ|EV_PERSIST, event_assign( c->c_read_event, base, c->c_fd, EV_READ|EV_PERSIST,
connection_read_cb, c ); connection_read_cb, c );
if ( c->c_live ) { if ( IS_ALIVE( c, c_live ) ) {
event_add( c->c_read_event, c->c_read_timeout ); 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 ) ...@@ -338,11 +357,11 @@ client_tls_handshake_cb( evutil_socket_t s, short what, void *arg )
CONNECTION_UNLOCK(c); CONNECTION_UNLOCK(c);
return; return;
} else if ( ber_sockbuf_ctrl( c->c_sb, LBER_SB_OPT_NEEDS_WRITE, NULL ) ) { } else if ( ber_sockbuf_ctrl( c->c_sb, LBER_SB_OPT_NEEDS_WRITE, NULL ) ) {
CONNECTION_LOCK(c); if ( IS_ALIVE( c, c_live ) ) {
if ( c->c_live ) { CONNECTION_LOCK(c);
event_add( c->c_write_event, lload_write_timeout ); event_add( c->c_write_event, lload_write_timeout );
CONNECTION_UNLOCK(c);
} }
CONNECTION_UNLOCK(c);
Debug( LDAP_DEBUG_CONNS, "client_tls_handshake_cb: " Debug( LDAP_DEBUG_CONNS, "client_tls_handshake_cb: "
"connid=%lu need write rc=%d\n", "connid=%lu need write rc=%d\n",
c->c_connid, rc ); c->c_connid, rc );
...@@ -537,6 +556,8 @@ client_destroy( LloadConnection *c ) ...@@ -537,6 +556,8 @@ client_destroy( LloadConnection *c )
assert( c->c_state == LLOAD_C_DYING ); assert( c->c_state == LLOAD_C_DYING );
c->c_state = LLOAD_C_INVALID; c->c_state = LLOAD_C_INVALID;
assert( c->c_ops == NULL );
if ( c->c_read_event ) { if ( c->c_read_event ) {
event_free( c->c_read_event ); event_free( c->c_read_event );
c->c_read_event = NULL; c->c_read_event = NULL;
......
...@@ -161,16 +161,13 @@ connection_read_cb( evutil_socket_t s, short what, void *arg ) ...@@ -161,16 +161,13 @@ connection_read_cb( evutil_socket_t s, short what, void *arg )
ber_len_t len; ber_len_t len;
epoch_t epoch; epoch_t epoch;
CONNECTION_LOCK(c); if ( !IS_ALIVE( c, c_live ) ) {
if ( !c->c_live ) {
event_del( c->c_read_event ); event_del( c->c_read_event );
CONNECTION_UNLOCK(c);
Debug( LDAP_DEBUG_CONNS, "connection_read_cb: " Debug( LDAP_DEBUG_CONNS, "connection_read_cb: "
"suspended read event on a dead connid=%lu\n", "suspended read event on a dead connid=%lu\n",
c->c_connid ); c->c_connid );
return; return;
} }
CONNECTION_UNLOCK(c);
if ( what & EV_TIMEOUT ) { if ( what & EV_TIMEOUT ) {
Debug( LDAP_DEBUG_CONNS, "connection_read_cb: " Debug( LDAP_DEBUG_CONNS, "connection_read_cb: "
...@@ -271,15 +268,12 @@ connection_write_cb( evutil_socket_t s, short what, void *arg ) ...@@ -271,15 +268,12 @@ connection_write_cb( evutil_socket_t s, short what, void *arg )
LloadConnection *c = arg; LloadConnection *c = arg;
epoch_t epoch; epoch_t epoch;
CONNECTION_LOCK(c);
Debug( LDAP_DEBUG_CONNS, "connection_write_cb: " Debug( LDAP_DEBUG_CONNS, "connection_write_cb: "
"considering writing to%s connid=%lu what=%hd\n", "considering writing to%s connid=%lu what=%hd\n",
c->c_live ? " live" : " dead", c->c_connid, what ); c->c_live ? " live" : " dead", c->c_connid, what );
if ( !c->c_live ) { if ( !IS_ALIVE( c, c_live ) ) {
CONNECTION_UNLOCK(c);
return; return;
} }
CONNECTION_UNLOCK(c);
if ( what & EV_TIMEOUT ) { if ( what & EV_TIMEOUT ) {
Debug( LDAP_DEBUG_CONNS, "connection_write_cb: " Debug( LDAP_DEBUG_CONNS, "connection_write_cb: "
......
...@@ -301,8 +301,7 @@ struct LloadConnection { ...@@ -301,8 +301,7 @@ struct LloadConnection {
#define CONNECTION_UNLOCK(c) ldap_pvt_thread_mutex_unlock( &(c)->c_mutex ) #define CONNECTION_UNLOCK(c) ldap_pvt_thread_mutex_unlock( &(c)->c_mutex )
#define CONNECTION_UNLINK_(c) \ #define CONNECTION_UNLINK_(c) \
do { \ do { \
if ( (c)->c_live ) { \ if ( __atomic_exchange_n( &(c)->c_live, 0, __ATOMIC_ACQ_REL ) ) { \
(c)->c_live = 0; \
RELEASE_REF( (c), c_refcnt, c->c_destroy ); \ RELEASE_REF( (c), c_refcnt, c->c_destroy ); \
(c)->c_unlink( (c) ); \ (c)->c_unlink( (c) ); \
} \ } \
......
...@@ -127,6 +127,10 @@ operation_init( LloadConnection *c, BerElement *ber ) ...@@ -127,6 +127,10 @@ operation_init( LloadConnection *c, BerElement *ber )
ber_len_t len; ber_len_t len;
int rc; int rc;
if ( !IS_ALIVE( c, c_live ) ) {
return NULL;
}
op = ch_calloc( 1, sizeof(LloadOperation) ); op = ch_calloc( 1, sizeof(LloadOperation) );
op->o_client = c; op->o_client = c;
op->o_client_connid = c->c_connid; op->o_client_connid = c->c_connid;
...@@ -343,7 +347,7 @@ operation_send_abandon( LloadOperation *op, LloadConnection *upstream ) ...@@ -343,7 +347,7 @@ operation_send_abandon( LloadOperation *op, LloadConnection *upstream )
BerElement *ber; BerElement *ber;
int rc = -1; int rc = -1;
if ( !IS_ALIVE( upstream, c_refcnt ) ) { if ( !IS_ALIVE( upstream, c_live ) ) {
return rc; return rc;
} }
...@@ -400,7 +404,7 @@ operation_abandon( LloadOperation *op ) ...@@ -400,7 +404,7 @@ operation_abandon( LloadOperation *op )
ldap_pvt_thread_mutex_lock( &op->o_link_mutex ); ldap_pvt_thread_mutex_lock( &op->o_link_mutex );
c = op->o_upstream; c = op->o_upstream;
ldap_pvt_thread_mutex_unlock( &op->o_link_mutex ); 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; goto done;
} }
...@@ -443,7 +447,7 @@ operation_send_reject( ...@@ -443,7 +447,7 @@ operation_send_reject(
ldap_pvt_thread_mutex_lock( &op->o_link_mutex ); ldap_pvt_thread_mutex_lock( &op->o_link_mutex );
c = op->o_client; c = op->o_client;
ldap_pvt_thread_mutex_unlock( &op->o_link_mutex ); 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: " Debug( LDAP_DEBUG_TRACE, "operation_send_reject: "
"not sending msgid=%d, client connid=%lu is dead\n", "not sending msgid=%d, client connid=%lu is dead\n",
op->o_client_msgid, op->o_client_connid ); op->o_client_msgid, op->o_client_connid );
......
...@@ -247,7 +247,7 @@ handle_one_response( LloadConnection *c ) ...@@ -247,7 +247,7 @@ handle_one_response( LloadConnection *c )
ldap_pvt_thread_mutex_lock( &op->o_link_mutex ); ldap_pvt_thread_mutex_lock( &op->o_link_mutex );
client = op->o_client; client = op->o_client;
ldap_pvt_thread_mutex_unlock( &op->o_link_mutex ); 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 ); rc = handler( client, op, ber );
} else { } else {
ber_free( ber, 1 ); ber_free( ber, 1 );
...@@ -1054,6 +1054,8 @@ upstream_destroy( LloadConnection *c ) ...@@ -1054,6 +1054,8 @@ upstream_destroy( LloadConnection *c )
assert( c->c_state == LLOAD_C_DYING ); assert( c->c_state == LLOAD_C_DYING );
c->c_state = LLOAD_C_INVALID; c->c_state = LLOAD_C_INVALID;
assert( c->c_ops == NULL );
if ( c->c_read_event ) { if ( c->c_read_event ) {
event_free( c->c_read_event ); event_free( c->c_read_event );
c->c_read_event = NULL; c->c_read_event = NULL;
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment