Ticket 47759 - Crash in replication when server is under write load

Bug Description:  When the server is under alot of load, a race condition allows
                  a replication connection LDAP struct to be freed(unbind) while
                  it is being used by another thread.  This leads to a crash.

Fix Description:  Extend the connection lock to also cover ldap client interaction
                  (e.g. conn->ld struct).

https://fedorahosted.org/389/ticket/47759

Reviewed by: nhosoi & rmeggins(Thanks!!)

ldap/servers/plugins/replication/repl5_connection.c

@@ -142,6 +142,7 @@ static void repl5_debug_timeout_callback(time_t when, void *arg);
 
 /* Forward declarations */
 static void close_connection_internal(Repl_Connection *conn);
+static void conn_delete_internal(Repl_Connection *conn);
 
 /*
  * Create a new connection object. Returns a pointer to the object, or
@@ -186,11 +187,22 @@ conn_new(Repl_Agmt *agmt)
 	rpc->plain = NULL;
 	return rpc;
 loser:
-	conn_delete(rpc);
+	conn_delete_internal(rpc);
 	slapi_ch_free((void**)&rpc);
 	return NULL;
 }
 
+static PRBool
+conn_connected_locked(Repl_Connection *conn, int locked)
+{
+	PRBool return_value;
+
+	if(!locked) PR_Lock(conn->lock);
+	return_value = STATE_CONNECTED == conn->state;
+	if(!locked) PR_Unlock(conn->lock);
+
+	return return_value;
+}
 
 /*
  * Return PR_TRUE if the connection is in the connected state
@@ -198,14 +210,9 @@ loser:
 static PRBool
 conn_connected(Repl_Connection *conn)
 {
-	PRBool return_value;
-	PR_Lock(conn->lock);
-	return_value = STATE_CONNECTED == conn->state;
-	PR_Unlock(conn->lock);
-	return return_value;
+	return conn_connected_locked(conn, 1);
 }
 
-
 /*
  * Destroy a connection object.
  */
@@ -247,7 +254,6 @@ conn_delete(Repl_Connection *conn)
 		if (slapi_eq_cancel(conn->linger_event) == 1)
 		{
 			/* Event was found and cancelled. Destroy the connection object. */
-			PR_Unlock(conn->lock);
 			destroy_it = PR_TRUE;
 		}
 		else
@@ -258,16 +264,15 @@ conn_delete(Repl_Connection *conn)
 			 * off, so arrange for the event to destroy the object .
 			 */
 			conn->delete_after_linger = PR_TRUE;
-			PR_Unlock(conn->lock);
 		}
 	}
 	if (destroy_it)
 	{
 		conn_delete_internal(conn);
 	}
+	PR_Unlock(conn->lock);
 }
 
-
 /*
  * Return the last operation type processed by the connection
  * object, and the LDAP error encountered.
@@ -331,17 +336,18 @@ conn_read_result_ex(Repl_Connection *conn, char **retoidp, struct berval **retda
 			while (!slapi_is_shutting_down())
 			{
 				/* we have to make sure the update sending thread does not
-				   attempt to call conn_disconnect while we are reading
+				   attempt to close connection while we are reading
 				   results - so lock the conn while we get the results */
 				PR_Lock(conn->lock);
+
 				if ((STATE_CONNECTED != conn->state) || !conn->ld) {
 					rc = -1;
 					return_value = CONN_NOT_CONNECTED;
 					PR_Unlock(conn->lock);
 					break;
 				}
-
 				rc = ldap_result(conn->ld, send_msgid, 1, &local_timeout, &res);
+
 				PR_Unlock(conn->lock);
 
 				if (0 != rc)
@@ -665,8 +671,10 @@ perform_operation(Repl_Connection *conn, int optype, const char *dn,
 	server_controls[1] = update_control;
 	server_controls[2] = NULL;
 
-	/* lock the conn to prevent the result reader thread
-	   from closing the connection out from under us */
+	/*
+	 * Lock the conn to prevent the result reader thread
+	 * from closing the connection out from under us.
+	 */
 	PR_Lock(conn->lock);
 	if (STATE_CONNECTED == conn->state)
 	{
@@ -808,7 +816,6 @@ conn_send_rename(Repl_Connection *conn, const char *dn,
 		NULL /* extop OID */, NULL /* extop payload */, message_id);
 }
 
-
 /*
  * Send an LDAP extended operation.
  */
@@ -822,7 +829,6 @@ conn_send_extended_operation(Repl_Connection *conn, const char *extop_oid,
 		update_control, extop_oid, payload, message_id);
 }
 
-
 /*
  * Synchronously read an entry and return a specific attribute's values.
  * Returns CONN_OPERATION_SUCCESS if successful. Returns
@@ -842,6 +848,8 @@ conn_read_entry_attribute(Repl_Connection *conn, const char *dn,
 	LDAPMessage *res = NULL;
 	char *attrs[2];
 
+	PR_Lock(conn->lock);
+
 	PR_ASSERT(NULL != type);
 	if (conn_connected(conn))
 	{
@@ -864,7 +872,7 @@ conn_read_entry_attribute(Repl_Connection *conn, const char *dn,
 		}
 		else if (IS_DISCONNECT_ERROR(ldap_rc))
 		{
-			conn_disconnect(conn);
+			close_connection_internal(conn);
 			return_value = CONN_NOT_CONNECTED;
 		}
 		else
@@ -882,10 +890,11 @@ conn_read_entry_attribute(Repl_Connection *conn, const char *dn,
 	{
 		return_value = CONN_NOT_CONNECTED;
 	}
+	PR_Unlock(conn->lock);
+
 	return return_value;
 }
 
-
 /*
  * Return an pointer to a string describing the connection's status.
 */
@@ -896,8 +905,6 @@ conn_get_status(Repl_Connection *conn)
 	return conn->status;
 }
 
-
-
 /*
  * Cancel any outstanding linger timer. Should be called when
  * a replication session is beginning.
@@ -929,7 +936,6 @@ conn_cancel_linger(Repl_Connection *conn)
 	PR_Unlock(conn->lock);
 }
 
-
 /*
  * Called when our linger timeout timer expires. This means
  * we should check to see if perhaps the connection's become
@@ -961,7 +967,6 @@ linger_timeout(time_t event_time, void *arg)
 	}
 }
 
-
 /*
  * Indicate that a session is ending. The linger timer starts when
  * this function is called.
@@ -999,8 +1004,6 @@ conn_start_linger(Repl_Connection *conn)
 	PR_Unlock(conn->lock);
 }
 
-
-
 /*
  * If no connection is currently active, opens a connection and binds to
  * the remote server. If a connection is open (e.g. lingering) then
@@ -1019,10 +1022,14 @@ conn_connect(Repl_Connection *conn)
 	ConnResult return_value = CONN_OPERATION_SUCCESS;
 	int pw_ret = 1;
 
-	/** Connection already open just return SUCCESS **/
-	if(conn->state == STATE_CONNECTED) goto done;
-
 	PR_Lock(conn->lock);
+
+	/* Connection already open, just return SUCCESS */
+	if(conn->state == STATE_CONNECTED){
+		PR_Unlock(conn->lock);
+		return return_value;
+	}
+
 	if (conn->flag_agmt_changed) {
 		/* So far we cannot change Hostname and Port */
 		/* slapi_ch_free((void **)&conn->hostname); */
@@ -1037,7 +1044,6 @@ conn_connect(Repl_Connection *conn)
 		conn->port = agmt_get_port(conn->agmt); /* port could be updated */
 		slapi_ch_free((void **)&conn->plain);
 	}
-	PR_Unlock(conn->lock);
 
 	creds = agmt_get_credentials(conn->agmt);
 
@@ -1178,6 +1184,7 @@ done:
 	{
 		close_connection_internal(conn);
 	}
+	PR_Unlock(conn->lock);
 
 	return return_value;
 }
@@ -1213,7 +1220,6 @@ conn_disconnect(Repl_Connection *conn)
 	PR_Unlock(conn->lock);
 }
 
-
 /*
  * Determine if the remote replica supports DS 5.0 replication.
  * Return codes:
@@ -1230,6 +1236,7 @@ conn_replica_supports_ds5_repl(Repl_Connection *conn)
 	ConnResult return_value;
 	int ldap_rc;
 
+	PR_Lock(conn->lock);
 	if (conn_connected(conn))
 	{
 		if (conn->supports_ds50_repl == -1) {
@@ -1277,7 +1284,7 @@ conn_replica_supports_ds5_repl(Repl_Connection *conn)
 				if (IS_DISCONNECT_ERROR(ldap_rc))
 				{
 					conn->last_ldap_error = ldap_rc;	/* specific reason */
-					conn_disconnect(conn);
+					close_connection_internal(conn);
 					return_value = CONN_NOT_CONNECTED;
 				}
 				else
@@ -1297,10 +1304,11 @@ conn_replica_supports_ds5_repl(Repl_Connection *conn)
 		/* Not connected */
 		return_value = CONN_NOT_CONNECTED;
 	}
+	PR_Unlock(conn->lock);
+
 	return return_value;
 }
 
-
 /*
  * Determine if the remote replica supports DS 7.1 replication.
  * Return codes:
@@ -1317,6 +1325,7 @@ conn_replica_supports_ds71_repl(Repl_Connection *conn)
 	ConnResult return_value;
 	int ldap_rc;
 
+	PR_Lock(conn->lock);
 	if (conn_connected(conn))
 	{
 		if (conn->supports_ds71_repl == -1) {
@@ -1348,7 +1357,7 @@ conn_replica_supports_ds71_repl(Repl_Connection *conn)
 				if (IS_DISCONNECT_ERROR(ldap_rc))
 				{
 					conn->last_ldap_error = ldap_rc;	/* specific reason */
-					conn_disconnect(conn);
+					close_connection_internal(conn);
 					return_value = CONN_NOT_CONNECTED;
 				}
 				else
@@ -1368,6 +1377,8 @@ conn_replica_supports_ds71_repl(Repl_Connection *conn)
 		/* Not connected */
 		return_value = CONN_NOT_CONNECTED;
 	}
+	PR_Unlock(conn->lock);
+
 	return return_value;
 }
 
@@ -1387,6 +1398,7 @@ conn_replica_supports_ds90_repl(Repl_Connection *conn)
 	ConnResult return_value;
 	int ldap_rc;
 
+	PR_Lock(conn->lock);
 	if (conn_connected(conn))
 	{
 		if (conn->supports_ds90_repl == -1) {
@@ -1418,7 +1430,7 @@ conn_replica_supports_ds90_repl(Repl_Connection *conn)
 				if (IS_DISCONNECT_ERROR(ldap_rc))
 				{
 					conn->last_ldap_error = ldap_rc;        /* specific reason */
-					conn_disconnect(conn);
+					close_connection_internal(conn);
 					return_value = CONN_NOT_CONNECTED;
 				}
 				else
@@ -1427,7 +1439,7 @@ conn_replica_supports_ds90_repl(Repl_Connection *conn)
 				}
 			}
 			if (NULL != res)
-                                ldap_msgfree(res);
+				ldap_msgfree(res);
 		}
 		else
 		{
@@ -1439,6 +1451,8 @@ conn_replica_supports_ds90_repl(Repl_Connection *conn)
 		/* Not connected */
 		return_value = CONN_NOT_CONNECTED;
 	}
+	PR_Unlock(conn->lock);
+
 	return return_value;
 }
 
@@ -1456,7 +1470,6 @@ conn_replica_is_readonly(Repl_Connection *conn)
 	}
 }
 
-
 /*
  * Return 1 if "value" is a value of attribute type "type" in entry "entry".
  * Otherwise, return 0.
@@ -1505,9 +1518,6 @@ attribute_string_value_present(LDAP *ld, LDAPMessage *entry, const char *type,
 	return return_value;
 }
 
-
-
-
 /*
  * Read the remote server's schema entry, then read the local schema entry,
  * and compare the nsschemacsn attribute. If the local csn is newer, or
@@ -1537,7 +1547,7 @@ conn_push_schema(Repl_Connection *conn, CSN **remotecsn)
 		return_value = CONN_OPERATION_FAILED;
 		slapi_log_error(SLAPI_LOG_FATAL, repl_plugin_name, "NULL remote CSN\n");
 	}
-	else if (!conn_connected(conn))
+	else if (!conn_connected_locked(conn, 0 /* not locked */))
 	{
 		return_value = CONN_NOT_CONNECTED;
 		slapi_log_error(SLAPI_LOG_REPL, repl_plugin_name,
@@ -1754,6 +1764,7 @@ conn_push_schema(Repl_Connection *conn, CSN **remotecsn)
 	{
 		csn_free(&localcsn);
 	}
+
 	return return_value;
 }
 

ldap/servers/slapd/ldaputil.c

@@ -1044,8 +1044,8 @@ slapi_ldap_bind(
        than the currently unused clientctrls */
     ldap_get_option(ld, LDAP_OPT_CLIENT_CONTROLS, &clientctrls);
     if (clientctrls && clientctrls[0] &&
-	slapi_control_present(clientctrls, START_TLS_OID, NULL, NULL)) {
-	secure = 2;
+        slapi_control_present(clientctrls, START_TLS_OID, NULL, NULL)) {
+        secure = 2;
     } else {
 #if defined(USE_OPENLDAP)
 	/* openldap doesn't have a SSL/TLS yes/no flag - so grab the
@@ -1084,12 +1084,12 @@ slapi_ldap_bind(
 	    slapi_log_error(SLAPI_LOG_SHELL, "slapi_ldap_bind",
 			    "Set up conn to use client auth\n");
 	}
-	bvcreds.bv_val = NULL; /* ignore username and passed in creds */
-	bvcreds.bv_len = 0; /* for external auth */
-	bindid = NULL;
+        bvcreds.bv_val = NULL; /* ignore username and passed in creds */
+        bvcreds.bv_len = 0; /* for external auth */
+        bindid = NULL;
     } else { /* other type of auth */
-	bvcreds.bv_val = (char *)creds;
-	bvcreds.bv_len = creds ? strlen(creds) : 0;
+        bvcreds.bv_val = (char *)creds;
+        bvcreds.bv_len = creds ? strlen(creds) : 0;
     }
 
     if (secure == 2) { /* send start tls */
@@ -1117,31 +1117,29 @@ slapi_ldap_bind(
 			bindid, creds);
 	if ((rc = ldap_sasl_bind(ld, bindid, mech, &bvcreds, serverctrls,
 	                         NULL /* clientctrls */, &mymsgid))) {
-	    char *myhostname = NULL;
-	    char *copy = NULL;
+	    char *hostname = NULL;
+	    char *host_port = NULL;
 	    char *ptr = NULL;
 	    int myerrno = errno;
 	    int gaierr = 0;
 
-	    ldap_get_option(ld, LDAP_OPT_HOST_NAME, &myhostname);
-	    if (myhostname) {
-	        ptr = strchr(myhostname, ':');
+	    ldap_get_option(ld, LDAP_OPT_HOST_NAME, &host_port);
+	    if (host_port) {
+	        ptr = strchr(host_port, ':');
 	        if (ptr) {
-	            copy = slapi_ch_strdup(myhostname);
-	            *(copy + (ptr - myhostname)) = '\0';
-	            slapi_ch_free_string(&myhostname);
-	            myhostname = copy;
+	            hostname = slapi_ch_strdup(host_port);
+	            *(hostname + (ptr - host_port)) = '\0';
 	        }
 	    }
-
 	    if (0 == myerrno) {
 	        struct addrinfo *result = NULL;
-	        gaierr = getaddrinfo(myhostname, NULL, NULL, &result);
+	        gaierr = getaddrinfo(hostname, NULL, NULL, &result);
 	        myerrno = errno;
 	        if (result) {
 	            freeaddrinfo(result);
 	        }
 	    }
+
 	    slapi_log_error(SLAPI_LOG_FATAL, "slapi_ldap_bind",
 			    "Error: could not send bind request for id "
 			    "[%s] authentication mechanism [%s]: error %d (%s), system error %d (%s), "
@@ -1152,8 +1150,9 @@ slapi_ldap_bind(
 			    PR_GetError(), slapd_pr_strerror(PR_GetError()),
 			    myerrno ? myerrno : gaierr,
 			    myerrno ? slapd_system_strerror(myerrno) : gai_strerror(gaierr),
-			    myhostname ? myhostname : "unknown host");
-	    slapi_ch_free_string(&myhostname);
+			    host_port ? host_port : "unknown host");
+	    slapi_ch_free_string(&hostname);
+	    slapi_ch_free_string(&host_port);
 	    goto done;
 	}