Re: Assertion failure in try_read1msg() (ITS#2982)

[lukeh@PADL.COM]

--19701020
Content-Type: text/plain; charset=US-ASCII
Content-Disposition: inline


It seems to me that if try_read1msg() is going to free lc, 
possibly dropping its reference count to zero, then it needs
to be passed a pointer from wait4msg() such that it can 
remove it from the linked list of connections. Otherwise
there is always the risk of a dangling pointer, which 
appears to be the issue here.

How is the attached patch?

-- Luke


--19701020
Content-Type: text/plain; name="result.c.diff"; x-unix-mode=0644
Content-Disposition: attachment; filename="result.c.diff"

Index: result.c
===================================================================
RCS file: /repo/OpenLDAP/pkg/ldap/libraries/libldap/result.c,v
retrieving revision 1.84.2.4
diff -u -r1.84.2.4 result.c
--- result.c	1 Jan 2004 18:16:30 -0000	1.84.2.4
+++ result.c	27 Feb 2004 22:54:58 -0000
@@ -73,7 +73,7 @@
 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, Sockbuf *sb, LDAPConn *lc, LDAPMessage **result ));
+	int all, Sockbuf *sb, 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));
@@ -261,7 +261,7 @@
 	struct timeval	tv, *tvp;
 	time_t		start_time = 0;
 	time_t		tmp_time;
-	LDAPConn	*lc, *nextlc;
+	LDAPConn	**lc, **nextlc;
 
 	assert( ld != NULL );
 	assert( result != NULL );
@@ -315,16 +315,20 @@
             rc = (*result)->lm_msgtype;
         } else {
 
-			for ( lc = ld->ld_conns; lc != NULL; lc = lc->lconn_next ) {
-				if ( ber_sockbuf_ctrl( lc->lconn_sb,
+			for ( lc = &ld->ld_conns; *lc != NULL; lc = nextlc ) {
+				nextlc = &((*lc)->lconn_next);
+				if ( ber_sockbuf_ctrl( (*lc)->lconn_sb,
 						LBER_SB_OPT_DATA_READY, NULL ) ) {
-					    rc = try_read1msg( ld, msgid, all, lc->lconn_sb,
+					    rc = try_read1msg( ld, msgid, all, (*lc)->lconn_sb,
 					        lc, result );
+					    if ( *lc == NULL ) {
+						*lc = *nextlc;
+					    }
 				    break;
 				}
 	        }
 
-		    if ( lc == NULL ) {
+		    if ( *lc == NULL ) {
 			    rc = ldap_int_select( ld, tvp );
 #ifdef LDAP_DEBUG
 			    if ( rc == -1 ) {
@@ -365,15 +369,18 @@
 #ifdef LDAP_R_COMPILE
 				    ldap_pvt_thread_mutex_unlock( &ld->ld_req_mutex );
 #endif
-				    for ( lc = ld->ld_conns; rc == -2 && lc != NULL;
+				    for ( lc = &ld->ld_conns; rc == -2 && *lc != NULL;
 				        lc = nextlc ) {
-					    nextlc = lc->lconn_next;
-					    if ( lc->lconn_status ==
+					    nextlc = &((*lc)->lconn_next);
+					    if ( (*lc)->lconn_status ==
 					        LDAP_CONNST_CONNECTED &&
 					        ldap_is_read_ready( ld,
-					        lc->lconn_sb )) {
+					        (*lc)->lconn_sb )) {
 						    rc = try_read1msg( ld, msgid, all,
-						        lc->lconn_sb, lc, result );
+						        (*lc)->lconn_sb, lc, result );
+						    if ( *lc == NULL ) {
+							*lc = *nextlc;
+						    }
 					    }
 				    }
 			    }
@@ -409,7 +416,7 @@
 	ber_int_t msgid,
 	int all,
 	Sockbuf *sb,
-	LDAPConn *lc,
+	LDAPConn **lcp,
 	LDAPMessage **result )
 {
 	BerElement	*ber;
@@ -419,6 +426,7 @@
 	ber_len_t	len;
 	int		foundit = 0;
 	LDAPRequest	*lr, *tmplr;
+	LDAPConn	*lc;
 	BerElement	tmpber;
 	int		rc, refer_cnt, hadref, simple_request;
 	ber_int_t	lderr;
@@ -432,7 +440,8 @@
 	int     v3ref;
 
 	assert( ld != NULL );
-	assert( lc != NULL );
+	assert( lcp != NULL );
+	assert( *lcp != NULL );
 	
 #ifdef NEW_LOGGING
 	LDAP_LOG ( OPERATION, ARGS, "read1msg: msgid %d, all %d\n", msgid, all, 0 );
@@ -440,6 +449,8 @@
 	Debug( LDAP_DEBUG_TRACE, "read1msg: msgid %d, all %d\n", msgid, all, 0 );
 #endif
 
+	lc = *lcp;
+
 retry:
 	if ( lc->lconn_ber == NULL ) {
 		lc->lconn_ber = ldap_alloc_ber_with_options(ld);
@@ -815,6 +826,9 @@
 
 			if ( lc != NULL ) {
 				ldap_free_connection( ld, lc, 0, 1 );
+				if ( lc->lconn_refcnt <= 0 ) {
+					*lcp = NULL;
+				}
 			}
 		}
 	}

--19701020--