From 21dcf57b4e8c8c308be4acb992c803ec8509fd25 Mon Sep 17 00:00:00 2001
From: rswindell <>
Date: Wed, 22 Aug 2012 08:05:22 +0000
Subject: [PATCH] Fix race-condition during shutdown or recycle that could
cause crash: active_clients (protected integer) could be 0 at the time of
recycle because there can be delay at the beginning of the FTP ctrl thread
(e.g. looking up filtered IPs/hostnames) before the active_clients is
incremented which could be during/after it was destroyed by the cleanup()
function. Now tracking number of threads using protected integer which
increments immediately upon child thread creation and is not destroyed until
the main thread terminates. There is currently *not* timeout while waiting
for child threads to terminate upon shutdown/recycle, so this could result in
a hung server thread if a child thread is indefinitely hung - consider using
a timeout.
---
src/sbbs3/ftpsrvr.c | 43 +++++++++++++++++++++++++++++--------------
1 file changed, 29 insertions(+), 14 deletions(-)
diff --git a/src/sbbs3/ftpsrvr.c b/src/sbbs3/ftpsrvr.c
index 9476df4979..7562982b24 100644
--- a/src/sbbs3/ftpsrvr.c
+++ b/src/sbbs3/ftpsrvr.c
@@ -8,7 +8,7 @@
* @format.tab-size 4 (Plain Text/Source Code File Header) *
* @format.use-tabs true (see http://www.synchro.net/ptsc_hdr.html) *
* *
- * Copyright 2011 Rob Swindell - http://www.synchro.net/copyright.html *
+ * Copyright 2012 Rob Swindell - http://www.synchro.net/copyright.html *
* *
* This program is free software; you can redistribute it and/or *
* modify it under the terms of the GNU General Public License *
@@ -80,6 +80,7 @@ static ftp_startup_t* startup=NULL;
static scfg_t scfg;
static SOCKET server_socket=INVALID_SOCKET;
static protected_int32_t active_clients;
+static protected_int32_t thread_count;
static volatile time_t uptime=0;
static volatile ulong served=0;
static volatile BOOL terminate_server=FALSE;
@@ -205,16 +206,20 @@ static void client_off(SOCKET sock)
startup->client_on(startup->cbdata,FALSE,sock,NULL,FALSE);
}
-static void thread_up(BOOL setuid)
+static int32_t thread_up(BOOL setuid)
{
+ int32_t count = protected_int32_adjust(&thread_count,1);
if(startup!=NULL && startup->thread_up!=NULL)
startup->thread_up(startup->cbdata,TRUE, setuid);
+ return count;
}
-static void thread_down(void)
+static int32_t thread_down(void)
{
+ int32_t count = protected_int32_adjust(&thread_count,-1);
if(startup!=NULL && startup->thread_up!=NULL)
startup->thread_up(startup->cbdata,FALSE, FALSE);
+ return count;
}
static SOCKET ftp_open_socket(int type)
@@ -4473,12 +4478,12 @@ static void ctrl_thread(void* arg)
ftp_close_socket(&tmp_sock,__LINE__);
{
- int32_t remain = protected_int32_adjust(&active_clients, -1);
+ int32_t clients = protected_int32_adjust(&active_clients, -1);
+ int32_t threads = thread_down();
update_clients();
- thread_down();
- lprintf(LOG_INFO,"%04d CTRL thread terminated (%ld clients remain, %lu served)"
- ,sock, remain, served);
+ lprintf(LOG_INFO,"%04d CTRL thread terminated (%ld clients and %ld threads remain, %lu served)"
+ ,sock, clients, threads, served);
}
}
@@ -4497,6 +4502,13 @@ static void cleanup(int code, int line)
if(server_socket!=INVALID_SOCKET)
ftp_close_socket(&server_socket,__LINE__);
+ if(thread_count.value > 1) {
+ lprintf(LOG_DEBUG,"#### Waiting for %d child threads to terminate", thread_count.value-1);
+ while(thread_count.value > 1) {
+ mswait(100);
+ }
+ }
+
if(active_clients.value)
lprintf(LOG_WARNING,"#### !FTP Server terminating with %ld active clients", active_clients.value);
else
@@ -4615,6 +4627,7 @@ void DLLCALL ftp_server(void* arg)
startup->recycle_now=FALSE;
startup->shutdown_now=FALSE;
terminate_server=FALSE;
+ protected_int32_init(&thread_count, 0);
do {
@@ -4641,7 +4654,7 @@ void DLLCALL ftp_server(void* arg)
if(!winsock_startup()) {
cleanup(1,__LINE__);
- return;
+ break;
}
t=time(NULL);
@@ -4660,7 +4673,7 @@ void DLLCALL ftp_server(void* arg)
lprintf(LOG_CRIT,"!ERROR %s",error);
lprintf(LOG_CRIT,"!Failed to load configuration files");
cleanup(1,__LINE__);
- return;
+ break;
}
if(startup->host_name[0]==0)
@@ -4683,7 +4696,7 @@ void DLLCALL ftp_server(void* arg)
if(!isdir(scfg.temp_dir)) {
lprintf(LOG_CRIT,"!Invalid temp directory: %s", scfg.temp_dir);
cleanup(1,__LINE__);
- return;
+ break;
}
if(!startup->max_clients) {
@@ -4724,7 +4737,7 @@ void DLLCALL ftp_server(void* arg)
if((server_socket=ftp_open_socket(SOCK_STREAM))==INVALID_SOCKET) {
lprintf(LOG_CRIT,"!ERROR %d opening socket", ERROR_VALUE);
cleanup(1,__LINE__);
- return;
+ break;
}
lprintf(LOG_DEBUG,"%04d FTP Server socket opened",server_socket);
@@ -4751,14 +4764,14 @@ void DLLCALL ftp_server(void* arg)
if(result!=0) {
lprintf(LOG_CRIT,"%04d %s", server_socket, BIND_FAILURE_HELP);
cleanup(1,__LINE__);
- return;
+ break;
}
if((result=listen(server_socket, 1))!= 0) {
lprintf(LOG_CRIT,"%04d !ERROR %d (%d) listening on socket"
,server_socket, result, ERROR_VALUE);
cleanup(1,__LINE__);
- return;
+ break;
}
lprintf(LOG_INFO,"%04d FTP Server listening on port %u",server_socket,startup->port);
@@ -4782,7 +4795,7 @@ void DLLCALL ftp_server(void* arg)
while(server_socket!=INVALID_SOCKET && !terminate_server) {
- if(active_clients.value==0) {
+ if(thread_count.value <= 1) {
if(!(startup->options&FTP_OPT_NO_RECYCLE)) {
if((p=semfile_list_check(&initialized,recycle_semfiles))!=NULL) {
lprintf(LOG_INFO,"0000 Recycle semaphore file (%s) detected",p);
@@ -4907,4 +4920,6 @@ void DLLCALL ftp_server(void* arg)
}
} while(!terminate_server);
+
+ protected_int32_destroy(thread_count);
}
--
GitLab