From dd2afc9286cf31ee3a9831309368ea39f7ea638d Mon Sep 17 00:00:00 2001
From: "Rob Swindell (on Windows 11)" <rob@synchro.net>
Date: Fri, 3 May 2024 17:09:48 -0700
Subject: [PATCH] Change client_t.protocol from pointer to buffer
Fix observed crash when shutting down services server where the client_t
protocol was pointing to a freed service's protocol description string.
This was the last pointer in client_t and should resolve the last race
conditions (memory ownership issues) with its data members.
This also resolves a small memory leak in getnodeclient() where the last
client "gotten" would have its heap-duplicated protocol string leaked.
---
src/sbbs3/answer.cpp | 4 ++--
src/sbbs3/client.h | 2 +-
src/sbbs3/ftpsrvr.c | 4 ++--
src/sbbs3/mailsrvr.c | 8 ++++----
src/sbbs3/main.cpp | 4 ++--
src/sbbs3/services.c | 8 ++++----
src/sbbs3/userdat.c | 7 +------
src/sbbs3/websrvr.c | 2 +-
8 files changed, 17 insertions(+), 22 deletions(-)
diff --git a/src/sbbs3/answer.cpp b/src/sbbs3/answer.cpp
index 04bd5605ab..f8f131cabe 100644
--- a/src/sbbs3/answer.cpp
+++ b/src/sbbs3/answer.cpp
@@ -435,7 +435,7 @@ bool sbbs_t::answer()
mouse_mode = MOUSE_MODE_OFF;
autoterm = 0;
sys_status |= SS_USERON;
- client.protocol = "SFTP";
+ SAFECOPY(client.protocol, "SFTP");
SAFECOPY(client.user, useron.alias);
client.usernum = useron.number;
client_on(client_socket, &client,/* update: */TRUE);
@@ -735,7 +735,7 @@ bool sbbs_t::answer()
} else {
lprintf(LOG_NOTICE, "no Telnet commands received, reverting to Raw TCP mode");
telnet_mode |= TELNET_MODE_OFF;
- client.protocol = "Raw";
+ SAFECOPY(client.protocol, "Raw");
client_on(client_socket, &client,/* update: */TRUE);
SAFECOPY(connection, client.protocol);
node_connection = NODE_CONNECTION_RAW;
diff --git a/src/sbbs3/client.h b/src/sbbs3/client.h
index d25088a859..a5b9012535 100644
--- a/src/sbbs3/client.h
+++ b/src/sbbs3/client.h
@@ -33,7 +33,7 @@ typedef struct {
char host[256]; /* host name */
uint16_t port; /* TCP port number */
time32_t time; /* connect time */
- const char* protocol; /* protocol description */
+ char protocol[32]; /* protocol description */
char user[32]; /* user name */
uint32_t usernum; /* user number (authenticated when non-zero) */
} client_t;
diff --git a/src/sbbs3/ftpsrvr.c b/src/sbbs3/ftpsrvr.c
index dc773adf9a..a952a80833 100644
--- a/src/sbbs3/ftpsrvr.c
+++ b/src/sbbs3/ftpsrvr.c
@@ -2320,7 +2320,7 @@ static void ctrl_thread(void* arg)
SAFECOPY(client.addr,host_ip);
SAFECOPY(client.host,host_name);
client.port=inet_addrport(&ftp.client_addr);
- client.protocol="FTP";
+ SAFECOPY(client.protocol, "FTP");
SAFECOPY(client.user, STR_UNKNOWN_USER);
client.usernum = 0;
client_on(sock,&client,FALSE /* update */);
@@ -2646,7 +2646,7 @@ static void ctrl_thread(void* arg)
got_pbsz = FALSE;
protection = FALSE;
lprintf(LOG_INFO, "%04d <%s> initialized TLS successfully", sock, host_ip);
- client.protocol = "FTPS";
+ SAFECOPY(client.protocol, "FTPS");
client_on(sock, &client, /* update: */TRUE);
continue;
}
diff --git a/src/sbbs3/mailsrvr.c b/src/sbbs3/mailsrvr.c
index 11abdcf8b3..7d30451b78 100644
--- a/src/sbbs3/mailsrvr.c
+++ b/src/sbbs3/mailsrvr.c
@@ -1049,7 +1049,7 @@ static bool pop3_client_thread(pop3_t* pop3)
socket = pop3->socket;
- client.protocol = pop3->tls_port ? "POP3S" : "POP3";
+ SAFECOPY(client.protocol, pop3->tls_port ? "POP3S" : "POP3");
if(startup->options&MAIL_OPT_DEBUG_POP3)
lprintf(LOG_DEBUG,"%04d %s session thread started", socket, client.protocol);
@@ -1221,7 +1221,7 @@ static bool pop3_client_thread(pop3_t* pop3)
}
}
i++;
- client.protocol = "POP3S";
+ SAFECOPY(client.protocol, "POP3S");
}
else
sockprintf(socket,client.protocol,session,"-ERR USER, APOP, CAPA, or STLS command expected");
@@ -2924,7 +2924,7 @@ static bool smtp_client_thread(smtp_t* smtp)
socket = smtp->socket;
- client.protocol = smtp->tls_port ? "SMTPS" : "SMTP";
+ SAFECOPY(client.protocol, smtp->tls_port ? "SMTPS" : "SMTP");
lprintf(LOG_DEBUG,"%04d %s Session thread started", socket, client.protocol);
@@ -4969,7 +4969,7 @@ static bool smtp_client_thread(smtp_t* smtp)
break;
}
}
- client.protocol = "SMTPS";
+ SAFECOPY(client.protocol, "SMTPS");
continue;
}
sockprintf(socket,client.protocol,session,"500 Syntax error");
diff --git a/src/sbbs3/main.cpp b/src/sbbs3/main.cpp
index 446efd8618..f3784716a1 100644
--- a/src/sbbs3/main.cpp
+++ b/src/sbbs3/main.cpp
@@ -5495,9 +5495,9 @@ NO_SSH:
#ifdef USE_CRYPTLIB
- client.protocol=rlogin ? "RLogin":(ssh ? "SSH" : "Telnet");
+ SAFECOPY(client.protocol, rlogin ? "RLogin":(ssh ? "SSH" : "Telnet"));
#else
- client.protocol=rlogin ? "RLogin":"Telnet";
+ SAFECOPY(client.protocol, rlogin ? "RLogin":"Telnet");
#endif
union xp_sockaddr local_addr;
memset(&local_addr, 0, sizeof(local_addr));
diff --git a/src/sbbs3/services.c b/src/sbbs3/services.c
index 80671ca3bc..ac6a27edea 100644
--- a/src/sbbs3/services.c
+++ b/src/sbbs3/services.c
@@ -668,7 +668,7 @@ js_client_add(JSContext *cx, uintN argc, jsval *arglist)
served++;
memset(&client,0,sizeof(client));
client.size=sizeof(client);
- client.protocol=service_client->service->protocol;
+ SAFECOPY(client.protocol, service_client->service->protocol);
client.time=time32(NULL);
SAFECOPY(client.user, STR_UNKNOWN_USER);
client.usernum = 0;
@@ -725,7 +725,7 @@ js_client_update(JSContext *cx, uintN argc, jsval *arglist)
memset(&client,0,sizeof(client));
client.size=sizeof(client);
- client.protocol=service_client->service->protocol;
+ SAFECOPY(client.protocol, service_client->service->protocol);
SAFECOPY(client.user, STR_UNKNOWN_USER);
SAFECOPY(client.host,client.user);
@@ -1150,7 +1150,7 @@ static void js_service_thread(void* arg)
client.time=time32(NULL);
SAFECOPY(client.host,host_name);
client.port=inet_addrport(&service_client.addr);
- client.protocol=service->protocol;
+ SAFECOPY(client.protocol, service->protocol);
SAFECOPY(client.user, STR_UNKNOWN_USER);
client.usernum = 0;
service_client.client=&client;
@@ -1525,7 +1525,7 @@ static void native_service_thread(void* arg)
client.time=time32(NULL);
SAFECOPY(client.host,host_name);
client.port=inet_addrport(&service_client.addr);
- client.protocol=service->protocol;
+ SAFECOPY(client.protocol, service->protocol);
SAFECOPY(client.user, STR_UNKNOWN_USER);
client.usernum = 0;
diff --git a/src/sbbs3/userdat.c b/src/sbbs3/userdat.c
index 7b0b385d4e..4bf292c571 100644
--- a/src/sbbs3/userdat.c
+++ b/src/sbbs3/userdat.c
@@ -1879,16 +1879,12 @@ int getnodeclient(scfg_t* cfg, uint number, client_t* client, time_t* done)
SOCKET sock = INVALID_SOCKET;
char path[MAX_PATH + 1];
char value[INI_MAX_VALUE_LEN];
- char* p;
FILE* fp;
if(!VALID_CFG(cfg)
|| client == NULL || number < 1 || number > cfg->sys_nodes)
return -1;
- if(client->size == sizeof(client)) {
- free((char*)client->protocol);
- }
memset(client, 0, sizeof(*client));
client->size = sizeof(client);
SAFEPRINTF(path, "%sclient.ini", cfg->node_path[number - 1]);
@@ -1901,8 +1897,7 @@ int getnodeclient(scfg_t* cfg, uint number, client_t* client, time_t* done)
client->usernum = iniReadInteger(fp, ROOT_SECTION, "user", 0);
SAFECOPY(client->addr, iniReadString(fp, ROOT_SECTION, "addr", "<none>", value));
SAFECOPY(client->host, iniReadString(fp, ROOT_SECTION, "host", "<none>", value));
- if((p = iniReadString(fp, ROOT_SECTION, "prot", NULL, value)) != NULL)
- client->protocol = strdup(p);
+ SAFECOPY(client->protocol, iniReadString(fp, ROOT_SECTION, "prot", "<unknown>", value));
SAFECOPY(client->user, iniReadString(fp, ROOT_SECTION, "name", "<unknown>", value));
*done = iniReadInteger(fp, ROOT_SECTION, "done", client->time);
fclose(fp);
diff --git a/src/sbbs3/websrvr.c b/src/sbbs3/websrvr.c
index a3d99bf582..1dd0aba536 100644
--- a/src/sbbs3/websrvr.c
+++ b/src/sbbs3/websrvr.c
@@ -6622,7 +6622,7 @@ void http_session_thread(void* arg)
thread_down();
return;
}
- session.client.protocol=session.is_tls ? "HTTPS":"HTTP";
+ SAFECOPY(session.client.protocol, session.is_tls ? "HTTPS":"HTTP");
lprintf(LOG_DEBUG,"%04d %s [%s] Session thread started", session.socket, session.client.protocol, session.host_ip);
if(startup->index_file_name==NULL || startup->cgi_ext==NULL)
--
GitLab