[Spread-users] [BUG][PATCH] fd to session troubles

Marc Zyngier Marc.Zyngier at evidian.com
Fri Jan 25 07:19:18 EST 2002


Hi all,

I'd like to report a problem we've been facing on a multiprocessor NT
system, as well as proposing a possible fix.

The problems is the way the spread daemon finds a session given a
mailbox.

>From sess_body.h :

ext    int             Session_index[FD_SETSIZE]; /* converting from fd to inde
x in Sessions */

The Session_index array is a fd indexed lookup table for sessions. As
noted in events.c, fd can be larger than FD_SETSIZE :

/* Windows bug: Reports FD_SETSIZE of 64 but select works on all
 * fd's even ones with numbers greater then 64.
 */

For example, on our test box, the first session gets fd number
88. Given that FD_SETSIZE is still 64, we are corrupting spread's
internal structures, and spread eventually crashes at some latter
time, thanks to Sess_update_session_index :

    for( i=0; i < Num_sessions; i++ )
    {
        if ( Is_op_session(Sessions[i].status) || Is_preauth_session(Sessions[i]
.status) )
            Session_index[ Sessions[i].mbox ] = i;
    }       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ <- Corruption here...

Of course, one solution is simply to increase the Session_index size,
but this looks to me like sweeping dust under the carpet...

The solution we are proposing in the enclosed patch is to use a simple
hash table instead of the Session_index array, with corresponding
methods to hash/unhash a session, as well as getting the session
index.

For this approach to be working, we had to rework the way sessions are
allocated/destroyed in the Sessions array (we are now building two
lists in the Sessions array, instead of the memmove-based
insertion/deletion).

The only semantic change is that Num_sessions now only represents the
number of active sessions, not the lowest free slot in the Sessions
array.

This patch (against 3.16.1) has been tested with success on our
Linux/Solaris/AIX/HP-UX/NT/W2K platforms. As usual, I'd be happy to
get any comment regarding this patch.

Regards,

	M.

diff -u vanilla/spread-src-3.16.1/auth-ip.c spread-src-3.16.1/auth-ip.c
--- vanilla/spread-src-3.16.1/auth-ip.c	Wed Aug 22 19:41:13 2001
+++ spread-src-3.16.1/auth-ip.c	Thu Jan 24 14:43:28 2002
@@ -187,7 +187,7 @@
         Sess_session_report_auth_result( sess_auth_p, FALSE );
         return;
     }
-    ses = Session_index[sess_auth_p->mbox];
+    ses = Sess_get_session_index (sess_auth_p->mbox);
     client_ip = Sessions[ses].address;
     rule_p = Allow_Rules;
     allowed = FALSE;
Common subdirectories: vanilla/spread-src-3.16.1/docs and spread-src-3.16.1/docs
diff -u vanilla/spread-src-3.16.1/groups.c spread-src-3.16.1/groups.c
--- vanilla/spread-src-3.16.1/groups.c	Tue Oct 16 17:53:44 2001
+++ spread-src-3.16.1/groups.c	Thu Jan 24 14:43:28 2002
@@ -342,7 +342,7 @@
                                         needed = 0;
                                         for( i=0; i < grp->num_local; i++ )
                                         {
-                                                ses = Session_index[ grp->mbox[i] ];
+                                                ses = Sess_get_session_index ( grp->mbox[i] );
                                                 if( Is_memb_session( Sessions[ ses ].status ) )
                                                         Sess_write( ses, mess_link, &needed );
                                         }
@@ -531,7 +531,7 @@
 					needed = 0;
 					for( i=0; i < grp->num_local; i++ )
 					{
-						ses = Session_index[ grp->mbox[i] ];
+						ses = Sess_get_session_index ( grp->mbox[i] );
 						if( Is_memb_session( Sessions[ ses ].status ) )
 							Sess_write( ses, mess_link, &needed );
 					}
@@ -768,7 +768,7 @@
 					needed = 0;
 					for( i=0; i < grp->num_local; i++ )
 					{
-						ses = Session_index[ grp->mbox[i] ];
+						ses = Sess_get_session_index ( grp->mbox[i] );
 						if( Is_memb_session( Sessions[ ses ].status ) )
 							Sess_write( ses, mess_link, &needed );
 					}
@@ -801,7 +801,7 @@
 						/* if new member is local we do not notify it here */
 						if( grp->mbox[i] == new_mbox ) continue;
 
-						ses = Session_index[ grp->mbox[i] ];
+						ses = Sess_get_session_index ( grp->mbox[i] );
 						if( Is_memb_session( Sessions[ ses ].status ) )
 							Sess_write( ses, mess_link, &needed );
 					}
@@ -830,7 +830,7 @@
                                                 Obj_Inc_Refcount(mess_link->mess);
 
 						needed = 0;
-						ses = Session_index[ new_mbox ];
+						ses = Sess_get_session_index ( new_mbox );
 						if( Is_memb_session( Sessions[ ses ].status ) )
 							Sess_write( ses, mess_link, &needed );
 						if ( !needed ) Sess_dispose_message( mess_link );
@@ -841,7 +841,7 @@
 					needed = 0;
 					for( i=0; i < grp->num_local; i++ )
 					{
-						ses = Session_index[ grp->mbox[i] ];
+						ses = Sess_get_session_index ( grp->mbox[i] );
 						if( Is_memb_session( Sessions[ ses ].status ) )
 							Sess_write( ses, mess_link, &needed );
 					}
@@ -875,7 +875,7 @@
 				needed = 0;
 				for( i=0; i < grp->num_local; i++ )
 				{
-					ses = Session_index[ grp->mbox[i] ];
+					ses = Sess_get_session_index ( grp->mbox[i] );
 					if( Is_memb_session( Sessions[ ses ].status ) )
 						Sess_write( ses, mess_link, &needed );
 				}
@@ -888,7 +888,7 @@
 				needed = 0;
 				for( i=0; i < grp->num_local; i++ )
 				{
-					ses = Session_index[ grp->mbox[i] ];
+					ses = Sess_get_session_index ( grp->mbox[i] );
 					if( Is_memb_session( Sessions[ ses ].status ) )
 						Sess_write( ses, mess_link, &needed );
 				}
@@ -1089,7 +1089,7 @@
 			needed = 0;
 			for( i=0; i < grp->num_local; i++ )
 			{
-				ses = Session_index[ grp->mbox[i] ];
+				ses = Sess_get_session_index ( grp->mbox[i] );
 				if( Is_memb_session( Sessions[ ses ].status ) )
 					Sess_write( ses, mess_link, &needed );
 			}
@@ -1284,7 +1284,7 @@
 				{
                                         int temp_ses;
 
-			 		temp_ses = Session_index[ grp->mbox[i] ];
+			 		temp_ses = Sess_get_session_index ( grp->mbox[i] );
 					if( Is_memb_session( Sessions[ temp_ses ].status ) )
 						Sess_write( temp_ses, mess_link, &needed );
 				}
@@ -2044,7 +2044,7 @@
 	}
 
 	/* convert mbox to sessions */
-	for( i=0; i < num_mbox; i++ ) target_sessions[i] = Session_index[ mbox[ i ] ];
+	for( i=0; i < num_mbox; i++ ) target_sessions[i] = Sess_get_session_index ( mbox[ i ] );
 	return( num_mbox );
 }
 
Common subdirectories: vanilla/spread-src-3.16.1/java and spread-src-3.16.1/java
Common subdirectories: vanilla/spread-src-3.16.1/perl and spread-src-3.16.1/perl
diff -u vanilla/spread-src-3.16.1/sess_body.h spread-src-3.16.1/sess_body.h
--- vanilla/spread-src-3.16.1/sess_body.h	Tue Aug 21 16:28:21 2001
+++ spread-src-3.16.1/sess_body.h	Thu Jan 24 14:43:28 2002
@@ -97,7 +97,6 @@
 
 ext	int		Num_sessions;
 ext	session		Sessions[MAX_SESSIONS+1]; /* +1 for rejecting the next one */
-ext	int		Session_index[FD_SETSIZE]; /* converting from fd to index in Sessions */
 
 ext	int		Session_threshold;
 
@@ -109,5 +108,6 @@
 void	Sess_write( int ses, message_link *mess_link, int *needed );
 void	Sess_dispose_message( message_link *mess_link );
 int	Sess_get_session( char *name );
+int	Sess_get_session_index (int mbox);
 
 #endif	/* INC_SESS_BODY */
diff -u vanilla/spread-src-3.16.1/session.c spread-src-3.16.1/session.c
--- vanilla/spread-src-3.16.1/session.c	Thu Nov  1 00:06:48 2001
+++ spread-src-3.16.1/session.c	Thu Jan 24 14:43:28 2002
@@ -97,6 +97,11 @@
 
 static	int		Protocol_threshold;
 
+static	session		*Sessions_hash_head[256];
+static	session		*Sessions_head;
+static	session		*Sessions_tail;
+static	session		*Sessions_free;
+
 static	void	Sess_attach_accept(void);
 static	void	Sess_detach_accept(void);
 static  void    Sess_recv_client_auth(mailbox mbox, int dummy, void *dummy_p);
@@ -105,7 +110,6 @@
 static	void    Sess_read( mailbox mbox, int domain, void *dummy );
 static	void	Sess_badger( mailbox mbox, void *dummy );
 static	void	Sess_kill( mailbox mbox );
-static  void    Sess_update_session_index(void);
 static	void	Sess_handle_join( message_link *mess_link );
 static	void	Sess_handle_leave( message_link *mess_link );
 static	void	Sess_handle_kill( message_link *mess_link );
@@ -113,6 +117,170 @@
 static  void    Sess_create_reject_message ( message_obj *msg );
 static  int     Sess_get_p2p_dests( int num_groups, char groups[][MAX_GROUP_NAME], char dests[][MAX_GROUP_NAME] );
 
+int	Sess_get_session_index (int mbox)
+{
+  session *tmp;
+  char *c = (char *) &mbox;
+  int i;
+
+  i = c[0] ^ c[1] ^ c[2] ^ c[3];
+
+  for (tmp = Sessions_hash_head[i]; tmp; tmp = tmp->hash_next)
+    if (tmp->mbox == mbox)
+      return (tmp - Sessions);
+
+  return -1;
+}
+
+static  void    Sess_hash_session (session *ses)
+{
+    int i;
+    char *c;
+    
+    c = (char *) &ses->mbox;
+    i = c[0] ^ c[1] ^ c[2] ^ c[3];
+    ses->hash_next = Sessions_hash_head[i];
+    Sessions_hash_head[i] = ses;
+}
+
+static  void    Sess_unhash_session (session *ses)
+{
+    int i;
+    char *c;
+    session *tmp;
+    
+    c = (char *) &ses->mbox;
+    i = c[0] ^ c[1] ^ c[2] ^ c[3];
+    tmp = Sessions_hash_head[i];
+    if (tmp == ses)
+    {
+      Sessions_hash_head[i] = ses->hash_next;
+      ses->hash_next = NULL;
+      return;
+    }
+
+    for ( ; tmp->hash_next != ses; tmp = tmp->hash_next);
+    tmp->hash_next = ses->hash_next;
+    ses->hash_next = NULL;
+}
+
+static	session	*Sess_get_free_session (void)
+{
+  session *ses;
+
+  if (!(ses = Sessions_free))
+  {
+    Alarm (EXIT, "Sess_get_free_session: BUG ! No free sessions !\n");
+  }
+
+  Sessions_free = Sessions_free->sort_next;
+
+  return ses;
+}
+
+static	void	Sess_free_session (session *ses)
+{
+  ses->sort_next = Sessions_free;
+  Sessions_free = ses;
+}
+
+static	int	Sess_insert_new_session (session *where, session *template)
+{
+  session *new_ses;
+
+  new_ses = Sess_get_free_session ();
+  memmove (new_ses, template, sizeof (*template));
+  
+  if (!where)
+  {
+    /* Ok, we insert a session at the end of the list... */
+    new_ses->sort_next = NULL;
+    
+    if (!Sessions_tail)
+    {
+      /* List is empty */
+      new_ses->sort_prev = NULL;
+      Sessions_head = Sessions_tail = new_ses;
+    }
+    else
+    {
+      new_ses->sort_prev = Sessions_tail;
+      Sessions_tail->sort_next = new_ses;
+      Sessions_tail = new_ses;
+    }
+  }
+  else
+  {
+    /* Ok, we insert a session in the middle of the list, just
+     * before where... */
+    new_ses->sort_next = where;
+    new_ses->sort_prev = where->sort_prev;
+    where->sort_prev = new_ses;
+    
+    if (!new_ses->sort_prev)
+    {
+      /* new_ses is new head */
+      Sessions_head = new_ses;
+    }
+    else
+    {
+      new_ses->sort_prev->sort_next = new_ses;
+    }
+  }
+
+  return (new_ses - Sessions);
+}
+
+static	void	Sess_remove_session (session *ses)
+{
+  if (!ses->sort_prev && !ses->sort_next)
+  {
+    /* Last session */
+    Sessions_head = Sessions_tail = NULL;
+    Sess_free_session (ses);
+    
+    return;
+  }
+
+  if (!ses->sort_prev)
+  {
+    /* Head */
+    Sessions_head = ses->sort_next;
+    ses->sort_next->sort_prev = NULL;
+    Sess_free_session (ses);
+    
+    return;
+  }
+
+  if (!ses->sort_next)
+  {
+    /* Tail */
+    Sessions_tail = ses->sort_prev;
+    ses->sort_prev->sort_next = NULL;
+    Sess_free_session (ses);
+    
+    return;
+  }
+
+  /* All troubled cases are above ;-) */
+  ses->sort_next->sort_prev = ses->sort_prev;
+  ses->sort_prev->sort_next = ses->sort_next;
+  Sess_free_session (ses);
+}
+
+static	void	Sess_init_sessions (void)
+{
+  int i;
+
+  for (i = 0; i < 256; i++)
+    Sessions_hash_head[i] = NULL;
+
+  Sessions_free = Sessions_head = Sessions_tail = NULL;
+
+  for (i = 0; i < MAX_SESSIONS; i++)
+    Sess_free_session (&Sessions[i]);
+}
+
 int     count_bits_set( int32u field, int first_index, int last_index)
 {       
         int i;
@@ -158,7 +326,9 @@
         {
                 Alarm(EXIT, "Sess_Init: Failure to Initialize DOWN_LINK memory objects\n");
         }
-        
+
+	Sess_init_sessions ();
+	
 	Num_sessions = 0;
 	GlobalStatus.num_sessions = Num_sessions;
 	GlobalStatus.message_delivered = 0;
@@ -371,7 +541,7 @@
 		return;
 	}
 
-        if ( ( (i = Session_index[Sessions[MAX_SESSIONS].mbox]) != -1) 
+        if ( ( (i = Sess_get_session_index(Sessions[MAX_SESSIONS].mbox)) != -1)
              && (Sessions[i].mbox == Sessions[MAX_SESSIONS].mbox) 
              && (Is_op_session( Sessions[i].status )) )
         {
@@ -436,6 +606,7 @@
 	int			ret, i, sess_location, rnum;
         char                    *allowed_auth_list;
         unsigned char           list_len;
+	session			*tmp_ses;
  
 	E_dequeue( Sess_accept_continue2, 1, NULL );
 	E_detach_fd( Sessions[MAX_SESSIONS].mbox, READ_FD );
@@ -546,9 +717,9 @@
                         memcpy( Sessions[MAX_SESSIONS].name, newname, MAX_PRIVATE_NAME);
 
                         /* checking if private name is unique */
-                        for( unique_private_name=1, sess_location=0; sess_location < Num_sessions; sess_location++ )
+                        for( unique_private_name=1, tmp_ses = Sessions_head; tmp_ses; tmp_ses = tmp_ses->sort_next )
                         {
-                                ret = strcmp( Sessions[MAX_SESSIONS].name, Sessions[sess_location].name );
+                                ret = strcmp( Sessions[MAX_SESSIONS].name, tmp_ses->name );
                                 if( ret <= 0 )
                                 {
                                         if( ret == 0 ) unique_private_name = 0;
@@ -592,9 +763,9 @@
                 }
 
                 /* checking if private name is unique */
-                for( unique_private_name=1, sess_location=0; sess_location < Num_sessions; sess_location++ )
+		for( unique_private_name=1, tmp_ses = Sessions_head; tmp_ses; tmp_ses = tmp_ses->sort_next )
                 {
-                        ret = strcmp( Sessions[MAX_SESSIONS].name, Sessions[sess_location].name );
+                        ret = strcmp( Sessions[MAX_SESSIONS].name, tmp_ses->name );
                         if( ret <= 0 )
                         {
                                 if( ret == 0 ) unique_private_name = 0;
@@ -617,16 +788,16 @@
 	ioctl_cmd = 0;
 	ret = ioctl( Sessions[MAX_SESSIONS].mbox, FIONBIO, &ioctl_cmd);
 
-	/* sorting the Sessions structure according to name */
-	memmove( &Sessions[sess_location+1], &Sessions[sess_location], (Num_sessions - sess_location)*sizeof(session) );
-	memmove( &Sessions[sess_location], &Sessions[MAX_SESSIONS], sizeof(session) );
-
+	/* Insert the new session just before the point we already
+	 * found while checking unique private name... */
+	sess_location = Sess_insert_new_session (tmp_ses, &Sessions[MAX_SESSIONS]);
+	
 	Num_sessions++;
 	GlobalStatus.num_sessions = Num_sessions;
 
         Sessions[sess_location].status = Set_preauth_session( Sessions[sess_location].status );
 
-        Sess_update_session_index();
+        Sess_hash_session (&Sessions[sess_location]);
 
         /* OLD client library without authentication/authorization code */
         if ( version[0]*10000 + version[1]*100 + version[2] < 31600 )
@@ -681,8 +852,8 @@
         void        (*auth_open)(struct session_auth_info *);
         struct session_auth_info *sess_auth_p;
 
-        ses = Session_index[mbox];
-	if( ses < 0 || ses >= Num_sessions ) {
+        ses = Sess_get_session_index(mbox);
+	if( ses < 0 || ses >= MAX_SESSIONS ) {
             Alarm( PRINT, "Sess_recv_client_auth: Illegal mbox %d for receiving client auth. Cannot deny or allow\n", mbox);
             return;
         }
@@ -756,8 +927,8 @@
         int permit_count, decision, i;
         void        (*auth_open)(struct session_auth_info *);
 
-        ses = Session_index[sess_auth_h->mbox];
-	if( ses < 0 || ses >= Num_sessions ) {
+        ses = Sess_get_session_index(sess_auth_h->mbox);
+	if( ses < 0 || ses >= MAX_SESSIONS ) {
             Alarm( PRINT, "Sess_session_report_auth_result: Illegal mbox %d for authentication. Cannot deny or allow\n", sess_auth_h->mbox);
             dispose( sess_auth_h );
             return;
@@ -806,7 +977,6 @@
 void    Sess_session_denied(int ses)
 {
         char response;
-        int i;
 
         if (!Is_preauth_session(Sessions[ses].status) )
         {
@@ -820,12 +990,11 @@
                Sessions[ses].mbox );
         close( Sessions[ses].mbox );
 
-        for( i=ses+1; i < Num_sessions; i++ )
-                memmove( &Sessions[i-1], &Sessions[i], sizeof(session) );
+        Sess_unhash_session (&Sessions[ses]);
+	Sess_remove_session (&Sessions[ses]);
         Num_sessions--;
         GlobalStatus.num_sessions = Num_sessions;
     
-        Sess_update_session_index();
         return;
 }
 
@@ -996,8 +1165,8 @@
 #endif /* ARCH_SCATTER_ACCRIGHTS */
 #endif /* 0 */
 
-        ses = Session_index[mbox];
-	if( ses < 0 || ses >= Num_sessions ) {
+        ses = Sess_get_session_index(mbox);
+	if( ses < 0 || ses >= MAX_SESSIONS ) {
             Alarm( PRINT, "Sess_read: Illegal mbox %d for read\n", mbox);
             return;
         }
@@ -1469,8 +1638,8 @@
 	int		ret;
 
 	Alarm( SESSION, "Sess_badger: for mbox %d\n", mbox );
-	ses = Session_index[ mbox ];
-	if( ses < 0 || ses >= Num_sessions ) return;
+	ses = Sess_get_session_index( mbox );
+	if( ses < 0 || ses >= MAX_SESSIONS ) return;
 	if( !Is_op_session( Sessions[ses].status ) ) return;
 
 	if( Sessions[ses].num_mess <= 0 ) return;
@@ -1543,18 +1712,6 @@
 	if( Sessions[ses].num_mess > 0 ) E_queue( Sess_badger, mbox, NULL, Badger_timeout );
 }
 
-static  void    Sess_update_session_index(void)
-{
-    int i;
-    /* updating Session_index structure */
-    for( i=0; i < FD_SETSIZE; i++ ) Session_index[i] = -1;
-    for( i=0; i < Num_sessions; i++ )
-    {
-        if ( Is_op_session(Sessions[i].status) || Is_preauth_session(Sessions[i].status) )
-            Session_index[ Sessions[i].mbox ] = i;
-    }
-}
-
 static	void	Sess_kill( mailbox mbox )
 {
 	int		ses;
@@ -1566,8 +1723,8 @@
         char            private_send_group[MAX_GROUP_NAME];
 
         head_size = Message_get_header_size();
-	ses = Session_index[mbox];
-	if( ses < 0 || ses >= Num_sessions ) {
+	ses = Sess_get_session_index(mbox);
+	if( ses < 0 || ses >= MAX_SESSIONS ) {
             Alarm( PRINT, "Sess_kill: Illegal mbox %d for killing. Cannot cleanup\n", mbox);
             return;
         }
@@ -1753,7 +1910,7 @@
 	char		proc_name[MAX_PROC_NAME];
 	message_header	*kill_head;
 	int		ses;
-	int		i, ret;
+	int		ret;
 
 	kill_head = Message_get_message_header(mess_link->mess);
 
@@ -1772,13 +1929,11 @@
 			/* delete session ses */
 			if( Is_op_session( Sessions[ses].status ) )
 				Alarm( EXIT, "Sess_handle_kill: killing unkilled session bug!\n");
+			Sess_unhash_session (&Sessions[ses]);
                         Prot_Destroy_Local_Session(&(Sessions[ses]) );
-			for( i=ses+1; i < Num_sessions; i++ )
-				memmove( &Sessions[i-1], &Sessions[i], sizeof(session) );
+			Sess_remove_session (&Sessions[ses]);
 			Num_sessions--;
 			GlobalStatus.num_sessions = Num_sessions;
-
-                        Sess_update_session_index();
 		}
 	}
         Prot_kill_session(mess_link->mess);
@@ -1794,14 +1949,14 @@
 
 int	Sess_get_session( char *name )
 {
-	int	i;
 	int	ret;
+	session	*ses;
 
-	for( i=0; i < Num_sessions; i++ )
+	for( ses = Sessions_head; ses; ses = ses->sort_next )
 	{
-		ret = strcmp( Sessions[i].name, name );
+		ret = strcmp( ses->name, name );
 		if( ret <  0 ) continue;
-		if( ret == 0 ) return( i );
+		if( ret == 0 ) return( ses - Sessions );
 		if( ret >  0 ) return( -1 );
 	}
 	return( -1 );
@@ -1813,3 +1968,4 @@
 	head_ptr->num_groups	= Flip_int32( head_ptr->num_groups );
 	head_ptr->data_len	= Flip_int32( head_ptr->data_len );
 }
+
Only in spread-src-3.16.1/: session.c.orig
diff -u vanilla/spread-src-3.16.1/session.h spread-src-3.16.1/session.h
--- vanilla/spread-src-3.16.1/session.h	Tue Aug 21 16:28:21 2001
+++ spread-src-3.16.1/session.h	Thu Jan 24 14:43:28 2002
@@ -69,6 +69,9 @@
         struct partial_message_info     write;  /* Write Queue to Client */
 	message_link	*first;                 /* Write Queue to Client */
 	message_link	*last;                  /* Write Queue to Client */
+	struct dummy_session *sort_prev;
+	struct dummy_session *sort_next;
+	struct dummy_session *hash_next;
 } session;
 
 void	Sess_init(void);


-- 
And don't forget you'll never get a dog to walk upright
Just 'cause you've got the power, that don't mean you've got the right.





More information about the Spread-users mailing list