lbbs: Fix various code defects.

Fixes various defects throughout the codebase, in particular:

* resource leaks
* locking issues
* unchecked return values
* assignments instead of comparisons
* possible NULL dereferences
* uninitialized variables

Author: InterLinked1

bbs/bbs.c

@@ -740,6 +740,7 @@ static void *monitor_sig_flags(void *unused)
 			break;
 		} else {
 			/* Shouldn't ever happen... */
+			pthread_mutex_unlock(&sig_lock);
 			bbs_warning("Received unactionable activity on sig alert pipe?\n");
 		}
 	}
@@ -776,6 +777,7 @@ static long bbs_is_running(void)
 	}
 	if (fscanf(f, "%ld", &file_pid) == EOF) {
 		bbs_debug(5, "PID file %s does not contain a PID\n", BBS_PID_FILE);
+		fclose(f);
 		return 0;
 	}
 	fclose(f);

bbs/curl.c

@@ -135,7 +135,10 @@ static int curl_common_setup(CURL **curl_ptr, const char *url)
 	curl_easy_setopt(curl, CURLOPT_NOSIGNAL, 1);
 	curl_easy_setopt(curl, CURLOPT_TIMEOUT, 20); /* Max 20 seconds */
 
-	curl_easy_setopt(curl, CURLOPT_URL, url);
+	if (curl_easy_setopt(curl, CURLOPT_URL, url)) {
+		bbs_warning("Failed to set cURL option CURLOPT_URL\n");
+		return -1;
+	}
 	return 0;
 }
 
@@ -187,7 +190,10 @@ static int curl_common_run(CURL *curl, struct bbs_curl *c, FILE *fp)
 	if (fp) {
 		/* Write response body to a file */
 		curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, NULL);
-		curl_easy_setopt(curl, CURLOPT_WRITEDATA, fp);
+		if (curl_easy_setopt(curl, CURLOPT_WRITEDATA, fp)) {
+			bbs_warning("Failed to set cURL opt CURLOPT_WRITEDATA\n");
+			return -1;
+		}
 	} else {
 		/* Write response body to an allocated string */
 		curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, WriteMemoryCallback);
@@ -304,7 +310,10 @@ int bbs_curl_post(struct bbs_curl *c)
 	if (curl_common_setup(&curl, c->url)) {
 		return -1;
 	}
-	curl_easy_setopt(curl, CURLOPT_POST, 1);
+	if (curl_easy_setopt(curl, CURLOPT_POST, 1)) {
+		bbs_warning("Failed to set cURL option CURLOPT_POST\n");
+		return -1;
+	}
 	if (c->postfields) {
 		curl_easy_setopt(curl, CURLOPT_POSTFIELDS, c->postfields);
 	} else {

bbs/fd.c

@@ -222,9 +222,9 @@ int __fdleak_socket(int domain, int type, int protocol, const char *file, int li
 	}
 
 	if (sproto) {
-		STORE_COMMON(res, "socket", "%s,%s,\"%s\"", sdomain, stype, sproto);
+		STORE_COMMON_HELPER(res, "socket", "%s,%s,\"%s\"", sdomain, stype, sproto);
 	} else {
-		STORE_COMMON(res, "socket", "%s,%s,\"%d\"", sdomain, stype, protocol);
+		STORE_COMMON_HELPER(res, "socket", "%s,%s,\"%d\"", sdomain, stype, protocol);
 	}
 	return res;
 }

bbs/module.c

@@ -627,7 +627,7 @@ static int unload_dependencies(struct bbs_module *mod, struct stringlist *restri
 		int usecount;
 		/* The module MAY have already been unloaded, in which case m is no longer valid memory.
 		 * Check to see if it's in the list. */
-		if (stringlist_contains(removed, r->name)) {
+		if (removed && stringlist_contains(removed, r->name)) {
 			free(r);
 			continue;
 		}

bbs/range.c

@@ -143,6 +143,7 @@ int uintlist_append2(unsigned int **a, unsigned int **b, int *lengths, int *allo
 			newb = realloc(*b, (size_t) newallocsize * sizeof(unsigned int));
 			if (ALLOC_FAILURE(newb)) {
 				/* This is tricky. We expanded a but failed to expand b. Keep the smaller size for our records. */
+				free(newa);
 				return -1;
 			}
 			*allocsizes = newallocsize;
@@ -282,4 +283,4 @@ int range_to_uintlist(char *s, unsigned int **list, int *length)
 		}
 	}
 	return 0;
-}
+}

bbs/socket.c

@@ -300,11 +300,12 @@ int bbs_resolve_hostname(const char *hostname, char *buf, size_t len)
 		if (ai->ai_family == AF_INET) {
 			saddr_in = (struct sockaddr_in *) ai->ai_addr;
 			inet_ntop(ai->ai_family, &saddr_in->sin_addr, buf, (socklen_t) len); /* Print IPv4*/
+			break; /* Use the 1st one that works */
 		} else if (ai->ai_family == AF_INET6) {
 			saddr_in6 = (struct sockaddr_in6 *) ai->ai_addr;
 			inet_ntop(ai->ai_family, &saddr_in6->sin6_addr, buf, (socklen_t) len); /* Print IPv6 */
+			break; /* Use the 1st one that works */
 		}
-		break; /* Use the 1st one that works */
 	}
 
 	freeaddrinfo(res);
@@ -357,7 +358,12 @@ int bbs_tcp_connect(const char *hostname, int port)
 		 * Using SO_SNDTIMEO works on Linux and is easier than doing bbs_unblock_fd before and bbs_block_fd after. */
 		timeout.tv_sec = 4; /* Wait up to 4 seconds to connect */
 		timeout.tv_usec = 0;
-		setsockopt(sfd, SOL_SOCKET, SO_SNDTIMEO, &timeout, sizeof(timeout));
+		if (setsockopt(sfd, SOL_SOCKET, SO_SNDTIMEO, &timeout, sizeof(timeout))) {
+			bbs_error("setsockopt failed: %s\n", strerror(errno));
+			close(sfd);
+			sfd = -1;
+			continue;
+		}
 		if (connect(sfd, ai->ai_addr, ai->ai_addrlen)) {
 			bbs_error("connect: %s\n", strerror(errno));
 			close(sfd);

bbs/utils.c

@@ -561,12 +561,6 @@ int bbs_dir_has_file_prefix(const char *path, const char *prefix)
 	}
 
 	closedir(dir);
-
-	if (res < 0 && errno) {
-		bbs_error("Error while reading directories (%d) - %s: %s\n", res, path, strerror(errno));
-		res = -1;
-	}
-
 	return res;
 }
 
@@ -598,12 +592,6 @@ int bbs_dir_has_subdirs(const char *path)
 	}
 
 	closedir(dir);
-
-	if (res < 0 && errno) {
-		bbs_error("Error while reading directories (%d) - %s: %s\n", res, path, strerror(errno));
-		res = -1;
-	}
-
 	return res;
 }
 
@@ -682,6 +670,7 @@ static int __bbs_dir_size(const char *path, long *size, int max_depth)
 				/* Don't use alloca or allocate on the stack, because we're in a loop */
 				full_path = malloc(strlen(path) + strlen(entry->d_name) + 2);
 				if (!full_path) {
+					closedir(dir);
 					return -1;
 				}
 				sprintf(full_path, "%s/%s", isroot ? "" : path, entry->d_name); /* Safe */
@@ -829,7 +818,9 @@ FILE *bbs_mkftemp(char *template, mode_t mode)
 	int pfd;
 
 	pfd = mkstemp(template); /* template will get updated to contain the actual filename */
-	chmod(template, mode);
+	if (chmod(template, mode)) {
+		bbs_warning("Failed to set permissions for %s: %s\n", template, strerror(errno));
+	}
 
 	if (pfd == -1) {
 		/* It's not specified if mkstemp sets errno, but check it anyways */

bbs/variables.c

@@ -228,6 +228,7 @@ int bbs_node_var_set(struct bbs_node *node, const char *key, const char *value)
 		if (!node->vars) {
 			vars = calloc(1, sizeof(*vars));
 			if (ALLOC_FAILURE(vars)) {
+				bbs_node_unlock(node);
 				return -1;
 			}
 			bbs_debug(5, "Allocated variable list for node %d\n", node->id);

doors/door_irc.c

@@ -190,6 +190,7 @@ static int load_config(void)
 		ircl = irc_client_new(hostname, port, username, password);
 		if (!ircl) {
 			free(client);
+			free_if(msgscript);
 			continue;
 		}
 		irc_client_autojoin(ircl, autojoin);
@@ -306,6 +307,7 @@ static struct participant *join_client(struct bbs_node *node, const char *name)
 	}
 	if (!client) {
 		bbs_error("IRC client %s doesn't exist\n", name);
+		RWLIST_UNLOCK(&clients);
 		return NULL;
 	}
 	/* Okay, we have the client. Add the newcomer to it. */

include/version.h

@@ -15,4 +15,4 @@
 
 #define BBS_MAJOR_VERSION 0
 #define BBS_MINOR_VERSION 4
-#define BBS_PATCH_VERSION 4
+#define BBS_PATCH_VERSION 5

modules/mod_auth_static.c

@@ -120,6 +120,7 @@ static struct bbs_user **get_users(void)
 	RWLIST_RDLOCK(&users);
 	userlist = malloc((size_t) (RWLIST_SIZE(&users, u, entry) + 1) * sizeof(*user)); /* The list will be NULL terminated, so add 1 */
 	if (ALLOC_FAILURE(userlist)) {
+		RWLIST_UNLOCK(&users);
 		return NULL;
 	}
 	/* Keep list locked here so count can't change on us */

modules/mod_mail.c

@@ -778,6 +778,9 @@ unsigned long mailbox_quota(struct mailbox *mbox)
 		fclose(fp);
 	} else {
 		mbox->quota = (unsigned int) maxquota;
+		if (fp) {
+			fclose(fp);
+		}
 	}
 	return mbox->quota;
 }
@@ -1221,6 +1224,7 @@ static unsigned long __maildir_modseq(struct mailbox *mbox, const char *director
 	if (res != 1) {
 		bbs_error("Error reading HIGHESTMODSEQ from %s\n", modseqfile);
 		/* No sane thing we can do here either */
+		fclose(fp);
 		return 0;
 	}
 

modules/mod_mysql.c

@@ -594,9 +594,11 @@ int sql_fetch_columns(int bind_ints[], long long bind_longs[], char *bind_string
 			break;
 		case 'b': /* Blob */
 			bbs_warning("Blobs are currently unsupported\n");
+			va_end(ap);
 			return -1;
 		default:
 			bbs_warning("Unknown SQL format type specifier: %c\n", *cur);
+			va_end(ap);
 			return -1;
 		}
 	}

modules/mod_relay_irc.c

@@ -254,6 +254,7 @@ static int wait_response(struct bbs_node *node, int fd, const char *requsername,
 	numericclient = clientname;
 	if (pipe(nickpipe)) {
 		bbs_error("pipe failed: %s\n", strerror(errno));
+		pthread_mutex_unlock(&nicklock);
 		return -1;
 	}
 	switch (numeric) {
@@ -860,16 +861,14 @@ static void doormsg_cb(const char *clientname, const char *channel, const char *
 			if (*msg == '<') {
 				safe_strncpy(msgbuf, msg, sizeof(msgbuf));
 				tmp = strchr(msgbuf, '>');
-				if (msg) {
-					*tmp++ = '\0';
-					msg = tmp;
-					sendnick = msgbuf + 1;
-					/* Okay, now the message is just the message, and we have extracted the real sender name */
-					snprintf(nativenick, sizeof(nativenick), "%s/%s", clientname,  sendnick);
+				*tmp++ = '\0';
+				msg = tmp;
+				sendnick = msgbuf + 1;
+				/* Okay, now the message is just the message, and we have extracted the real sender name */
+				snprintf(nativenick, sizeof(nativenick), "%s/%s", clientname,  sendnick);
 #pragma GCC diagnostic pop /* -Wformat-truncation */
-					sendnick = nativenick;
-					/* Now we have a unique nick that doesn't conflict with this same nick on our local IRC server */
-				}
+				sendnick = nativenick;
+				/* Now we have a unique nick that doesn't conflict with this same nick on our local IRC server */
 			}
 			irc_relay_send(cp->channel1, CHANNEL_USER_MODE_NONE, S_OR(cp->client2, clientname), sendnick, NULL, msg, cp->ircuser);
 		}

modules/mod_sieve.c

@@ -388,6 +388,7 @@ static int my_getheader(sieve2_context_t *s, void *varg)
 	fp = fopen(sieve->mproc->datafile, "r");
 	if (!fp) {
 		bbs_error("Failed to open %s: %s\n", sieve->mproc->datafile, strerror(errno));
+		free(headers);
 		return SIEVE2_ERROR_FAIL;
 	}
 

modules/mod_slack.c

@@ -606,6 +606,7 @@ static int on_message(struct slack_event *event, const char *channel, const char
 		pthread_mutex_lock(&u->lock);
 		if (!strcmp(u->lastmsg, text)) {
 			pthread_mutex_unlock(&cp->lock);
+			pthread_mutex_unlock(&u->lock);
 			bbs_debug(4, "Not echoing our own direct message post...\n");
 			return 0;
 		}
@@ -967,7 +968,9 @@ static int start_clients(void)
 	struct slack_relay *relay;
 	RWLIST_RDLOCK(&relays);
 	RWLIST_TRAVERSE(&relays, relay, entry) {
-		bbs_pthread_create(&relay->thread, NULL, slack_relay, relay);
+		if (bbs_pthread_create(&relay->thread, NULL, slack_relay, relay)) {
+			bbs_warning("Failed to start Slack relay %s\n", relay->name);
+		}
 	}
 	RWLIST_UNLOCK(&relays);
 	return 0;

modules/mod_smtp_delivery_external.c

@@ -352,7 +352,7 @@ static int try_send(struct smtp_session *smtp, struct smtp_tx_data *tx, const ch
 	off_t send_offset = offset;
 	int caps = 0, maxsendsize = 0;
 	char sendercopy[64];
-	char *user, *domain;
+	char *user, *domain, *saslstr = NULL;
 
 	bbs_assert(datafd != -1);
 	bbs_assert(writelen > 0);
@@ -483,7 +483,7 @@ static int try_send(struct smtp_session *smtp, struct smtp_tx_data *tx, const ch
 				}
 			}
 		} else if (caps & SMTP_CAPABILITY_AUTH_LOGIN) {
-			char *saslstr = bbs_sasl_encode(username, username, password);
+			saslstr = bbs_sasl_encode(username, username, password);
 			if (!saslstr) {
 				res = -1;
 				goto cleanup;
@@ -554,6 +554,7 @@ static int try_send(struct smtp_session *smtp, struct smtp_tx_data *tx, const ch
 	res = 0;
 
 cleanup:
+	free_if(saslstr);
 	if (res > 0) {
 		smtp_client_send(&client, "QUIT\r\n");
 	}

modules/mod_smtp_mailing_lists.c

@@ -273,7 +273,7 @@ static int listify(struct mailing_list *l, struct smtp_response *resp, FILE *fp,
 			firstword = strsep(&subjectcopy, " ");
 			/* Ignore Re:, Fwd:, Fw:, and such prefixes. In fact, just ignore all prefixes. */
 			tmp = strchr(firstword, ':');
-			if (tmp && !++tmp) { /* Ends in : */
+			if (tmp && !*++tmp) { /* Ends in : */
 				has_prefix = 1;
 			} else if (!strcmp(l->tag, firstword)) {
 				has_prefix = 1;

modules/mod_test_range.c

@@ -67,6 +67,7 @@ static int test_range_generation(void)
 	return 0;
 
 cleanup:
+	free_if(ranges);
 	return -1;
 }
 

modules/mod_webmail.c

@@ -1236,6 +1236,10 @@ static void append_header_meta(json_t *restrict json, char *headers, int fetchli
 
 		if (isspace(header[0])) {
 			/* Continuation of previous header */
+			if (!prevval) {
+				bbs_warning("No previous header to continue?\n");
+				continue;
+			}
 			dyn_str_append(&dynstr, prevval, strlen(prevval));
 			prevval = header;
 		} else {
@@ -2266,7 +2270,7 @@ static int fetch_mime_recurse(json_t *root, json_t *attachments, struct mailmime
 							case MAILIMF_FIELD_SUBJECT:
 								subject = f->fld_data.fld_subject;
 								decoded = subject ? mime_header_decode(subject->sbj_value) : NULL;
-								name = decoded ? decoded : subject->sbj_value;
+								name = decoded ? decoded : subject ? subject->sbj_value : NULL;
 								bbs_debug(5, "Subject: %s\n", name);
 								json_set_header(root, "subject", json_string(name));
 								free_if(decoded);

nets/net_imap.c

@@ -2040,7 +2040,7 @@ static int handle_move(struct imap_session *imap, const char *sequences, const c
 	unsigned int *olduids = NULL, *newuids = NULL, *expunged = NULL, *expungedseqs = NULL;
 	int lengths = 0, allocsizes = 0;
 	int exp_lengths = 0, exp_allocsizes = 0;
-	unsigned int uidvalidity, uidnext, uidres;
+	unsigned int uidvalidity = 0, uidnext, uidres;
 	char *olduidstr = NULL, *newuidstr = NULL;
 	int destacl;
 	int error = 0;
@@ -2062,6 +2062,7 @@ static int handle_move(struct imap_session *imap, const char *sequences, const c
 	files = scandir(imap->curdir, &entries, NULL, imap_uidsort);
 	if (files < 0) {
 		bbs_error("scandir(%s) failed: %s\n", imap->curdir, strerror(errno));
+		mailbox_unlock(imap->mbox);
 		return -1;
 	}
 	while (fno < files && (entry = entries[fno++])) {
@@ -2770,6 +2771,7 @@ static int handle_remote_move(struct imap_session *imap, char *dest, const char
 			}
 			if (items_received < 3) {
 				bbs_warning("Incomplete download, only got %d/3 items\n", items_received);
+				close_if(destfd);
 				goto cleanup;
 			}
 			/* Finish processing */

nets/net_imap/imap_server_search.c

@@ -2079,6 +2079,7 @@ static int thread_references_step1(struct thread_messageid *tmsgids, struct thre
 #endif
 				childmsg = push_threadmsgid(tmsgids, ref, 1);
 				if (ALLOC_FAILURE(childmsg)) {
+					free(refdup);
 					return -1;
 				}
 			}
@@ -2222,7 +2223,6 @@ static int thread_references_step3(struct thread_messageid *msgids, unsigned int
 #endif
 
 	/* Prune dummy messages */
-	t = msgids;
 	t = msgids->llnext; /* Skip the dummy root */
 	while (t) {
 		struct thread_messageid *next = t->llnext; /* Since we could free t within the loop */

nets/net_irc.c

@@ -3324,6 +3324,7 @@ static void irc_handler(struct bbs_node *node, int secure)
 		ssl = ssl_node_new_accept(node, &rfd, &wfd);
 		if (!ssl) {
 			bbs_error("Failed to create SSL\n");
+			free(user);
 			return;
 		}
 	} else {