[Spread-users] Sess_recv_client_auth: BUG! Session is alreadyauthorized (status 0x1)
Jonathan Stanton
jonathan at cnds.jhu.edu
Tue Oct 23 17:32:11 EDT 2001
I believe this problem is fixed now and I have committed a fix to cvs. The
patch that is attached to this email is essentially the same as the one
that was reported by Dave Parker to fix the problem earlier today by
email.
The problem this fixes is a longstanding race when connections are closed
and opened. I do not think it was introduced in any recent version, but
rather has been around a long time but might have showed up in different
ways depending on the version. This might explain some problems people
had with clients who would create and destroy connections very quickly
(mod_log_spread in some configurations)
Basically, when a client closes a connection to spread, the daemon has to
send a message to all of the other daemons informing them that the client
has quit and they should remove any state that refers to that client. This
includes which groups that the client belonged to for example. When the
daemon is able to order this message (possibly a short while later) it
completes the removal of the client from its list of local sessions.
During the period between when the client disconnects (and the daemon
calls 'close' on the socket) and when the message about the client
disconnection is ordered and the session state removed, the client
exists as a 'non-operational' session. This should be fine, as the code
makes sure a non-operational client is not able to do anything. However,
the case of a new 'accept' call returning the same file descriptor as a
non-operational session causes one datastructure to confuse the new and
old sessions (it uses the fd as a key) and caused the problem Dave
expierenced. It should have caused similar problems (with possibly
different symptoms) in earlier versions of the code.
If anyone has any comments on this, or is able to verify that it fixes any
connection problems they have I would appreciate hearing about it.
Thanks,
Jonathan
--
-------------------------------------------------------
Jonathan R. Stanton jonathan at cs.jhu.edu
Dept. of Computer Science
Johns Hopkins University
-------------------------------------------------------
-------------- next part --------------
Index: session.c
===================================================================
RCS file: /storage/cvsroot/spread/daemon/session.c,v
retrieving revision 1.2
diff -u -r1.2 session.c
--- session.c 22 Aug 2001 17:41:13 -0000 1.2
+++ session.c 23 Oct 2001 21:12:50 -0000
@@ -105,6 +105,7 @@
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 );
@@ -370,6 +371,13 @@
return;
}
+ if ( ( (i = Session_index[Sessions[MAX_SESSIONS].mbox]) != -1)
+ && (Sessions[i].mbox == Sessions[MAX_SESSIONS].mbox)
+ && (Is_op_session( Sessions[i].status )) )
+ {
+ /* This is impossible as the mbox must have been closed to be returned by accept */
+ Alarm(EXIT, "Sess_accept: BUG! Accepted new FD %d that is currently in use(ses %d).\n", Sessions[i].mbox, i);
+ }
for( i=10; i <= 200; i+=5 )
{
#ifdef ARCH_PC_LINUX
@@ -616,13 +624,10 @@
Num_sessions++;
GlobalStatus.num_sessions = Num_sessions;
- /* updating Session_index structure */
- for( i=0; i < FD_SETSIZE; i++ ) Session_index[i] = -1;
- for( i=0; i < Num_sessions; i++ )
- Session_index[ Sessions[i].mbox ] = i;
-
Sessions[sess_location].status = Set_preauth_session( Sessions[sess_location].status );
+ Sess_update_session_index();
+
/* OLD client library without authentication/authorization code */
if ( version[0]*10000 + version[1]*100 + version[2] < 31600 )
{
@@ -677,6 +682,10 @@
struct session_auth_info *sess_auth_p;
ses = Session_index[mbox];
+ if( ses < 0 || ses >= Num_sessions ) {
+ Alarm( PRINT, "Sess_recv_client_auth: Illegal mbox %d for receiving client auth. Cannot deny or allow\n", mbox);
+ return;
+ }
if (!Is_preauth_session(Sessions[ses].status) )
{
Alarm( EXIT, "Sess_recv_client_auth: BUG! Session is already authorized (status 0x%x)\n", Sessions[ses].status);
@@ -748,6 +757,11 @@
void (*auth_open)(struct session_auth_info *);
ses = Session_index[sess_auth_h->mbox];
+ if( ses < 0 || ses >= Num_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;
+ }
num_auths = sess_auth_h->num_required_auths;
/* finished another method. See if entire set of checks is complete */
sess_auth_h->required_auth_results[sess_auth_h->completed_required_auths] = authenticated_p;
@@ -811,11 +825,7 @@
Num_sessions--;
GlobalStatus.num_sessions = Num_sessions;
- /* updating Session_index structure */
- for( i=0; i < FD_SETSIZE; i++ ) Session_index[i] = -1;
- for( i=0; i < Num_sessions; i++ )
- Session_index[ Sessions[i].mbox ] = i;
-
+ Sess_update_session_index();
return;
}
@@ -987,6 +997,10 @@
#endif /* 0 */
ses = Session_index[mbox];
+ if( ses < 0 || ses >= Num_sessions ) {
+ Alarm( PRINT, "Sess_read: Illegal mbox %d for read\n", mbox);
+ return;
+ }
if (Sessions[ses].read_mess == NULL)
Sessions[ses].read_mess = Message_new_message();
@@ -1529,6 +1543,18 @@
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;
@@ -1541,6 +1567,10 @@
head_size = Message_get_header_size();
ses = Session_index[mbox];
+ if( ses < 0 || ses >= Num_sessions ) {
+ Alarm( PRINT, "Sess_kill: Illegal mbox %d for killing. Cannot cleanup\n", mbox);
+ return;
+ }
if( !Is_op_session( Sessions[ses].status ) )
{
if ( !Is_preauth_session( Sessions[ses].status ) )
@@ -1585,7 +1615,7 @@
E_dequeue( Sess_badger, mbox, NULL );
E_detach_fd( mbox, READ_FD );
E_detach_fd( mbox, EXCEPT_FD );
- close( mbox );
+ close(mbox);
/* the mailbox is closed but the entry still points to it */
Sessions[ses].status = Clear_op_session( Sessions[ses].status );
Alarm( SESSION, "Sess_kill: killing session %s ( mailbox %d )\n",Sessions[ses].name, mbox );
@@ -1746,10 +1776,7 @@
Num_sessions--;
GlobalStatus.num_sessions = Num_sessions;
- /* updating Session_index structure */
- for( i=0; i < FD_SETSIZE; i++ ) Session_index[i] = -1;
- for( i=0; i < Num_sessions; i++ )
- Session_index[ Sessions[i].mbox ] = i;
+ Sess_update_session_index();
}
}
Prot_kill_session(mess_link->mess);
More information about the Spread-users
mailing list