FreeRADIUS 1.1.1 Segmentation fault on Fedora 4

[Alan DeKok]

"Alan DeKok" <aland at nitros9.org> wrote:
>   Sorry, sent the wrong patch.

  With a lock bug.  Dang.  I'll get it right one of these days.

  OK, This should work.

  Alan DeKok.
Index: src/modules/rlm_eap/eap.h
===================================================================
RCS file: /source/radiusd/src/modules/rlm_eap/eap.h,v
retrieving revision 1.27.4.1
diff -u -r1.27.4.1 eap.h
--- src/modules/rlm_eap/eap.h	6 Feb 2006 16:23:50 -0000	1.27.4.1
+++ src/modules/rlm_eap/eap.h	8 May 2006 21:53:07 -0000
@@ -121,6 +121,9 @@
 	int		status;
 
 	int		stage;
+
+	int		in_list;
+	void		*inst;
 } EAP_HANDLER;
 
 /*
Index: src/modules/rlm_eap/mem.c
===================================================================
RCS file: /source/radiusd/src/modules/rlm_eap/mem.c,v
retrieving revision 1.14.4.1
diff -u -r1.14.4.1 mem.c
--- src/modules/rlm_eap/mem.c	6 Feb 2006 16:23:51 -0000	1.14.4.1
+++ src/modules/rlm_eap/mem.c	8 May 2006 21:53:08 -0000
@@ -25,6 +25,10 @@
 
 static const char rcsid[] = "$Id: mem.c,v 1.14.4.1 2006/02/06 16:23:51 nbk Exp $";
 
+static void eaplist_delete_locked(rlm_eap_t *inst, EAP_HANDLER *handler);
+static void eaplist_delete(rlm_eap_t *inst, EAP_HANDLER *handler);
+
+
 /*
  * Allocate a new EAP_PACKET
  */
@@ -111,8 +115,8 @@
 {
 	EAP_HANDLER	*handler;
 
-	handler = rad_malloc(sizeof(EAP_HANDLER));
-	memset(handler, 0, sizeof(EAP_HANDLER));
+	handler = rad_malloc(sizeof(*handler));
+	memset(handler, 0, sizeof(*handler));
 	return handler;
 }
 
@@ -121,6 +125,12 @@
 	if (!handler)
 		return;
 
+	/*
+	 *	This is a hack to work around request_data_add not
+	 *	taking an "inst" pointer
+	 */
+	if (handler->inst) return eaplist_delete(handler->inst, handler);
+
 	if (handler->identity) {
 		free(handler->identity);
 		handler->identity = NULL;
@@ -195,6 +205,7 @@
 	 */
 	handler->timestamp = handler->request->timestamp;
 	handler->status = 1;
+	handler->inst = inst;
 
 	memcpy(handler->state, state->strvalue, sizeof(handler->state));
 	handler->src_ipaddr = handler->request->packet->src_ipaddr;
@@ -216,6 +227,8 @@
 	 */
 	status = rbtree_insert(inst->session_tree, handler);
 
+	handler->in_list = 1;
+
 	if (status) {
 		EAP_HANDLER *prev;
 
@@ -244,6 +257,56 @@
 }
 
 /*
+ *	Delete, assuming that the mutex is locked.
+ */
+static void eaplist_delete_locked(rlm_eap_t *inst, EAP_HANDLER *handler)
+{
+	rbtree_deletebydata(inst->session_tree, handler);
+
+	/*
+	 *	And unsplice it from the linked list.
+	 */
+	if (handler->prev) {
+		handler->prev->next = handler->next;
+	} else {
+		inst->session_head = NULL;
+	}
+	if (handler->next) {
+		handler->next->prev = handler->prev;
+	} else {
+		inst->session_tail = NULL;
+	}
+	handler->prev = handler->next = NULL;
+
+	handler->inst = NULL;
+	handler->in_list = 0;
+	eap_handler_free(handler);
+}
+
+
+/*
+ *	Delete a handler from the list.
+ */
+static void eaplist_delete(rlm_eap_t *inst, EAP_HANDLER *handler)
+{
+	if (!handler->in_list || !handler->inst) {
+		handler->inst = NULL;
+		handler->in_list = 0;
+		eap_handler_free(handler);
+		return;
+	}
+
+	/*
+	 *	Playing with a data structure shared among threads
+	 *	means that we need a lock, to avoid conflict.
+	 */
+	pthread_mutex_lock(&(inst->session_mutex));
+	eaplist_delete_locked(inst, handler);
+	pthread_mutex_unlock(&(inst->session_mutex));
+}
+
+
+/*
  *	Find a a previous EAP-Request sent by us, which matches
  *	the current EAP-Response.
  *
@@ -292,13 +355,7 @@
 		handler = inst->session_head;
 		if (handler &&
 		    ((request->timestamp - handler->timestamp) > inst->timer_limit)) {
-			node = rbtree_find(inst->session_tree, handler);
-			rad_assert(node != NULL);
-			rbtree_delete(inst->session_tree, node);
-			
-			inst->session_head = handler->next;
-			if (handler->next) handler->next->prev = NULL;
-			eap_handler_free(handler);
+			eaplist_delete_locked(inst, handler);
 		}
 	}
 
@@ -320,25 +377,7 @@
 		if (verify_state(state, handler->timestamp) != 0) {
 			handler = NULL;
 		} else {
-			/*
-			 *	It's OK, delete it from the tree.
-			 */
-			rbtree_delete(inst->session_tree, node);
-
-			/*
-			 *	And unsplice it from the linked list.
-			 */
-			if (handler->prev) {
-				handler->prev->next = handler->next;
-			} else {
-				inst->session_head = NULL;
-			}
-			if (handler->next) {
-				handler->next->prev = handler->prev;
-			} else {
-				inst->session_tail = NULL;
-			}
-			handler->prev = handler->next = NULL;
+			eaplist_delete_locked(inst, handler);
 		}
 	}