From 07abb0eb3a4c352b99c862d7f7979af1448bbc87 Mon Sep 17 00:00:00 2001 From: Pierangelo Masarati Date: Sat, 28 Oct 2006 16:20:59 +0000 Subject: [PATCH] fix concurrency issue when binding before a search; rework and cleanup data structures (remove unused members) --- servers/slapd/back-meta/back-meta.h | 18 ++-- servers/slapd/back-meta/bind.c | 9 ++ servers/slapd/back-meta/candidates.c | 14 ++- servers/slapd/back-meta/conn.c | 40 ++++++--- servers/slapd/back-meta/init.c | 5 +- servers/slapd/back-meta/search.c | 129 +++++++++++++++++++++++---- 6 files changed, 174 insertions(+), 41 deletions(-) diff --git a/servers/slapd/back-meta/back-meta.h b/servers/slapd/back-meta/back-meta.h index 0771431096..3f3c933bca 100644 --- a/servers/slapd/back-meta/back-meta.h +++ b/servers/slapd/back-meta/back-meta.h @@ -162,12 +162,11 @@ ldap_dnattr_result_rewrite( struct metainfo_t; -typedef struct metasingleconn_t { - int msc_candidate; -#define META_NOT_CANDIDATE ((ber_tag_t)0x0) -#define META_CANDIDATE ((ber_tag_t)0x1) -#define META_BINDING ((ber_tag_t)0x2) +#define META_NOT_CANDIDATE ((ber_tag_t)0x0) +#define META_CANDIDATE ((ber_tag_t)0x1) +#define META_BINDING ((ber_tag_t)0x2) +typedef struct metasingleconn_t { #define META_CND_ISSET(rs,f) ( ( (rs)->sr_tag & (f) ) == (f) ) #define META_CND_SET(rs,f) ( (rs)->sr_tag |= (f) ) #define META_CND_CLEAR(rs,f) ( (rs)->sr_tag &= ~(f) ) @@ -188,8 +187,6 @@ typedef struct metasingleconn_t { /* NOTE: lc_lcflags is redefined to msc_mscflags to reuse the macros * defined for back-ldap */ #define lc_lcflags msc_mscflags - - struct metainfo_t *msc_info; } metasingleconn_t; typedef struct metaconn_t { @@ -212,6 +209,9 @@ typedef struct metaconn_t { int mc_authz_target; #define META_BOUND_NONE (-1) #define META_BOUND_ALL (-2) + + struct metainfo_t *mc_info; + /* supersedes the connection stuff */ metasingleconn_t mc_conns[ 1 ]; /* NOTE: mc_conns must be last, because @@ -490,7 +490,9 @@ meta_clear_unused_candidates( extern int meta_clear_one_candidate( - metasingleconn_t *mc ); + Operation *op, + metaconn_t *mc, + int candidate ); extern int meta_clear_candidates( diff --git a/servers/slapd/back-meta/bind.c b/servers/slapd/back-meta/bind.c index d2d9cb02d6..59c5fabd35 100644 --- a/servers/slapd/back-meta/bind.c +++ b/servers/slapd/back-meta/bind.c @@ -33,6 +33,8 @@ #include "slap.h" #include "../back-ldap/back-ldap.h" #include "back-meta.h" +#undef ldap_debug /* silence a warning in ldap-int.h */ +#include "../../../libraries/libldap/ldap-int.h" #include "lutil_ldap.h" @@ -392,6 +394,13 @@ retry:; * because there's a pending bind that will not * be acknowledged */ ldap_pvt_thread_mutex_lock( &mi->mi_conninfo.lai_mutex ); + assert( LDAP_BACK_CONN_BINDING( msc ) ); + +#if 0 + Debug( LDAP_DEBUG_ANY, "### %s meta_back_bind_op_result ldap_unbind_ext[%d] ld=%p\n", + op->o_log_prefix, candidate, (void *)msc->msc_ld ); +#endif + ldap_unbind_ext( msc->msc_ld, NULL, NULL ); msc->msc_ld = NULL; LDAP_BACK_CONN_BINDING_CLEAR( msc ); diff --git a/servers/slapd/back-meta/candidates.c b/servers/slapd/back-meta/candidates.c index 64b56cc8ad..878e6aee58 100644 --- a/servers/slapd/back-meta/candidates.c +++ b/servers/slapd/back-meta/candidates.c @@ -181,9 +181,19 @@ meta_clear_unused_candidates( */ int meta_clear_one_candidate( - metasingleconn_t *msc ) + Operation *op, + metaconn_t *mc, + int candidate ) { + metasingleconn_t *msc = &mc->mc_conns[ candidate ]; + if ( msc->msc_ld ) { + +#if 0 + Debug( LDAP_DEBUG_ANY, "### %s meta_clear_one_candidate ldap_unbind_ext[%d] mc=%p\n", + op ? op->o_log_prefix : "", candidate, (void *)mc ); +#endif + ldap_unbind_ext( msc->msc_ld, NULL, NULL ); msc->msc_ld = NULL; } @@ -214,7 +224,7 @@ meta_clear_candidates( Operation *op, metaconn_t *mc ) int c; for ( c = 0; c < mi->mi_ntargets; c++ ) { - meta_clear_one_candidate( &mc->mc_conns[ c ] ); + meta_clear_one_candidate( op, mc, c ); } return 0; diff --git a/servers/slapd/back-meta/conn.c b/servers/slapd/back-meta/conn.c index 976fc7f77d..8d226df8ad 100644 --- a/servers/slapd/back-meta/conn.c +++ b/servers/slapd/back-meta/conn.c @@ -195,7 +195,7 @@ metaconn_alloc( { metainfo_t *mi = ( metainfo_t * )op->o_bd->be_private; metaconn_t *mc; - int i, ntargets = mi->mi_ntargets; + int ntargets = mi->mi_ntargets; assert( ntargets > 0 ); @@ -206,9 +206,7 @@ metaconn_alloc( return NULL; } - for ( i = 0; i < ntargets; i++ ) { - mc->mc_conns[ i ].msc_info = mi; - } + mc->mc_info = mi; mc->mc_authz_target = META_BOUND_NONE; mc->mc_refcnt = 1; @@ -425,6 +423,12 @@ retry:; if ( rs->sr_err == LDAP_SERVER_DOWN || ( rs->sr_err != LDAP_SUCCESS && LDAP_BACK_TLS_CRITICAL( mi ) ) ) { + +#if 0 + Debug( LDAP_DEBUG_ANY, "### %s meta_back_init_one_conn(TLS) ldap_unbind_ext[%d] ld=%p\n", + op->o_log_prefix, candidate, (void *)msc->msc_ld ); +#endif + ldap_unbind_ext( msc->msc_ld, NULL, NULL ); msc->msc_ld = NULL; goto error_return; @@ -487,6 +491,12 @@ retry:; if ( ldap_back_dn_massage( &dc, &op->o_conn->c_dn, &msc->msc_bound_ndn ) ) { + +#if 0 + Debug( LDAP_DEBUG_ANY, "### %s meta_back_init_one_conn(rewrite) ldap_unbind_ext[%d] ld=%p\n", + op->o_log_prefix, candidate, (void *)msc->msc_ld ); +#endif + ldap_unbind_ext( msc->msc_ld, NULL, NULL ); msc->msc_ld = NULL; goto error_return; @@ -563,7 +573,7 @@ meta_back_retry( op->o_log_prefix, candidate, buf ); } - meta_clear_one_candidate( msc ); + meta_clear_one_candidate( op, mc, candidate ); LDAP_BACK_CONN_ISBOUND_CLEAR( msc ); ( void )rewrite_session_delete( mt->mt_rwmap.rwm_rw, op->o_conn ); @@ -571,6 +581,8 @@ meta_back_retry( /* mc here must be the regular mc, reset and ready for init */ rc = meta_back_init_one_conn( op, rs, mc, candidate, LDAP_BACK_CONN_ISPRIV( mc ), sendok ); + + /* restore the "binding" flag, in case */ if ( binding ) { LDAP_BACK_CONN_BINDING_SET( msc ); } @@ -584,11 +596,19 @@ meta_back_retry( "meta_back_single_dobind=%d\n", op->o_log_prefix, candidate, rc ); if ( rc == LDAP_SUCCESS ) { - if ( be_isroot( op ) ) { + if ( !BER_BVISNULL( &msc->msc_bound_ndn ) && + !BER_BVISEMPTY( &msc->msc_bound_ndn ) ) + { LDAP_BACK_CONN_ISBOUND_SET( msc ); + } else { LDAP_BACK_CONN_ISANON_SET( msc ); } + + /* when bound, dispose of the "binding" flag */ + if ( binding ) { + LDAP_BACK_CONN_BINDING_CLEAR( msc ); + } } } } @@ -1184,7 +1204,8 @@ retry_lock2:; for ( i = 0; i < mi->mi_ntargets; i++ ) { metatarget_t *mt = mi->mi_targets[ i ]; - metasingleconn_t *msc = &mc->mc_conns[ i ]; + + META_CANDIDATE_RESET( &candidates[ i ] ); if ( i == cached || meta_back_is_candidate( mt, &op->o_req_ndn, @@ -1221,7 +1242,7 @@ retry_lock2:; * be tried? */ if ( new_conn ) { - ( void )meta_clear_one_candidate( msc ); + ( void )meta_clear_one_candidate( op, mc, i ); } /* leave the target candidate, but record the error for later use */ candidates[ i ].sr_err = lerr; @@ -1258,9 +1279,8 @@ retry_lock2:; } else { if ( new_conn ) { - ( void )meta_clear_one_candidate( msc ); + ( void )meta_clear_one_candidate( op, mc, i ); } - META_CANDIDATE_RESET( &candidates[ i ] ); } } diff --git a/servers/slapd/back-meta/init.c b/servers/slapd/back-meta/init.c index 46aee301ca..8bec4e3fe2 100644 --- a/servers/slapd/back-meta/init.c +++ b/servers/slapd/back-meta/init.c @@ -170,17 +170,18 @@ meta_back_conn_free( { metaconn_t *mc = v_mc; int ntargets; + Operation op; assert( mc != NULL ); assert( mc->mc_refcnt == 0 ); /* at least one must be present... */ assert( mc->mc_conns != NULL ); - ntargets = mc->mc_conns[ 0 ].msc_info->mi_ntargets; + ntargets = mc->mc_info->mi_ntargets; assert( ntargets > 0 ); for ( ; ntargets--; ) { - (void)meta_clear_one_candidate( &mc->mc_conns[ ntargets ] ); + (void)meta_clear_one_candidate( NULL, mc, ntargets ); } if ( !BER_BVISNULL( &mc->mc_local_ndn ) ) { diff --git a/servers/slapd/back-meta/search.c b/servers/slapd/back-meta/search.c index 919cb92760..3c34c3aa65 100644 --- a/servers/slapd/back-meta/search.c +++ b/servers/slapd/back-meta/search.c @@ -110,6 +110,7 @@ meta_search_dobind_init( LDAP_BACK_CONN_BINDING_SET( msc ); } ldap_pvt_thread_mutex_unlock( &mi->mi_conninfo.lai_mutex ); + if ( retcode != META_SEARCH_BINDING ) { return retcode; } @@ -186,6 +187,7 @@ meta_search_dobind_init( NULL, NULL, &candidates[ candidate ].sr_msgid ); switch ( rc ) { case LDAP_SUCCESS: + assert( candidates[ candidate ].sr_msgid >= 0 ); META_BINDING_SET( &candidates[ candidate ] ); return META_SEARCH_BINDING; @@ -194,6 +196,7 @@ down:; /* This is the worst thing that could happen: * the search will wait until the retry is over. */ if ( meta_back_retry( op, rs, mcp, candidate, LDAP_BACK_DONTSEND ) ) { + candidates[ candidate ].sr_msgid = META_MSGID_IGNORE; return META_SEARCH_CANDIDATE; } @@ -536,7 +539,9 @@ meta_back_search( Operation *op, SlapReply *rs ) int rc = 0, sres = LDAP_SUCCESS; char *matched = NULL; int last = 0, ncandidates = 0, - initial_candidates = 0, candidate_match = 0; + initial_candidates = 0, candidate_match = 0, + needbind = 0; + ldap_back_send_t sendok = LDAP_BACK_SENDERR; long i; dncookie dc; int is_ok = 0; @@ -549,7 +554,8 @@ meta_back_search( Operation *op, SlapReply *rs ) * FIXME: in case of values return filter, we might want * to map attrs and maybe rewrite value */ - mc = meta_back_getconn( op, rs, NULL, LDAP_BACK_SENDERR ); +getconn:; + mc = meta_back_getconn( op, rs, NULL, sendok ); if ( !mc ) { return rs->sr_err; } @@ -585,9 +591,12 @@ meta_back_search( Operation *op, SlapReply *rs ) case META_SEARCH_NOT_CANDIDATE: break; + case META_SEARCH_NEED_BIND: + ++needbind; + /* fallthru */ + case META_SEARCH_CANDIDATE: case META_SEARCH_BINDING: - case META_SEARCH_NEED_BIND: candidates[ i ].sr_type = REP_INTERMEDIATE; ++ncandidates; break; @@ -602,6 +611,26 @@ meta_back_search( Operation *op, SlapReply *rs ) } } + if ( ncandidates > 0 && needbind == ncandidates ) { + assert( ( sendok & LDAP_BACK_BINDING ) == 0 ); + + /* FIXME: better create a separate connection? */ + sendok |= LDAP_BACK_BINDING; + +#if 0 + Debug( LDAP_DEBUG_TRACE, "*** %s drop mc=%p create new connection\n", + op->o_log_prefix, mc, 0 ); +#endif + + meta_back_release_conn( op, mc ); + mc = NULL; + + needbind = 0; + ncandidates = 0; + + goto getconn; + } + initial_candidates = ncandidates; if ( LogTest( LDAP_DEBUG_TRACE ) ) { @@ -668,7 +697,10 @@ meta_back_search( Operation *op, SlapReply *rs ) * among the candidates */ for ( rc = 0; ncandidates > 0; ) { - int gotit = 0, doabandon = 0; + int gotit = 0, + doabandon = 0; + + needbind = ncandidates; /* check time limit */ if ( op->ors_tlimit != SLAP_NO_LIMIT @@ -701,17 +733,11 @@ meta_back_search( Operation *op, SlapReply *rs ) op->o_log_prefix, i, retcode ); switch ( retcode ) { - case META_SEARCH_BINDING: case META_SEARCH_NEED_BIND: - break; + needbind--; + /* fallthru */ - case META_SEARCH_NOT_CANDIDATE: - /* - * When no candidates are left, - * the outer cycle finishes - */ - candidates[ i ].sr_msgid = META_MSGID_IGNORE; - --ncandidates; + case META_SEARCH_BINDING: break; case META_SEARCH_ERR: @@ -722,6 +748,16 @@ meta_back_search( Operation *op, SlapReply *rs ) op->o_private = savepriv; goto finish; } + /* fallthru */ + + case META_SEARCH_NOT_CANDIDATE: + /* + * When no candidates are left, + * the outer cycle finishes + */ + candidates[ i ].sr_msgid = META_MSGID_IGNORE; + assert( ncandidates > 0 ); + --ncandidates; break; case META_SEARCH_CANDIDATE: @@ -729,14 +765,10 @@ meta_back_search( Operation *op, SlapReply *rs ) switch ( meta_back_search_start( op, rs, &dc, &mc, i, candidates ) ) { case META_SEARCH_CANDIDATE: + assert( candidates[ i ].sr_msgid >= 0 ); break; - /* means that failed but onerr == continue */ - case META_SEARCH_NOT_CANDIDATE: case META_SEARCH_ERR: - candidates[ i ].sr_msgid = META_MSGID_IGNORE; - --ncandidates; - if ( META_BACK_ONERR_STOP( mi ) ) { savepriv = op->o_private; op->o_private = (void *)i; @@ -744,6 +776,14 @@ meta_back_search( Operation *op, SlapReply *rs ) op->o_private = savepriv; goto finish; } + /* fallthru */ + + case META_SEARCH_NOT_CANDIDATE: + /* means that meta_back_search_start() + * failed but onerr == continue */ + candidates[ i ].sr_msgid = META_MSGID_IGNORE; + assert( ncandidates > 0 ); + --ncandidates; break; default: @@ -814,6 +854,7 @@ really_bad:; assert( 0 ); default: + /* unrecoverable error */ rc = rs->sr_err = LDAP_OTHER; goto finish; } @@ -833,6 +874,7 @@ really_bad:; * the outer cycle finishes */ candidates[ i ].sr_msgid = META_MSGID_IGNORE; + assert( ncandidates > 0 ); --ncandidates; rs->sr_err = candidates[ i ].sr_err; continue; @@ -919,6 +961,7 @@ really_bad:; #endif /* ! ENABLE_REWRITE */ /* FIXME: merge all and return at the end */ + for ( cnt = 0; references[ cnt ]; cnt++ ) ; @@ -1181,6 +1224,13 @@ really_bad:; ldap_pvt_thread_mutex_lock( &mi->mi_conninfo.lai_mutex ); if ( LDAP_BACK_CONN_BINDING( &mc->mc_conns[ i ] ) ) { /* if still binding, destroy */ + +#if 0 + Debug( LDAP_DEBUG_ANY, "### %s meta_back_search(abandon) " + "ldap_unbind_ext[%ld] ld=%p\n", + op->o_log_prefix, i, (void *)mc->mc_conns[i].msc_ld ); +#endif + ldap_unbind_ext( mc->mc_conns[ i ].msc_ld, NULL, NULL ); mc->mc_conns[ i ].msc_ld = NULL; LDAP_BACK_CONN_BINDING_CLEAR( &mc->mc_conns[ i ] ); @@ -1195,6 +1245,8 @@ really_bad:; } candidates[ i ].sr_msgid = META_MSGID_IGNORE; + assert( ncandidates > 0 ); + --ncandidates; } } @@ -1219,7 +1271,40 @@ really_bad:; lutil_timermul( &save_tv, 2, &save_tv ); } - ldap_pvt_thread_yield(); +#if 0 + if ( LogTest( LDAP_DEBUG_TRACE ) ) { + char buf[ BUFSIZ ]; + + snprintf( buf, sizeof( buf ), "%s %ld.%06ld %d/%d mc=%p", + op->o_log_prefix, save_tv.tv_sec, save_tv.tv_usec, + ncandidates, initial_candidates, mc ); + Debug( LDAP_DEBUG_TRACE, "### %s\n", buf, 0, 0 ); + for ( i = 0; i < mi->mi_ntargets; i++ ) { + if ( candidates[ i ].sr_msgid == META_MSGID_IGNORE ) { + continue; + } + + snprintf( buf, sizeof( buf ), "[%ld] ld=%p%s%s\n", + i, + mc->mc_conns[ i ].msc_ld, + ( candidates[ i ].sr_msgid == META_MSGID_NEED_BIND ) ? " needbind" : "", + META_IS_BINDING( &candidates[ i ] ) ? " binding" : "" ); + Debug( LDAP_DEBUG_TRACE, "### %s\n", buf, 0, 0 ); + } + } +#endif + + if ( needbind == 0 ) { +#if 0 + Debug( LDAP_DEBUG_TRACE, "### %s select(%ld.%06ld)\n", + op->o_log_prefix, save_tv.tv_sec, save_tv.tv_usec ); +#endif + tv = save_tv; + (void)select( 0, NULL, NULL, NULL, &tv ); + + } else { + ldap_pvt_thread_yield(); + } } } @@ -1364,6 +1449,12 @@ finish:; assert( candidates[ i ].sr_msgid >= 0 ); assert( mc->mc_conns[ i ].msc_ld != NULL ); +#if 0 + Debug( LDAP_DEBUG_ANY, "### %s meta_back_search(cleanup) " + "ldap_unbind_ext[%ld] ld=%p\n", + op->o_log_prefix, i, (void *)mc->mc_conns[i].msc_ld ); +#endif + /* if still binding, destroy */ ldap_unbind_ext( mc->mc_conns[ i ].msc_ld, NULL, NULL ); mc->mc_conns[ i ].msc_ld = NULL; -- 2.39.5