[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