From: Ondřej Kuzník <ondra@openldap.org>
Date: Fri, 29 Mar 2019 13:29:07 +0000 (+0000)
Subject: ITS#9001 Use a TAvl for request tracking in libldap
X-Git-Tag: OPENLDAP_REL_ENG_2_5_3BETA~3^2~5
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3bd1b0909ac8538a138b77dbdd408fe240362991;p=thirdparty%2Fopenldap.git

ITS#9001 Use a TAvl for request tracking in libldap
---

diff --git a/libraries/libldap/abandon.c b/libraries/libldap/abandon.c
index d88f63a849..9c5ee043a9 100644
--- a/libraries/libldap/abandon.c
+++ b/libraries/libldap/abandon.c
@@ -40,7 +40,7 @@ static int
 do_abandon(
 	LDAP *ld,
 	ber_int_t origid,
-	ber_int_t msgid,
+	LDAPRequest *lr,
 	LDAPControl **sctrls,
 	int sendabandon );
 
@@ -75,7 +75,7 @@ ldap_abandon_ext(
 
 	rc = ldap_int_client_controls( ld, cctrls );
 	if ( rc == LDAP_SUCCESS ) {
-		rc = do_abandon( ld, msgid, msgid, sctrls, 1 );
+		rc = do_abandon( ld, msgid, NULL, sctrls, 1 );
 	}
 
 	LDAP_MUTEX_UNLOCK( &ld->ld_req_mutex );
@@ -112,7 +112,7 @@ ldap_pvt_discard(
 	int	rc;
 
 	LDAP_MUTEX_LOCK( &ld->ld_req_mutex );
-	rc = do_abandon( ld, msgid, msgid, NULL, 0 );
+	rc = do_abandon( ld, msgid, NULL, NULL, 0 );
 	LDAP_MUTEX_UNLOCK( &ld->ld_req_mutex );
 	return rc;
 }
@@ -121,49 +121,52 @@ static int
 do_abandon(
 	LDAP *ld,
 	ber_int_t origid,
-	ber_int_t msgid,
+	LDAPRequest *lr,
 	LDAPControl **sctrls,
 	int sendabandon )
 {
 	BerElement	*ber;
 	int		i, err;
+	ber_int_t	msgid = origid;
 	Sockbuf		*sb;
-	LDAPRequest	*lr;
-
-	Debug2( LDAP_DEBUG_TRACE, "do_abandon origid %d, msgid %d\n",
-		origid, msgid );
-
-	/* find the request that we are abandoning */
-start_again:;
-	lr = ld->ld_requests;
-	while ( lr != NULL ) {
-		/* this message */
-		if ( lr->lr_msgid == msgid ) {
-			break;
-		}
-
-		/* child: abandon it */
-		if ( lr->lr_origid == msgid && !lr->lr_abandoned ) {
-			(void)do_abandon( ld, lr->lr_origid, lr->lr_msgid,
-				sctrls, sendabandon );
-
-			/* restart, as lr may now be dangling... */
-			goto start_again;
-		}
+	LDAPRequest	needle = {0};
 
-		lr = lr->lr_next;
-	}
+	needle.lr_msgid = origid;
 
 	if ( lr != NULL ) {
-		if ( origid == msgid && lr->lr_parent != NULL ) {
+		msgid = lr->lr_msgid;
+		Debug2( LDAP_DEBUG_TRACE, "do_abandon origid %d, msgid %d\n",
+				origid, msgid );
+	} else if ( (lr = ldap_tavl_find( ld->ld_requests, &needle, ldap_req_cmp )) != NULL ) {
+		Debug2( LDAP_DEBUG_TRACE, "do_abandon origid %d, msgid %d\n",
+				origid, msgid );
+		if ( lr->lr_parent != NULL ) {
 			/* don't let caller abandon child requests! */
 			ld->ld_errno = LDAP_PARAM_ERROR;
 			return( LDAP_PARAM_ERROR );
 		}
+		msgid = lr->lr_msgid;
+	}
+
+	if ( lr != NULL ) {
+		LDAPRequest **childp = &lr->lr_child;
+
+		needle.lr_msgid = lr->lr_msgid;
+
 		if ( lr->lr_status != LDAP_REQST_INPROGRESS ) {
 			/* no need to send abandon message */
 			sendabandon = 0;
 		}
+
+		while ( *childp ) {
+			/* Abandon children */
+			LDAPRequest *child = *childp;
+
+			(void)do_abandon( ld, lr->lr_origid, child, sctrls, sendabandon );
+			if ( *childp == child ) {
+				childp = &child->lr_refnext;
+			}
+		}
 	}
 
 	/* ldap_msgdelete locks the res_mutex. Give up the req_mutex
@@ -179,12 +182,7 @@ start_again:;
 
 	/* fetch again the request that we are abandoning */
 	if ( lr != NULL ) {
-		for ( lr = ld->ld_requests; lr != NULL; lr = lr->lr_next ) {
-			/* this message */
-			if ( lr->lr_msgid == msgid ) {
-				break;
-			}
-		}
+		lr = ldap_tavl_find( ld->ld_requests, &needle, ldap_req_cmp );
 	}
 
 	err = 0;
diff --git a/libraries/libldap/ldap-int.h b/libraries/libldap/ldap-int.h
index e40518b32d..06268a96f2 100644
--- a/libraries/libldap/ldap-int.h
+++ b/libraries/libldap/ldap-int.h
@@ -26,6 +26,7 @@
 
 #include "../liblber/lber-int.h"
 #include "lutil.h"
+#include "ldap_avl.h"
 
 #ifdef LDAP_R_COMPILE
 #include <ldap_pvt_thread.h>
@@ -438,7 +439,7 @@ struct ldap_common {
 
 	/* do not mess with these */
 	/* protected by req_mutex */
-	LDAPRequest	*ldc_requests;	/* list of outstanding requests */
+	TAvlnode	*ldc_requests;	/* list of outstanding requests */
 	/* protected by res_mutex */
 	LDAPMessage	*ldc_responses;	/* list of outstanding responses */
 #define	ld_requests		ldc->ldc_requests
@@ -776,6 +777,8 @@ LDAP_F (LDAPConn *) ldap_new_connection( LDAP *ld, LDAPURLDesc **srvlist,
 	int use_ldsb, int connect, LDAPreqinfo *bind, int m_req, int m_res );
 LDAP_F (LDAPRequest *) ldap_find_request_by_msgid( LDAP *ld, ber_int_t msgid );
 LDAP_F (void) ldap_return_request( LDAP *ld, LDAPRequest *lr, int freeit );
+LDAP_F (int) ldap_req_cmp( const void *l, const void *r );
+LDAP_F (void) ldap_do_free_request( void *arg );
 LDAP_F (void) ldap_free_request( LDAP *ld, LDAPRequest *lr );
 LDAP_F (void) ldap_free_connection( LDAP *ld, LDAPConn *lc, int force, int unbind );
 LDAP_F (void) ldap_dump_connection( LDAP *ld, LDAPConn *lconns, int all );
diff --git a/libraries/libldap/open.c b/libraries/libldap/open.c
index fdded08149..1289ced907 100644
--- a/libraries/libldap/open.c
+++ b/libraries/libldap/open.c
@@ -574,7 +574,8 @@ ldap_open_internal_connection( LDAP **ldp, ber_socket_t *fdp )
 	lr->lr_status = LDAP_REQST_INPROGRESS;
 	lr->lr_res_errno = LDAP_SUCCESS;
 	/* no mutex lock needed, we just created this ld here */
-	ld->ld_requests = lr;
+	rc = ldap_tavl_insert( &ld->ld_requests, lr, ldap_req_cmp, ldap_avl_dup_error );
+	assert( rc == LDAP_SUCCESS );
 
 	LDAP_MUTEX_LOCK( &ld->ld_conn_mutex );
 	/* Attach the passed socket as the *LDAP's connection */
diff --git a/libraries/libldap/request.c b/libraries/libldap/request.c
index e60139e8e0..73ce725b8a 100644
--- a/libraries/libldap/request.c
+++ b/libraries/libldap/request.c
@@ -328,11 +328,16 @@ ldap_send_server_request(
 	 * request to be in WRITING state.
 	 */
 	rc = 0;
-	if ( ld->ld_requests &&
-		ld->ld_requests->lr_status == LDAP_REQST_WRITING &&
-		ldap_int_flush_request( ld, ld->ld_requests ) < 0 )
-	{
-		rc = -1;
+	if ( ld->ld_requests != NULL ) {
+		TAvlnode *node = ldap_tavl_end( ld->ld_requests, TAVL_DIR_RIGHT );
+		LDAPRequest *lr;
+
+		assert( node != NULL );
+		lr = node->avl_data;
+		if ( lr->lr_status == LDAP_REQST_WRITING &&
+				ldap_int_flush_request( ld, lr ) < 0 ) {
+			rc = -1;
+		}
 	}
 	if ( rc ) {
 		ber_free( ber, 1 );
@@ -399,12 +404,8 @@ ldap_send_server_request(
 		}
 	}
 
-	lr->lr_prev = NULL;
-	lr->lr_next = ld->ld_requests;
-	if ( lr->lr_next != NULL ) {
-		lr->lr_next->lr_prev = lr;
-	}
-	ld->ld_requests = lr;
+	rc = ldap_tavl_insert( &ld->ld_requests, lr, ldap_req_cmp, ldap_avl_dup_error );
+	assert( rc == LDAP_SUCCESS );
 
 	ld->ld_errno = LDAP_SUCCESS;
 	if ( ldap_int_flush_request( ld, lr ) == -1 ) {
@@ -803,19 +804,10 @@ ldap_free_connection( LDAP *ld, LDAPConn *lc, int force, int unbind )
 		/* FIXME: is this at all possible?
 		 * ldap_ld_free() in unbind.c calls ldap_free_connection()
 		 * with force == 1 __after__ explicitly calling
-		 * ldap_free_request() on all requests */
+		 * ldap_tavl_free on ld->ld_requests */
 		if ( force ) {
-			LDAPRequest	*lr;
-
-			for ( lr = ld->ld_requests; lr; ) {
-				LDAPRequest	*lr_next = lr->lr_next;
-
-				if ( lr->lr_conn == lc ) {
-					ldap_free_request_int( ld, lr );
-				}
-
-				lr = lr_next;
-			}
+			ldap_tavl_free( ld->ld_requests, ldap_do_free_request );
+			ld->ld_requests = NULL;
 		}
 
 		if ( lc->lconn_sb != ld->ld_sb ) {
@@ -913,17 +905,19 @@ ldap_dump_connection( LDAP *ld, LDAPConn *lconns, int all )
 void
 ldap_dump_requests_and_responses( LDAP *ld )
 {
-	LDAPRequest	*lr;
 	LDAPMessage	*lm, *l;
+	TAvlnode *node;
 	int		i;
 
 	Debug1( LDAP_DEBUG_TRACE, "** ld %p Outstanding Requests:\n",
 		(void *)ld );
-	lr = ld->ld_requests;
-	if ( lr == NULL ) {
+	node = ldap_tavl_end( ld->ld_requests, TAVL_DIR_LEFT );
+	if ( node == NULL ) {
 		Debug0( LDAP_DEBUG_TRACE, "   Empty\n" );
 	}
-	for ( i = 0; lr != NULL; lr = lr->lr_next, i++ ) {
+	for ( i = 0 ; node != NULL; i++, node = ldap_tavl_next( node, TAVL_DIR_RIGHT ) ) {
+		LDAPRequest	*lr = node->avl_data;
+
 		Debug3( LDAP_DEBUG_TRACE, " * msgid %d,  origid %d, status %s\n",
 			lr->lr_msgid, lr->lr_origid,
 			( lr->lr_status == LDAP_REQST_INPROGRESS ) ? "InProgress" :
@@ -959,39 +953,21 @@ ldap_dump_requests_and_responses( LDAP *ld )
 #endif /* LDAP_DEBUG */
 
 /* protected by req_mutex */
-static void
-ldap_free_request_int( LDAP *ld, LDAPRequest *lr )
+void
+ldap_do_free_request( void *arg )
 {
-	LDAP_ASSERT_MUTEX_OWNER( &ld->ld_req_mutex );
+	LDAPRequest *lr = arg;
+
+	Debug3( LDAP_DEBUG_TRACE, "ldap_do_free_request: "
+			"asked to free lr %p msgid %d refcnt %d\n",
+			lr, lr->lr_msgid, lr->lr_refcnt );
 	/* if lr_refcnt > 0, the request has been looked up 
 	 * by ldap_find_request_by_msgid(); if in the meanwhile
 	 * the request is free()'d by someone else, just decrease
-	 * the reference count and extract it from the request
-	 * list; later on, it will be freed. */
-	if ( lr->lr_prev == NULL ) {
-		if ( lr->lr_refcnt == 0 ) {
-			/* free'ing the first request? */
-			assert( ld->ld_requests == lr );
-		}
-
-		if ( ld->ld_requests == lr ) {
-			ld->ld_requests = lr->lr_next;
-		}
-
-	} else {
-		lr->lr_prev->lr_next = lr->lr_next;
-	}
-
-	if ( lr->lr_next != NULL ) {
-		lr->lr_next->lr_prev = lr->lr_prev;
-	}
-
+	 * the reference count; later on, it will be freed. */
 	if ( lr->lr_refcnt > 0 ) {
+		assert( lr->lr_refcnt == 1 );
 		lr->lr_refcnt = -lr->lr_refcnt;
-
-		lr->lr_prev = NULL;
-		lr->lr_next = NULL;
-
 		return;
 	}
 
@@ -1013,6 +989,29 @@ ldap_free_request_int( LDAP *ld, LDAPRequest *lr )
 	LDAP_FREE( lr );
 }
 
+int
+ldap_req_cmp( const void *l, const void *r )
+{
+	const LDAPRequest *left = l, *right = r;
+	return left->lr_msgid - right->lr_msgid;
+}
+
+/* protected by req_mutex */
+static void
+ldap_free_request_int( LDAP *ld, LDAPRequest *lr )
+{
+	LDAPRequest *removed;
+
+	LDAP_ASSERT_MUTEX_OWNER( &ld->ld_req_mutex );
+	removed = ldap_tavl_delete( &ld->ld_requests, lr, ldap_req_cmp );
+	assert( !removed || removed == lr );
+	Debug3( LDAP_DEBUG_TRACE, "ldap_free_request_int: "
+			"lr %p msgid %d%s removed\n",
+			lr, lr->lr_msgid, removed ? "" : " not" );
+
+	ldap_do_free_request( lr );
+}
+
 /* protected by req_mutex */
 void
 ldap_free_request( LDAP *ld, LDAPRequest *lr )
@@ -1662,19 +1661,24 @@ re_encode_request( LDAP *ld,
 LDAPRequest *
 ldap_find_request_by_msgid( LDAP *ld, ber_int_t msgid )
 {
-	LDAPRequest	*lr;
-
-	for ( lr = ld->ld_requests; lr != NULL; lr = lr->lr_next ) {
-		if ( lr->lr_status == LDAP_REQST_COMPLETED ) {
-			continue;	/* Skip completed requests */
-		}
-		if ( msgid == lr->lr_msgid ) {
-			lr->lr_refcnt++;
-			break;
-		}
+	LDAPRequest	*lr, needle = {0};
+	needle.lr_msgid = msgid;
+
+	lr = ldap_tavl_find( ld->ld_requests, &needle, ldap_req_cmp );
+	if ( lr != NULL && lr->lr_status != LDAP_REQST_COMPLETED ) {
+		/* try_read1msg is the only user at the moment and we would free it
+		 * multiple times if retrieving the request again */
+		assert( lr->lr_refcnt == 0 );
+		lr->lr_refcnt++;
+		Debug3( LDAP_DEBUG_TRACE, "ldap_find_request_by_msgid: "
+				"msgid %d, lr %p lr->lr_refcnt = %d\n",
+				msgid, lr, lr->lr_refcnt );
+		return lr;
 	}
 
-	return( lr );
+	Debug2( LDAP_DEBUG_TRACE, "ldap_find_request_by_msgid: "
+			"msgid %d, lr %p\n", msgid, lr );
+	return NULL;
 }
 
 /* protected by req_mutex */
@@ -1683,23 +1687,26 @@ ldap_return_request( LDAP *ld, LDAPRequest *lrx, int freeit )
 {
 	LDAPRequest	*lr;
 
-	for ( lr = ld->ld_requests; lr != NULL; lr = lr->lr_next ) {
-		if ( lr == lrx ) {
-			if ( lr->lr_refcnt > 0 ) {
-				lr->lr_refcnt--;
-
-			} else if ( lr->lr_refcnt < 0 ) {
-				lr->lr_refcnt++;
-				if ( lr->lr_refcnt == 0 ) {
-					lr = NULL;
-				}
+	lr = ldap_tavl_find( ld->ld_requests, lrx, ldap_req_cmp );
+	Debug2( LDAP_DEBUG_TRACE, "ldap_return_request: "
+			"lrx %p, lr %p\n", lrx, lr );
+	if ( lr ) {
+		assert( lr == lrx );
+		if ( lr->lr_refcnt > 0 ) {
+			lr->lr_refcnt--;
+		} else if ( lr->lr_refcnt < 0 ) {
+			lr->lr_refcnt++;
+			if ( lr->lr_refcnt == 0 ) {
+				lr = NULL;
 			}
-			break;
 		}
 	}
+	Debug3( LDAP_DEBUG_TRACE, "ldap_return_request: "
+			"lrx->lr_msgid %d, lrx->lr_refcnt is now %d, lr is %s present\n",
+			lrx->lr_msgid, lrx->lr_refcnt, lr ? "still" : "not" );
+	/* The request is not tracked anymore */
 	if ( lr == NULL ) {
 		ldap_free_request_int( ld, lrx );
-
 	} else if ( freeit ) {
 		ldap_free_request( ld, lrx );
 	}
diff --git a/libraries/libldap/result.c b/libraries/libldap/result.c
index a6977caefb..bc47ddafe0 100644
--- a/libraries/libldap/result.c
+++ b/libraries/libldap/result.c
@@ -344,13 +344,17 @@ wait4msg(
 				int serviced = 0;
 				rc = LDAP_MSG_X_KEEP_LOOKING;
 				LDAP_MUTEX_LOCK( &ld->ld_req_mutex );
-				if ( ld->ld_requests &&
-					ld->ld_requests->lr_status == LDAP_REQST_WRITING &&
-					ldap_is_write_ready( ld,
-						ld->ld_requests->lr_conn->lconn_sb ) )
-				{
-					serviced = 1;
-					ldap_int_flush_request( ld, ld->ld_requests );
+				if ( ld->ld_requests != NULL ) {
+					TAvlnode *node = ldap_tavl_end( ld->ld_requests, TAVL_DIR_RIGHT );
+					LDAPRequest *lr;
+
+					assert( node != NULL );
+					lr = node->avl_data;
+					if ( lr->lr_status == LDAP_REQST_WRITING &&
+							ldap_is_write_ready( ld, lr->lr_conn->lconn_sb ) ) {
+						serviced = 1;
+						ldap_int_flush_request( ld, lr );
+					}
 				}
 				for ( lc = ld->ld_conns;
 					rc == LDAP_MSG_X_KEEP_LOOKING && lc != NULL;
@@ -452,7 +456,7 @@ try_read1msg(
 	LDAPRequest	*lr, *tmplr, dummy_lr = { 0 };
 	BerElement	tmpber;
 	int		rc, refer_cnt, hadref, simple_request, err;
-	ber_int_t	lderr;
+	ber_int_t	lderr = -1;
 
 #ifdef LDAP_CONNECTIONLESS
 	LDAPMessage	*tmp = NULL, *chain_head = NULL;
diff --git a/libraries/libldap/unbind.c b/libraries/libldap/unbind.c
index 51f85b8162..ca89930d12 100644
--- a/libraries/libldap/unbind.c
+++ b/libraries/libldap/unbind.c
@@ -108,9 +108,8 @@ ldap_ld_free(
 
 	/* free LDAP structure and outstanding requests/responses */
 	LDAP_MUTEX_LOCK( &ld->ld_req_mutex );
-	while ( ld->ld_requests != NULL ) {
-		ldap_free_request( ld, ld->ld_requests );
-	}
+	ldap_tavl_free( ld->ld_requests, ldap_do_free_request );
+	ld->ld_requests = NULL;
 	LDAP_MUTEX_UNLOCK( &ld->ld_req_mutex );
 	LDAP_MUTEX_LOCK( &ld->ld_conn_mutex );