modules, nets: Fix various edge case crashes.

module.c: Don't abort on spurious module decref.
net_imap: Don't abort if client->imap is NULL.
net_imap: Abort __imap_client_send_wait_response
  if bbs_write returns -1.
mod_mail: Fix fopen/fseek arguments for EXPUNGE.
mod_discord: Improve detection of anomalies
  when dropping messages.
socket.c: Refine TCP listener startup logic to
  prevent writing to not-yet-opened pipe, which
  could happen if starting the first TCP listener
  after the BBS was already fully started (and
  subsequently trigger an assertion).

bbs/module.c

@@ -61,9 +61,13 @@ struct bbs_module {
 	struct {
 		/*! This module is awaiting a reload. */
 		unsigned int reloadpending:1;
+		/*! Actively being unloaded. */
+		unsigned int unloading:1;
 	} flags;
 	/*! Load order */
 	int loadorder;
+	/*! Load time */
+	time_t loadtime;
 	/* Module references */
 	struct module_refs refs;
 	/* Next entry */
@@ -160,7 +164,7 @@ void bbs_module_unregister(const struct bbs_module_info *info)
 	}
 }
 
-static void log_module_ref(struct bbs_module *mod, int pair, void *refmod, const char *file, int line, const char *func, int diff)
+static int log_module_ref(struct bbs_module *mod, int pair, void *refmod, const char *file, int line, const char *func, int diff)
 {
 	struct bbs_module_reference *r;
 
@@ -193,9 +197,12 @@ static void log_module_ref(struct bbs_module *mod, int pair, void *refmod, const
 		RWLIST_TRAVERSE_SAFE_END;
 		if (!r) { /* Should only happen legitimately if allocation failed during ref */
 			bbs_error("Failed to find existing reference for %s with pair ID %d\n", mod->name, pair - 1);
+			RWLIST_UNLOCK(&mod->refs);
+			return -1;
 		}
 	}
 	RWLIST_UNLOCK(&mod->refs);
+	return 0;
 }
 
 struct bbs_module *__bbs_module_ref(struct bbs_module *mod, int pair, void *refmod, const char *file, int line, const char *func)
@@ -208,7 +215,20 @@ struct bbs_module *__bbs_module_ref(struct bbs_module *mod, int pair, void *refm
 
 void __bbs_module_unref(struct bbs_module *mod, int pair, void *refmod, const char *file, int line, const char *func)
 {
-	log_module_ref(mod, pair, refmod, file, line, func, -1); /* Do this first, since module can't disappear while it has a positive refcount */
+	int res;
+	bbs_soft_assert(pair >= 0);
+	res = log_module_ref(mod, pair, refmod, file, line, func, -1); /* Do this first, since module can't disappear while it has a positive refcount */
+	if (pair <= 0) {
+		/* Observed in one stack trace where the pair ID was 0.
+		 * In this case, the assertion below failed, suggesting
+		 * that that the refcount was never bumped for the module
+		 * in the first place.
+		 * Thus, this is likely to be invalid and we should just return.
+		 * Worst that can happen is this was a false positive and
+		 * now mod can never be unloaded while the BBS is running. */
+		bbs_error("Not decrementing refcount of %s (pair ID: %d, decref %s)\n", bbs_module_name(mod), pair, res ? "failed" : "succeeded");
+		return;
+	}
 	bbs_atomic_fetchadd_int(&mod->usecount, -1);
 	bbs_assert(mod->usecount >= 0);
 
@@ -721,7 +741,7 @@ static void dec_refcounts(struct bbs_module *mod)
 		dependencies = dependencies_buf;
 		while ((dependency = strsep(&dependencies, ","))) {
 			struct bbs_module *m = find_resource(dependency);
-			bbs_debug(9, "No longer depend on module %s\n", dependency);
+			bbs_debug(9, "%s no longer depends on module %s\n", bbs_module_name(mod), dependency);
 			if (m) {
 				__bbs_unrequire_module(m, mod);
 			} else {
@@ -731,6 +751,33 @@ static void dec_refcounts(struct bbs_module *mod)
 	}
 }
 
+int __bbs_module_is_unloading(struct bbs_module *mod)
+{
+	bbs_assert_exists(mod);
+	return mod->flags.reloadpending;
+}
+
+int __bbs_module_is_shutting_down(struct bbs_module *mod)
+{
+	bbs_assert_exists(mod);
+	return mod->flags.reloadpending || bbs_is_shutting_down();
+}
+
+time_t __bbs_module_load_time(struct bbs_module *mod)
+{
+	bbs_assert_exists(mod);
+	return mod->loadtime;
+}
+
+static inline int __unload_module(struct bbs_module *mod)
+{
+	int res;
+	mod->flags.reloadpending = 1;
+	res = mod->info->unload();
+	mod->flags.reloadpending = 0;
+	return res;
+}
+
 static struct bbs_module *unload_resource_nolock(struct bbs_module *mod, int force, int *usecount, struct stringlist *restrict removed)
 {
 	int res;
@@ -775,7 +822,7 @@ static struct bbs_module *unload_resource_nolock(struct bbs_module *mod, int for
 	}
 
 	bbs_debug(1, "Unloading %s\n", mod->name);
-	res = mod->info->unload();
+	res = __unload_module(mod);
 	if (res) {
 		bbs_warning("Firm unload failed for %s\n", mod->name);
 		if (force <= 2) {
@@ -1401,7 +1448,7 @@ static void unload_modules_helper(void)
 			}
 			/* Module doesn't appear to still be in use (though internally it may be), so try to unload the module. */
 			bbs_debug(2, "Attempting to unload %s\n", mod->name);
-			if (mod->info->unload()) {
+			if (__unload_module(mod)) {
 				/* Could actually still be cleaning up. Skip on this pass. */
 				bbs_debug(2, "Module %s declined to unload, skipping on pass %d\n", mod->name, passes + 1);
 				if (passes == 0) {

bbs/socket.c

@@ -759,12 +759,17 @@ int __bbs_start_tcp_listener(int port, const char *name, void *(*handler)(void *
 	 * One is better performance.
 	 * The other is that we probably don't want to accept TCP connections before we're fully started, anyways. */
 	pthread_mutex_lock(&tcp_start_lock);
-	if (!bbs_is_fully_started()) {
-		if (!tcp_multilistener_started) {
-			bbs_register_startup_callback(start_tcp_multilistener, STARTUP_PRIORITY_URGENT);
-			tcp_multilistener_started = 1;
-		}
-	} else {
+	if (!tcp_multilistener_started) {
+		/* The first time that a module requests a TCP listener,
+		 * we'll need to either queue the multilistener thread to be started
+		 * once startup completes, or go ahead and start it immediately. */
+		bbs_run_when_started(start_tcp_multilistener, STARTUP_PRIORITY_URGENT);
+		tcp_multilistener_started = 1;
+	} else if (bbs_is_fully_started()) {
+		/* If we already started the listener,
+		 * we only need to signal it if the BBS is already started.
+		 * If it's still starting, the listener thread won't start
+		 * listening until startup finishes anyways. */
 		bbs_alertpipe_write(multilistener_alertpipe);
 	}
 	pthread_mutex_unlock(&tcp_start_lock);

include/module.h

@@ -44,10 +44,36 @@ struct bbs_module_info {
 /*! \brief Get name of a module */
 const char *bbs_module_name(const struct bbs_module *mod);
 
+/*!
+ * \brief Whether this module is currently being unloaded
+ * \retval 1 if unload actively being attempted
+ * \retval 0 if not
+ */
+#define bbs_module_is_unloading() __bbs_module_is_unloading(BBS_MODULE_SELF)
+
+int __bbs_module_is_unloading(struct bbs_module *mod);
+
+/*!
+ * \brief Whether this module is currently being unloaded and/or the BBS is shutting down
+ * \retval 1 if unload or shutdown actively being attempted
+ * \retval 0 if not
+ */
+#define bbs_module_is_shutting_down() __bbs_module_is_shutting_down(BBS_MODULE_SELF)
+
+int __bbs_module_is_shutting_down(struct bbs_module *mod);
+
+/*!
+ * \brief Get load time of this module
+ * \return Module load time in epoch time
+ */
+#define bbs_module_load_time() __bbs_module_load_time(BBS_MODULE_SELF)
+
+time_t __bbs_module_load_time(struct bbs_module *mod);
+
 /*!
  * \brief Increment ref count of a module
  * \param mod
- * \param pairid A unique ID only used once in a source file for a corresponding ref/unref sequence.
+ * \param pairid A positive unique ID only used once in a source file for a corresponding ref/unref sequence.
  */
 #define bbs_module_ref(mod, pairid) __bbs_module_ref(mod, pairid, BBS_MODULE_SELF, __FILE__, __LINE__, __func__)
 
@@ -56,7 +82,7 @@ struct bbs_module *__bbs_module_ref(struct bbs_module *mod, int pair, void *refm
 /*!
  * \brief Decrement ref count of a module
  * \param mod
- * \param pairid A unique ID only used once in a source file for a corresponding ref/unref sequence.
+ * \param pairid A positive unique ID only used once in a source file for a corresponding ref/unref sequence.
  */
 #define bbs_module_unref(mod, pairid) __bbs_module_unref(mod, pairid, BBS_MODULE_SELF, __FILE__, __LINE__, __func__)
 

modules/mod_discord.c

@@ -846,6 +846,25 @@ static void dump_user(struct bbs_node *node, int fd, const char *requsername, st
 	irc_relay_who_response(node, fd, "Discord", requsername, combined, unique, !(u->status == STATUS_IDLE || u->status == STATUS_OFFLINE));
 }
 
+static inline int discord_is_ready(void)
+{
+	time_t now, diff;
+
+	if (likely(discord_ready)) {
+		return 1;
+	}
+
+	bbs_debug(1, "Discord is %s, dropping message\n", bbs_module_is_shutting_down() ? "shutting down" : "not yet ready");
+	now = time(NULL);
+	diff = now - bbs_module_load_time();
+	if (diff > 60 && !bbs_module_is_shutting_down()) {
+		/* We shouldn't need more than a minute to initialize.
+		 * If this happens, something is probably wrong. */
+		bbs_warning("Discord not yet initialized after %lu seconds?\n", diff);
+	}
+	return 0;
+}
+
 /*!
  * \param fd
  * \param numeric: 318 = WHOIS, 352 = WHO, 353 = NAMES
@@ -865,8 +884,7 @@ static int nicklist(struct bbs_node *node, int fd, int numeric, const char *requ
 	bbs_debug(9, "Nicklist callback for %d: %s/%s\n", numeric, S_IF(channel), S_IF(user));
 #endif
 
-	if (!discord_ready) {
-		bbs_debug(1, "Discord is not yet ready, dropping message\n");
+	if (!discord_is_ready()) {
 		return 0;
 	} else if (!expose_members) {
 		bbs_debug(5, "Ignoring since exposemembers=no\n");
@@ -973,8 +991,7 @@ static int discord_send(const char *channel, const char *sender, const char *msg
 	struct chan_pair *cp;
 	int handled = 0;
 
-	if (!discord_ready) {
-		bbs_debug(1, "Discord is not yet ready, dropping message\n");
+	if (!discord_is_ready()) {
 		return 0;
 	} else if (!(cp = find_mapping_irc(channel))) {
 		/* No relay exists for this channel */

modules/mod_mail.c

@@ -1345,7 +1345,7 @@ unsigned long maildir_indicate_expunged(enum mailbox_event_type type, struct bbs
 	maxmodseq = __maildir_modseq(mbox, directory, 1); /* Must be atomic */
 
 	snprintf(modseqfile, sizeof(modseqfile), "%s/../.modseqs", directory);
-	fp = fopen(modseqfile, "wb");
+	fp = fopen(modseqfile, "ab");
 	if (!fp) {
 		bbs_error("Failed to open %s\n", modseqfile);
 		mailbox_uid_unlock(mbox);
@@ -1361,10 +1361,13 @@ unsigned long maildir_indicate_expunged(enum mailbox_event_type type, struct bbs
 	 */
 
 	/* Check if we created the file or opened an existing one by getting our position.
+	 * Per fopen(3), when appending, stream is positioned at end of file.
+	 * Thus, we can use the current file offset when opening the file to determine if we just created it
+	 * (this assumes that we didn't create an empty file early, i.e. if the file existed it was non-empty).
 	 * If the file is empty, write HIGHESTMODSEQ first, then the expunged messages.
 	 * Otherwise, append all the expunged messages, then seek back to the beginning and overwrite HIGHESTMODSEQ. */
 	if (fseek(fp, -1, SEEK_END)) {
-		bbs_warning("fseek failed: %s\n", strerror(errno));
+		bbs_warning("fseek(%s) failed: %s\n", modseqfile, strerror(errno));
 	}
 	pos = ftell(fp);
 	bbs_debug(7, "Current position is %ld\n", pos);

nets/net_imap/imap_client.c

@@ -451,7 +451,7 @@ ssize_t __imap_client_send_log(struct imap_client *client, int log, const char *
 int __imap_client_send_wait_response(struct imap_client *client, int fd, int ms, int echo, int lineno, int (*cb)(struct imap_client *client, const char *buf, size_t len, void *cbdata), void *cbdata, const char *fmt, ...)
 {
 	char *buf;
-	int len, res;
+	int len, res = -1;
 	char tagbuf[15];
 	int taglen;
 	va_list ap;
@@ -484,11 +484,16 @@ int __imap_client_send_wait_response(struct imap_client *client, int fd, int ms,
 #else
 	UNUSED(lineno);
 #endif
-	bbs_write(client->client.wfd, tagbuf, (unsigned int) taglen);
-	bbs_write(client->client.wfd, buf, (unsigned int) len);
+	if (bbs_write(client->client.wfd, tagbuf, (unsigned int) taglen) < 0) {
+		goto cleanup;
+	} else if (bbs_write(client->client.wfd, buf, (unsigned int) len)) {
+		goto cleanup;
+	}
 	imap_debug(7, "=> %s%s", tagbuf, buf);
 	/* Read until we get the tagged respones */
 	res = client_command_passthru(client, fd, tagbuf, taglen, buf, len, ms, echo, cb, cbdata) <= 0;
+
+cleanup:
 	free(buf);
 	return res;
 }

nets/net_imap/imap_client_status.c

@@ -506,7 +506,13 @@ ssize_t remote_status(struct imap_client *client, const char *remotename, const
 	const char *add1, *add2, *add3;
 	int issue_status = 1;
 
-	bbs_assert_exists(client->imap); /* Added in response to a segfault observed on the following line */
+	if (!client->imap) {
+		/* Originally added as an assertion in response to a segfault observed on the following line.
+		 * Not yet sure how this can happen, but if it does, don't crash. */
+		bbs_error("Remote IMAP client does not have an associated IMAP session???\n");
+		bbs_soft_assert(0);
+		return -1;
+	}
 	tag = client->imap->tag;
 
 	/* In order for caching of SIZE to be reliable, we must invalidate it whenever anything