From: Howard Chu Date: Tue, 10 Feb 2009 09:51:31 +0000 (+0000) Subject: ITS#5853 restructure wait4msg / try_read1msg again. Consolidate X-Git-Tag: ACLCHECK_0~853 X-Git-Url: https://git.sur5r.net/?a=commitdiff_plain;h=80c6ea52ea8035d1265b074ca36bc9e6fc66dd7b;p=openldap ITS#5853 restructure wait4msg / try_read1msg again. Consolidate the two try_read1msg cases into one, bump refcnts to prevent lconn's from being freed prematurely. --- diff --git a/libraries/libldap/os-ip.c b/libraries/libldap/os-ip.c index bebd82382f..3fc414564b 100644 --- a/libraries/libldap/os-ip.c +++ b/libraries/libldap/os-ip.c @@ -957,6 +957,9 @@ ldap_is_read_ready( LDAP *ld, Sockbuf *sb ) sip = (struct selectinfo *)ld->ld_selectinfo; + if (ber_sockbuf_ctrl( sb, LBER_SB_OPT_DATA_READY, NULL )) + return 1; + ber_sockbuf_ctrl( sb, LBER_SB_OPT_GET_FD, &sd ); #ifdef HAVE_POLL diff --git a/libraries/libldap/result.c b/libraries/libldap/result.c index 10ca5b7d3c..21f02c9831 100644 --- a/libraries/libldap/result.c +++ b/libraries/libldap/result.c @@ -71,7 +71,7 @@ static int ldap_mark_abandoned LDAP_P(( LDAP *ld, ber_int_t msgid, int idx )); static int wait4msg LDAP_P(( LDAP *ld, ber_int_t msgid, int all, struct timeval *timeout, LDAPMessage **result )); static ber_tag_t try_read1msg LDAP_P(( LDAP *ld, ber_int_t msgid, - int all, LDAPConn **lc, LDAPMessage **result )); + int all, LDAPConn *lc, LDAPMessage **result )); static ber_tag_t build_result_ber LDAP_P(( LDAP *ld, BerElement **bp, LDAPRequest *lr )); static void merge_error_info LDAP_P(( LDAP *ld, LDAPRequest *parentr, LDAPRequest *lr )); static LDAPMessage * chkResponseList LDAP_P(( LDAP *ld, int msgid, int all)); @@ -106,7 +106,7 @@ ldap_result( struct timeval *timeout, LDAPMessage **result ) { - LDAPMessage *lm = NULL; + LDAPMessage *lm; int rc; assert( ld != NULL ); @@ -118,19 +118,7 @@ ldap_result( ldap_pvt_thread_mutex_lock( &ld->ld_res_mutex ); #endif -#if 0 - /* this is already done inside wait4msg(), right?... */ - lm = chkResponseList( ld, msgid, all ); -#endif - - if ( lm == NULL ) { - rc = wait4msg( ld, msgid, all, timeout, result ); - - } else { - *result = lm; - ld->ld_errno = LDAP_SUCCESS; - rc = lm->lm_msgtype; - } + rc = wait4msg( ld, msgid, all, timeout, result ); #ifdef LDAP_R_COMPILE ldap_pvt_thread_mutex_unlock( &ld->ld_res_mutex ); @@ -335,13 +323,6 @@ wait4msg( if ( ber_sockbuf_ctrl( lc->lconn_sb, LBER_SB_OPT_DATA_READY, NULL ) ) { -#ifdef LDAP_R_COMPILE - ldap_pvt_thread_mutex_unlock( &ld->ld_conn_mutex ); -#endif - rc = try_read1msg( ld, msgid, all, &lc, result ); -#ifdef LDAP_R_COMPILE - ldap_pvt_thread_mutex_lock( &ld->ld_conn_mutex ); -#endif lc_ready = 1; break; } @@ -375,54 +356,63 @@ wait4msg( rc = LDAP_MSG_X_KEEP_LOOKING; /* select interrupted: loop */ } else { - rc = LDAP_MSG_X_KEEP_LOOKING; + lc_ready = 1; + } + } + if ( lc_ready ) { + LDAPConn *lnext; + rc = LDAP_MSG_X_KEEP_LOOKING; #ifdef LDAP_R_COMPILE - ldap_pvt_thread_mutex_lock( &ld->ld_req_mutex ); + ldap_pvt_thread_mutex_lock( &ld->ld_req_mutex ); #endif - if ( ld->ld_requests && - ld->ld_requests->lr_status == LDAP_REQST_WRITING && - ldap_is_write_ready( ld, - ld->ld_requests->lr_conn->lconn_sb ) ) - { - ldap_int_flush_request( ld, ld->ld_requests ); - } + if ( ld->ld_requests && + ld->ld_requests->lr_status == LDAP_REQST_WRITING && + ldap_is_write_ready( ld, + ld->ld_requests->lr_conn->lconn_sb ) ) + { + ldap_int_flush_request( ld, ld->ld_requests ); + } #ifdef LDAP_R_COMPILE - ldap_pvt_thread_mutex_unlock( &ld->ld_req_mutex ); - ldap_pvt_thread_mutex_lock( &ld->ld_conn_mutex ); + ldap_pvt_thread_mutex_unlock( &ld->ld_req_mutex ); + ldap_pvt_thread_mutex_lock( &ld->ld_conn_mutex ); #endif - for ( lc = ld->ld_conns; - rc == LDAP_MSG_X_KEEP_LOOKING && lc != NULL; ) + for ( lc = ld->ld_conns; + rc == LDAP_MSG_X_KEEP_LOOKING && lc != NULL; + lc = lnext ) + { + if ( lc->lconn_status == LDAP_CONNST_CONNECTED && + ldap_is_read_ready( ld, lc->lconn_sb ) ) { - if ( lc->lconn_status == LDAP_CONNST_CONNECTED && - ldap_is_read_ready( ld, lc->lconn_sb ) ) - { + /* Don't let it get freed out from under us */ + ++lc->lconn_refcnt; #ifdef LDAP_R_COMPILE - ldap_pvt_thread_mutex_unlock( &ld->ld_conn_mutex ); + ldap_pvt_thread_mutex_unlock( &ld->ld_conn_mutex ); #endif - rc = try_read1msg( ld, msgid, all, &lc, result ); + rc = try_read1msg( ld, msgid, all, lc, result ); + lnext = lc->lconn_next; + + /* Only take locks if we're really freeing */ + if ( lc->lconn_refcnt <= 1 ) { #ifdef LDAP_R_COMPILE - ldap_pvt_thread_mutex_lock( &ld->ld_conn_mutex ); + ldap_pvt_thread_mutex_lock( &ld->ld_req_mutex ); #endif - if ( lc == NULL ) { - /* if lc gets free()'d, - * there's no guarantee - * lc->lconn_next is still - * sane; better restart - * (ITS#4405) */ - lc = ld->ld_conns; - - /* don't get to next conn! */ - break; - } + ldap_free_connection( ld, lc, 0, 1 ); +#ifdef LDAP_R_COMPILE + ldap_pvt_thread_mutex_unlock( &ld->ld_req_mutex ); +#endif + } else { + --lc->lconn_refcnt; } - - /* next conn */ - lc = lc->lconn_next; - } #ifdef LDAP_R_COMPILE - ldap_pvt_thread_mutex_unlock( &ld->ld_conn_mutex ); + ldap_pvt_thread_mutex_lock( &ld->ld_conn_mutex ); #endif + } else { + lnext = lc->lconn_next; + } } +#ifdef LDAP_R_COMPILE + ldap_pvt_thread_mutex_unlock( &ld->ld_conn_mutex ); +#endif } } @@ -482,7 +472,7 @@ try_read1msg( LDAP *ld, ber_int_t msgid, int all, - LDAPConn **lcp, + LDAPConn *lc, LDAPMessage **result ) { BerElement *ber; @@ -493,7 +483,6 @@ try_read1msg( ber_len_t len; int foundit = 0; LDAPRequest *lr, *tmplr, dummy_lr = { 0 }; - LDAPConn *lc; BerElement tmpber; int rc, refer_cnt, hadref, simple_request, err; ber_int_t lderr; @@ -504,8 +493,7 @@ try_read1msg( #endif assert( ld != NULL ); - assert( lcp != NULL ); - assert( *lcp != NULL ); + assert( lc != NULL ); #ifdef LDAP_R_COMPILE LDAP_PVT_THREAD_ASSERT_MUTEX_OWNER( &ld->ld_res_mutex ); @@ -514,8 +502,6 @@ try_read1msg( Debug( LDAP_DEBUG_TRACE, "read1msg: ld %p msgid %d all %d\n", (void *)ld, msgid, all ); - lc = *lcp; - retry: if ( lc->lconn_ber == NULL ) { lc->lconn_ber = ldap_alloc_ber_with_options( ld ); @@ -561,14 +547,8 @@ nextresp3: if ( err == EAGAIN ) return LDAP_MSG_X_KEEP_LOOKING; #endif ld->ld_errno = LDAP_SERVER_DOWN; -#ifdef LDAP_R_COMPILE - ldap_pvt_thread_mutex_lock( &ld->ld_req_mutex ); -#endif - ldap_free_connection( ld, lc, 1, 0 ); -#ifdef LDAP_R_COMPILE - ldap_pvt_thread_mutex_unlock( &ld->ld_req_mutex ); -#endif - lc = *lcp = NULL; + --lc->lconn_refcnt; + lc->lconn_status = 0; return -1; default: @@ -938,14 +918,8 @@ nextresp2: * shouldn't necessarily end the connection */ if ( lc != NULL && id != 0 ) { -#ifdef LDAP_R_COMPILE - ldap_pvt_thread_mutex_lock( &ld->ld_req_mutex ); -#endif - ldap_free_connection( ld, lc, 0, 1 ); -#ifdef LDAP_R_COMPILE - ldap_pvt_thread_mutex_unlock( &ld->ld_req_mutex ); -#endif - lc = *lcp = NULL; + --lc->lconn_refcnt; + lc = NULL; } } } @@ -1011,14 +985,7 @@ nextresp2: /* get rid of the connection... */ if ( lc != NULL ) { -#ifdef LDAP_R_COMPILE - ldap_pvt_thread_mutex_lock( &ld->ld_req_mutex ); -#endif - ldap_free_connection( ld, lc, 0, 1 ); -#ifdef LDAP_R_COMPILE - ldap_pvt_thread_mutex_unlock( &ld->ld_req_mutex ); -#endif - lc = *lcp = NULL; + --lc->lconn_refcnt; } /* need to return -1, because otherwise