From e7b63f3c5bf43e7907b3313a6e8d06421f262e11 Mon Sep 17 00:00:00 2001
From: InterLinked1 <24227567+InterLinked1@users.noreply.github.com>
Date: Wed, 25 Dec 2024 15:45:33 -0500
Subject: [PATCH] net_ws: Fix possible reference count leak.

It was possible for module references to be leaked by net_ws,
since find_route increases the module refcount (e.g. of mod_webmail),
but on certain off-nominal failure paths, the module refcount was
never decremented since we would jump to an exit path further in
the cleanup process. These have been adjusted so that all jumps
after find_route will clean up the previously leaked refcount.

This is likely the underlying issue that prompted LBBS-3.
---
 nets/net_imap/imap_client_status.c |  5 +++++
 nets/net_smtp.c                    |  2 +-
 nets/net_ws.c                      | 18 +++++++++++-------
 3 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/nets/net_imap/imap_client_status.c b/nets/net_imap/imap_client_status.c
index befc32d..068b872 100644
--- a/nets/net_imap/imap_client_status.c
+++ b/nets/net_imap/imap_client_status.c
@@ -606,6 +606,11 @@ ssize_t remote_status(struct imap_client *client, const char *remotename, const
 				return -1;
 			}
 			if (STARTS_WITH(buf, "* ")) { /* Tolerate unrelated untagged responses interleaved */
+				/* While it's technically legal to receive untagged responses here,
+				 * this really shouldn't happen if we are doing everything right.
+				 * This is because we should be idling on all remote server connections
+				 * when they're not in use, and if we're idling, we should be processing
+				 * these untagged responses as they come in. */
 				bbs_warning("Unexpected response: %s\n", buf);
 				continue;
 			}
diff --git a/nets/net_smtp.c b/nets/net_smtp.c
index 249d7a0..61ed80d 100644
--- a/nets/net_smtp.c
+++ b/nets/net_smtp.c
@@ -496,7 +496,7 @@ static int handle_connect(struct smtp_session *smtp)
 				bbs_debug(3, "Client appears to have disconnected\n");
 				return -1;
 			}
-			bbs_warning("Pregreet: %lu bytes received before banner finished\n", bytes);
+			bbs_warning("Pregreet: %lu byte%s received before banner finished\n", bytes, ESS(bytes));
 			smtp->failures += 3;
 			if (smtp_tarpit(smtp, 220, "Waiting for service to initialize...")) {
 				return -1;
diff --git a/nets/net_ws.c b/nets/net_ws.c
index 7a8a3e8..cab9af2 100644
--- a/nets/net_ws.c
+++ b/nets/net_ws.c
@@ -840,7 +840,7 @@ static unsigned int max_websocket_timeout_ms = MAX_WEBSOCKET_PING_MS;
 static void ws_handler(struct bbs_node *node, struct http_session *http, int proxied)
 {
 	struct ws_session ws;
-	struct ws_route *route;
+	struct ws_route *route = NULL;
 	struct wss_client *client;
 	int res;
 	int want_ping = 0;
@@ -873,21 +873,24 @@ static void ws_handler(struct bbs_node *node, struct http_session *http, int pro
 		goto exit; /* Get lost, dude */
 	}
 
+	/* Past this point, we must not goto exit,
+	 * since find_route called bbs_module_ref, we need to unref to clean up properly. */
+
 	if (allowed_origins) {
 		/* Check that the client's origin is allowed. */
 		char match_str[256];
 		const char *origin = http_request_header(http, "Origin");
 		if (strlen_zero(origin)) {
 			bbs_warning("No Origin header supplied\n");
-			goto exit; /* Goodbye */
+			goto done2; /* Goodbye */
 		} else if (strchr(origin, ',')) {
 			bbs_warning("Origin header seems invalid: %s\n", origin);
-			goto exit;
+			goto done2;
 		}
 		snprintf(match_str, sizeof(match_str), ",%s,", origin);
 		if (!strstr(allowed_origins, match_str)) {
 			bbs_warning("Client origin '%s' is not explicitly allowed, rejecting\n", origin);
-			goto exit;
+			goto done2;
 		}
 		bbs_debug(4, "Origin '%s' is explicitly allowed\n", origin);
 	}
@@ -895,14 +898,14 @@ static void ws_handler(struct bbs_node *node, struct http_session *http, int pro
 	client = wss_client_new(&ws, node->rfd, node->wfd);
 	if (!client) {
 		bbs_error("Failed to create WebSocket client\n");
-		goto exit;
+		goto done2;
 	}
 	ws.client = client; /* Needed as part of structure so it can be accessed in websocket_sendtext */
 
 	bbs_mutex_init(&ws.lock, NULL);
 
 	if (route->callbacks->on_open && route->callbacks->on_open(&ws)) {
-		goto exit;
+		goto done2;
 	}
 
 	memset(&pfds, 0, sizeof(pfds));
@@ -1110,9 +1113,10 @@ static void ws_handler(struct bbs_node *node, struct http_session *http, int pro
 		wss_client_destroy(client);
 		bbs_mutex_destroy(&ws.lock);
 	}
-	bbs_module_unref(route->mod, 1);
 	php_vars_destroy(&ws.varlist);
 	php_vars_destroy(&ws.cookievals);
+done2:
+	bbs_module_unref(route->mod, 1);
 exit:
 	RWLIST_HEAD_DESTROY(&ws.varlist);
 	RWLIST_HEAD_DESTROY(&ws.cookievals);