mod_webmail: Fix memory leak when using SORT/SEARCH.

When using our custom libetpan function for SORT/SEARCH,
the clist of results was not being freed afterwards since
neither of the conditions to free was true. These conditions
were correct, since the corresponding free functions cannot
be used, but we still need to manually free the list.

Basic tests have also been added for mod_webmail that captured
this memory leak (and now pass).

Related fixes:
* mod_http: Don't compare Connection header value case-sensitively.
  Per RFC 7230, this value is not case-sensitive. The mod_webmail
  test uses it in a different case intentionally to test this.
* net_ws: Remove dependency on net_http. Although you would probably
  want this module loaded, it is not actually a hard dependency,
  i.e. net_http exports no symbols required by net_ws to load.
  This eliminates some otherwise confusing error messages about
  dependencies failing to load, since it's not actually one.
* tests/Makefile: Use $^ instead of *.o as the inputs for the test
  binary. I'm not sure how it ever worked the old way, since the
  object files for the test modules shouldn't get linked in.

LBBS-99 #close

Author: InterLinked1

bbs/module.c

@@ -310,7 +310,7 @@ struct bbs_module *__bbs_require_module(const char *module, void *refmod)
 		bbs_debug(5, "Module dependency '%s' is satisfied (required by %s)\n", module, reffing_mod->name);
 		__bbs_module_ref(mod, 1, refmod, __FILE__, __LINE__, __func__);
 	} else {
-		bbs_warning("Module %s dependency is not satisfied (required by %s)\n", module, reffing_mod->name);
+		bbs_error("Module %s dependency is not satisfied (required by %s)\n", module, reffing_mod->name);
 	}
 	return mod;
 }
@@ -953,7 +953,7 @@ static void dec_refcounts(struct bbs_module *mod)
 			if (m) {
 				__bbs_unrequire_module(m, mod);
 			} else {
-				bbs_warning("Dependency %s not currently loaded?\n", dependency);
+				bbs_error("Dependency %s not currently loaded?\n", dependency);
 			}
 		}
 	}
@@ -1077,7 +1077,9 @@ static int on_module(const char *dir_name, const char *filename, void *obj)
 	UNUSED(dir_name);
 	UNUSED(obj);
 
+#ifdef EXTRA_DEBUG
 	bbs_debug(7, "Detected dynamic module %s\n", filename);
+#endif
 	a = calloc(1, sizeof(*a) + strlen(filename) + 1);
 	if (ALLOC_FAILURE(a)) {
 		return -1;

modules/mod_http.c

@@ -882,7 +882,7 @@ int http_websocket_upgrade_requested(struct http_session *http)
 {
 	const char *value;
 	value = http_request_header(http, "Connection");
-	if (!strlen_zero(value) && !strcmp(value, "Upgrade")) {
+	if (!strlen_zero(value) && !strcasecmp(value, "Upgrade")) { /* Connection header value is case-insensitive (RFC 7230 6.1) */
 		value = http_request_header(http, "Upgrade");
 		if (!strlen_zero(value) && !strcasecmp(value, "WebSocket")) {
 			return 1;

modules/mod_mail.c

@@ -2014,7 +2014,7 @@ int imap_client_login(struct bbs_tcp_client *client, struct bbs_url *url, struct
 				IMAP_CLIENT_SEND(client, "");
 			}
 			/* Won't get it, but at least see what the server had to say */
-			bbs_tcp_client_expect(client, "\r\n", 1, 2000, "a1 OK"); /* Don't use IMAP_CLIENT_EXPECT, or we'll bypass the warning below when it fails */
+			bbs_tcp_client_expect(client, "\r\n", 1, SEC_MS(7), "a1 OK"); /* Don't use IMAP_CLIENT_EXPECT, or we'll bypass the warning below when it fails */
 			bbs_warning("Login failed, got '%s'\n", client->buf);
 			return -1;
 		}

modules/mod_webmail.c

@@ -1639,7 +1639,7 @@ static int mailimap_search_sort_fuller(mailimap *session, const char *sortkey, c
 	for (cur = clist_begin(session->imap_response_info->rsp_extension_list); cur; cur = clist_next(cur)) {
 		struct mailimap_extension_data *ext_data = clist_content(cur);
 		if (ext_data->ext_extension->ext_id == MAILIMAP_EXTENSION_SORT) {
-			if (sortkey && !sort_result) {
+			if (sortkey && !sort_result) { /* Sort */
 				sort_result = ext_data->ext_data;
 				ext_data->ext_data = NULL;
 				ext_data->ext_type = -1;
@@ -1650,7 +1650,7 @@ static int mailimap_search_sort_fuller(mailimap *session, const char *sortkey, c
 	clist_free(session->imap_response_info->rsp_extension_list);
 	session->imap_response_info->rsp_extension_list = NULL;
 
-	if (!sort_result) {
+	if (!sort_result) { /* Search only */
 		sort_result = session->imap_response_info->rsp_search_result;
 		session->imap_response_info->rsp_search_result = NULL;
 	}
@@ -1672,11 +1672,11 @@ static int mailimap_search_sort_fuller(mailimap *session, const char *sortkey, c
 
 static struct mailimap_search_key *mailimap_search_key_new_type(int sk_type)
 {
-  return mailimap_search_key_new(sk_type, NULL, NULL, NULL, NULL, NULL,
-	NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, 0, NULL, NULL, NULL, NULL, NULL, NULL, 0, NULL, NULL, NULL);
+	return mailimap_search_key_new(sk_type, NULL, NULL, NULL, NULL, NULL,
+		NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, 0, NULL, NULL, NULL, NULL, NULL, NULL, 0, NULL, NULL, NULL);
 }
 
-static clist *sortall(struct imap_client *client, const char *sort, const char *search, int *searched, int *sorted)
+static clist *sortall(struct imap_client *client, const char *sort, const char *search, int *restrict searched, int *restrict sorted)
 {
 	int res = -1;
 	clist *list = NULL;
@@ -1721,7 +1721,7 @@ static clist *sortall(struct imap_client *client, const char *sort, const char *
 				bbs_warning("Unsupported search criteria: '%s'\n", search);
 			}
 		}
-		*sorted = *searched = 0;
+		*sorted = *searched = 0; /* Yes, this is correct, neither should be set when we return as list is not exactly the same */
 		res = mailimap_search_sort_fuller(client->imap, sort, search, &list);
 	} else {
 		struct mailimap_sort_key *sortkey = NULL;
@@ -2030,6 +2030,7 @@ static int fetchlist(struct ws_session *ws, struct imap_client *client, const ch
 	json_t *root = NULL, *arr;
 	int added = 0;
 	int fetch_attempts = 0;
+	clist *sorted = NULL;
 
 	/* start to end is inclusive */
 	expected = end - start + 1;
@@ -2043,22 +2044,39 @@ static int fetchlist(struct ws_session *ws, struct imap_client *client, const ch
 		client_set_status(ws, "Your IMAP server does not support SORT");
 		sort = NULL; /* If server doesn't support sort, we can't sort */
 	}
+
+/* The else case is to handle frees when LIBETPAN_SEARCH_KEYS_BROKEN_ABI is true (which it currently is).
+ * In this case, neither searchlist or sortlist will be set to 1, because mailimap_sort_result_free
+ * and mailimap_search_result_free are not the correct functions to use.
+ * In that case, we can just manually free the list and everything in it. */
+#define FREE_SORT_SEARCH_LISTS() \
+	if (sortlist) { \
+		mailimap_sort_result_free(sorted); \
+		sorted = NULL; \
+	} else if (searchlist) { \
+		mailimap_search_result_free(sorted); \
+		sorted = NULL; \
+	} else { \
+		for (cur = clist_begin(sorted); cur; cur = clist_next(cur)) { \
+			uint32_t *s = clist_content(cur); \
+			free(s); \
+		} \
+		clist_free(sorted); \
+		sorted = NULL; \
+	}
+
 	/* Thankfully, SEARCH is not an extension, it should be supported by all IMAP servers */
 	if (sort || filter) {
 		int index = 1;
 		int searchlist, sortlist;
 		/*! \todo Cache this between FETCHLIST calls */
-		clist *sorted = sortall(client, sort, filter, &searchlist, &sortlist); /* This could be somewhat slow, since we have sort the ENTIRE mailbox every time */
+		sorted = sortall(client, sort, filter, &searchlist, &sortlist); /* This could be somewhat slow, since we have sort the ENTIRE mailbox every time */
 		if (!sorted) {
 			return -1;
 		}
 		set = mailimap_set_new_empty();
 		if (!set) {
-			if (sortlist) {
-				mailimap_sort_result_free(sorted);
-			} else if (searchlist) {
-				mailimap_search_result_free(sorted);
-			}
+			FREE_SORT_SEARCH_LISTS();
 			return -1;
 		}
 		if (filter) {
@@ -2088,28 +2106,22 @@ static int fetchlist(struct ws_session *ws, struct imap_client *client, const ch
 			added++;
 			if (res != MAILIMAP_NO_ERROR) {
 				bbs_error("Failed to add sorted seqno to list: %u\n", *seqno);
-				if (sortlist) {
-					mailimap_sort_result_free(sorted);
-				} else if (searchlist) {
-					mailimap_search_result_free(sorted);
-				}
+				FREE_SORT_SEARCH_LISTS();
 				return -1;
 			}
 			if (index >= end) {
 				break;
 			}
 		}
 		bbs_debug(6, "SEARCH/SORT matched %d result%s\n", added, ESS(added));
-		if (sortlist) {
-			mailimap_sort_result_free(sorted);
-		} else if (searchlist) {
-			mailimap_search_result_free(sorted);
-		}
+		FREE_SORT_SEARCH_LISTS();
 	} else {
 		set = mailimap_set_new_interval((uint32_t) start, (uint32_t) end);
 		added = (end - start + 1);
 	}
 
+	bbs_assert(sorted == NULL); /* Should've been freed by now */
+
 	client->start = start;
 	client->end = end;
 

nets/net_ws.c

@@ -49,7 +49,6 @@ static char phpsessprefix[84] = "";
 
 static void ws_log(int level, int len, const char *file, const char *function, int line, const char *buf)
 {
-	/*! \todo should be a version of __bbs_log that doesn't call asprintf, for efficiency (accepts # bytes) */
 	switch (level) {
 		case WS_LOG_ERROR:
 			__bbs_log(LOG_ERROR, 0, file, line, function, "%.*s", len, buf);
@@ -1307,4 +1306,4 @@ static int load_module(void)
 	return 0;
 }
 
-BBS_MODULE_INFO_FLAGS_DEPENDENT("WebSocket Server", MODFLAG_GLOBAL_SYMBOLS, "mod_http.so,net_http.so");
+BBS_MODULE_INFO_FLAGS_DEPENDENT("WebSocket Server", MODFLAG_GLOBAL_SYMBOLS, "mod_http.so");

scripts/run_tests.sh

@@ -75,10 +75,10 @@ install_valgrind() {
 
 if [ "$TEST" = "" ]; then # run all tests
 	# First, do one pass without -e, in case there's a failure, it'll be caught much more quickly
-	tests/test -ddddddddd -DDDDDDDDDD -x || handle_failure
+	tests/test -dddddddddd -DDDDDDDDDD -x || handle_failure
 	# If all good so far, repeat but under valgrind
 	install_valgrind
-	tests/test -ddddddddd -DDDDDDDDDD -ex || handle_failure
+	tests/test -dddddddddd -DDDDDDDDDD -ex || handle_failure
 else
 	# If we are only running a specific test, don't bother with the first pass, just run directly with the -e option (valgrind)
 	install_valgrind

tests/Makefile

@@ -9,6 +9,8 @@ INCLUDE_FILES := $(wildcard ../include/*.h)
 # the include directory is in the parent
 INC = -I..
 
+TEST_LDFLAGS = -shared -fPIC
+
 # Older versions of valgrind do not have the --show-error-list option
 VALGRIND_VERSION_MM := $(shell valgrind --version 2> /dev/null | cut -d'-' -f2 | cut -d'.' -f1-2)
 ifdef VALGRIND_VERSION_MM
@@ -32,10 +34,14 @@ test.o : test.c
 	$(CC) $(CFLAGS) -Wno-unused-result -DTEST_IN_CORE -DTEST_DIR=$(CURDIR) $(EXTRA_FLAGS) $(INC) -c $^
 
 $(TEST_EXE) : test.o readline.o
-	$(CC) $(CFLAGS) -Wl,--export-dynamic -o $(TEST_EXE) *.o -ldl -lpthread
+	$(CC) $(CFLAGS) -Wl,--export-dynamic -o $(TEST_EXE) $^ -ldl -lpthread
 
 %.so : %.o
 	@echo "  [LD] $^ -> $@"
-	$(CC) -shared -fPIC -o $(basename $^).so $^
+	$(CC) $(TEST_LDFLAGS) -o $(basename $^).so $^
+
+test_webmail.so : test_webmail.o
+	@echo "  [LD] $^ -> $@"
+	$(CC) $(TEST_LDFLAGS) -o $(basename $^).so $^ -lwss
 
 .PHONY: all

tests/net_ws.conf

@@ -0,0 +1,7 @@
+[general]
+
+[origins]
+http://localhost = allow
+
+[ws]
+port=8143

tests/test.c

@@ -374,7 +374,7 @@ int test_client_expect_eventually_buf(int fd, int ms, const char *restrict s, in
 			}
 			buf[bytes] = '\0'; /* Safe */
 			/* Probably ends in LF, so skip one here */
-			bbs_debug(10, "Analyzing output: %s", buf); /* Particularly under valgrind, we'll end up reading individual lines more than chunks, so using CLIENT_DRAIN is especially important */
+			bbs_debug(10, "Analyzing output(%d): %s", line, buf); /* Particularly under valgrind, we'll end up reading individual lines more than chunks, so using CLIENT_DRAIN is especially important */
 			/* XXX Should use bbs_readline_append for reliability */
 			if (strstr(buf, s)) {
 				return 0;
@@ -1000,7 +1000,7 @@ static int run_test(const char *filename, int multiple)
 				res = -1;
 				goto cleanup;
 			}
-			bbs_debug(3, "Spawned child process %d\n", childpid);
+			bbs_debug(3, "Spawned child process %d (%s)\n", childpid, option_errorcheck ? "valgrind" : option_strace ? "strace" : "lbbs");
 			/* Wait for the BBS to fully start */
 			/* XXX If we could receive this event outside of the BBS process, that would be more elegant */
 			res = test_bbs_expect("BBS is fully started", SEC_MS(STARTUP_TIMEOUT));

tests/test.h

@@ -53,6 +53,7 @@ struct test_module *TEST_MODULE_SELF_SYM(void);
 #define TEST_NEWS_DIR DIRCAT(TEST_ROOT_DIR, "newsdir")
 #define TEST_TRANSFER_DIR DIRCAT(TEST_ROOT_DIR, "ftp")
 #define TEST_HOME_DIR_ROOT DIRCAT(TEST_TRANSFER_DIR, "home")
+#define TEST_WWW_DIR DIRCAT(TEST_ROOT_DIR, "www")
 
 /* Yuck, but why reinvent the wheel */
 #define TEST_ADD_CONFIG(filename) system("cp " filename " " TEST_CONFIG_DIR)
@@ -68,6 +69,7 @@ struct test_module *TEST_MODULE_SELF_SYM(void);
 
 #define TEST_USER "testuser"
 #define TEST_PASS "P@ssw0rd"
+#define TEST_PASS_BASE64 "UEBzc3cwcmQ="
 #define TEST_HASH "$2y$10$1vtttulZgw5Sz.Ks8PePFumnPCztHfp0YzgHLnuIQ1vAb0mSQpv2q"
 #define TEST_SASL "dGVzdHVzZXIAdGVzdHVzZXIAUEBzc3cwcmQ="
 #define TEST_EMAIL TEST_USER "@" TEST_HOSTNAME