net_imap: Fix duplicated/malformed RFC822.HEADER FETCH response.

The handling for RFC822.HEADER previously resulted in output being
flushed immediately, before the response for that message even began.
Move it to afterwards, and also eliminate duplicate handling of this
FETCH item.

Additionally, send_message did not correctly handle the HEADERS_ONLY
case, as it would have the length of the headers but the offset of
the body start, rather than the headers start. This would result in
a request to bbs_sendfile to send past the end of the file if the
message's body was short enough, resulting in an infinite loop since
sendfile would return 0. To address this, bbs_sendfile now handles
a return of 0 by waiting for the file descriptor to become writable,
and also checks if the offset exceeds the file size. send_message
is also fixed to correctly handle HEADERS_ONLY.

A test has been added to verify both of the above.

LBBS-85 #close

bbs/socket.c

@@ -2487,6 +2487,7 @@ ssize_t bbs_timed_write(int fd, const char *buf, size_t len, int ms)
 		return -1;
 	}
 
+	memset(&pfd, 0, sizeof(pfd));
 	pfd.fd = fd;
 	pfd.events = POLLOUT;
 
@@ -2503,17 +2504,42 @@ ssize_t bbs_timed_write(int fd, const char *buf, size_t len, int ms)
 	return res;
 }
 
+/* \note Assumes fd is a file descriptor corresponding to a file */
+static inline off_t fd_get_filesize(int fd)
+{
+	off_t max_offset, cur_offset;
+
+	cur_offset = lseek(fd, 0, SEEK_CUR);
+	if (cur_offset == -1) {
+		bbs_error("Failed to get current file position: %s\n", strerror(errno));
+		return -1;
+	}
+	max_offset = lseek(fd, 0, SEEK_END);
+	if (max_offset == -1) {
+		bbs_error("Failed to seek to end of file: %s\n", strerror(errno));
+		return -1;
+	}
+	lseek(fd, cur_offset, SEEK_SET); /* Restore original offset */
+	return max_offset;
+}
+
 ssize_t bbs_sendfile(int out_fd, int in_fd, off_t *offset, size_t count)
 {
 	/* sendfile (like write) can return before writing all the requested bytes.
 	 * This wrapper function will retry as appropriate to fully write the message as best we can. */
 	ssize_t written = 0;
+	off_t localoffset = 0;
+	struct pollfd pfd;
+
+	memset(&pfd, 0, sizeof(pfd));
+	pfd.fd = out_fd;
+	pfd.events = POLLOUT;
 
 	for (;;) {
 #ifdef __linux__
-		ssize_t res = sendfile(out_fd, in_fd, offset, count);
+		ssize_t res = sendfile(out_fd, in_fd, offset ? offset : &localoffset, count);
 #elif defined(__FreeBSD__)
-		ssize_t res = sendfile(out_fd, in_fd, *offset, count, NULL, NULL, 0);
+		ssize_t res = sendfile(out_fd, in_fd, offset ? *offset : localoffset, count, NULL, NULL, 0);
 #else
 #error "Missing sendfile"
 #endif
@@ -2527,8 +2553,48 @@ ssize_t bbs_sendfile(int out_fd, int in_fd, off_t *offset, size_t count)
 		}
 		/* It's typical for sendfile to only send 8,192 bytes at a time,
 		 * so for sending much more than that, this can be a very spammy message. */
-		bbs_debug(10, "Wanted to write %lu bytes but was only able to write %ld this round\n", count, res); /* Keep trying */
+		bbs_debug(10, "Wanted to write %lu bytes at offset %ld, but was only able to write %ld this round\n", count, offset ? *offset : localoffset, res); /* Keep trying */
 		count -= (size_t) res;
+		/* if offset is not NULL, then on Linux, sendfile automatically increments it
+		 * by the number of sent bytes. */
+#ifdef __FreeBSD__
+		/* Also adjust the offset for sendfile */
+		if (offset) {
+			*offset += res;
+		} else {
+			localoffset += res;
+		}
+#endif
+		if (!res) {
+			int pres;
+			/* Wasn't able to write any bytes this round.
+			 * This means that either the file descriptor isn't writable right now,
+			 * or the specified offset/byte count is not valid. */
+			off_t eff_offset = offset ? *offset : localoffset;
+			off_t size = fd_get_filesize(in_fd);
+			if (size > 0 && eff_offset >= size) {
+				/* Offset is at the end of the file or past it.
+				 * This will never work. */
+				bbs_error("Specified file offset (%ld) exceeds file size (%ld)\n", eff_offset, size);
+				return -1;
+			}
+			bbs_debug(8, "Waiting for file descriptor %d to become writable\n", out_fd);
+			pfd.revents = 0;
+			pres = poll(&pfd, 1, SEC_MS(30));
+			if (pres < 0) {
+				if (errno != EINTR) {
+					bbs_error("poll failed: %s\n", strerror(errno));
+					return -1;
+				}
+			} else if (!pres) {
+				/* Can't wait forever... */
+				bbs_warning("File descriptor %d did not become writable after 30 seconds\n", out_fd);
+				return -1;
+			} else if (!(pfd.revents & POLLOUT)) {
+				bbs_warning("Exceptional activity on file descriptor %d while waiting for it to become writable\n", out_fd);
+				return -1;
+			}
+		}
 	}
 
 	return written;

modules/mod_webmail.c

@@ -1827,6 +1827,15 @@ static void fetchlist_single(struct mailimap_msg_att *msg_att, json_t *arr)
 					append_internaldate(msgitem, item->att_data.att_static->att_data.att_internal_date);
 					break;
 				case MAILIMAP_MSG_ATT_RFC822_SIZE:
+					/* The number of octets in the message.
+					 * Note: Microsoft IMAP server has (yet another) major bug where it returns an RFC822.SIZE
+					 * that is 4-5 times the actual size of the message.
+					 * I have reported this, but who knows if it will actually get fixed, or if anyone there even cares.
+					 * Offline clients show the correct size, since they download the whole message,
+					 * but there is no hope for webmail clients, we have to take the server's response
+					 * to be correct (as with most things), and it would be inefficient to download
+					 * every single message in a FETCHLIST just to compute the size.
+					 * TL;DR, if your server is stupid, this information may also be wrong. */
 					json_object_set_new(msgitem, "size", json_integer(item->att_data.att_static->att_data.att_rfc822_size));
 					break;
 				case MAILIMAP_MSG_ATT_BODY_SECTION:

nets/net_imap.c

@@ -271,7 +271,7 @@ static int cli_imap_clients(struct bbs_cli_args *a)
 	struct imap_session *imap;
 	time_t now = time(NULL);
 
-	bbs_dprintf(a->fdout, "%4s %-25s %6s %4s %8s %10s %s\n", "Node", "Name", "Active", "Dead", "Idle", "Age", "TCP Client");
+	bbs_dprintf(a->fdout, "%4s %-25s %6s %4s %9s %10s %s\n", "Node", "Name", "Active", "Dead", "Idle", "Age", "TCP Client");
 	RWLIST_RDLOCK(&sessions);
 	RWLIST_TRAVERSE(&sessions, imap, entry) {
 		struct imap_client *client;
@@ -281,7 +281,7 @@ static int cli_imap_clients(struct bbs_cli_args *a)
 			time_t idle_elapsed = now - client->idlestarted;
 			bbs_soft_assert(now >= client->idlestarted);
 			print_time_elapsed(client->created, now, elapsed, sizeof(elapsed));
-			bbs_dprintf(a->fdout, "%4u %-25s %6s %4s %3" TIME_T_FMT "/%4d %10s %p\n",
+			bbs_dprintf(a->fdout, "%4u %-25s %6s %4s %4" TIME_T_FMT "/%4d %10s %p\n",
 				imap->node->id, client->name, BBS_YN(client->active), BBS_YN(client->dead), client->idling ? idle_elapsed : 0, client->maxidlesec, elapsed, &client->client);
 		}
 		RWLIST_UNLOCK(&imap->clients);

nets/net_imap/imap_client_status.c

@@ -576,6 +576,10 @@ ssize_t remote_status(struct imap_client *client, const char *remotename, const
 	}
 
 	if (issue_status) {
+		/* Necessary even though we do it above? */
+		if (client->idling) {
+			imap_client_idle_stop(client);
+		}
 		/* XXX Same tag is reused here, so we expect the same prefix (rtag) */
 		imap_client_send(client, "A.%s.1 STATUS \"%s\" (%s%s%s%s)\r\n", tag, remotename, items, add1, add2, add3);
 		for (;;) {

nets/net_imap/imap_server_fetch.c

@@ -576,8 +576,7 @@ static ssize_t send_message(struct imap_session *imap, struct fetch_body_request
 		offset = compute_body_offset(*fp);
 		sendsize = *size - (size_t) offset;
 	} else if (sendmask == HEADERS_ONLY) {
-		offset = compute_body_offset(*fp);
-		sendsize = (size_t) offset;
+		sendsize = (size_t) compute_body_offset(*fp);
 	} else {
 		sendsize = *size;
 	}
@@ -762,9 +761,6 @@ static int process_fetch_finalize(struct imap_session *imap, struct fetch_reques
 	bbs_node_cork(imap->node, 1); /* Cork the TCP session */
 
 	/* Do as much as possible before we even start the reply, to minimize number of writes */
-	if (fetchreq->rfc822header) {
-		send_headers(imap, NULL, "RFC822.HEADER", &fp, fullname);
-	}
 	if (fetchreq->body || fetchreq->bodystructure) {
 		char *bodystructure;
 		/* BODY is BODYSTRUCTURE without extensions (which we don't send anyways, in either case) */
@@ -790,7 +786,12 @@ static int process_fetch_finalize(struct imap_session *imap, struct fetch_reques
 		send_message(imap, NULL, "RFC822", &fp, &size, fullname, BOTH);
 	}
 	if (fetchreq->rfc822header) {
+		/* XXX In theory, these should behave identically, not sure which one to use? */
+#if 1
 		send_message(imap, NULL, "RFC822.HEADER", &fp, &size, fullname, HEADERS_ONLY);
+#else
+		send_headers(imap, NULL, "RFC822.HEADER", &fp, fullname);
+#endif
 	}
 	if (fetchreq->rfc822text) { /* Same as BODY[TEXT] */
 		send_message(imap, NULL, "RFC822.TEXT", &fp, &size, fullname, BODY_ONLY);

tests/test_imap_fetch.c

@@ -89,7 +89,7 @@ static int write_file_to_socket(int client, const char *filename)
 	return 0;
 }
 
-static int send_count = 0;
+static int send_count;
 
 static int send_message(int client1, const char *filename)
 {
@@ -125,11 +125,48 @@ static int send_message(int client1, const char *filename)
 	return -1;
 }
 
+static int send_short_message(int client1)
+{
+	if (!send_count++) {
+		CLIENT_EXPECT_EVENTUALLY(client1, "220 ");
+		SWRITE(client1, "EHLO " TEST_EXTERNAL_DOMAIN ENDL);
+		CLIENT_EXPECT_EVENTUALLY(client1, "250 "); /* "250 " since there may be multiple "250-" responses preceding it */
+	} else {
+		SWRITE(client1, "RSET" ENDL);
+		CLIENT_EXPECT(client1, "250");
+	}
+
+	SWRITE(client1, "MAIL FROM:<" TEST_EMAIL_EXTERNAL ">\r\n");
+	CLIENT_EXPECT(client1, "250");
+	SWRITE(client1, "RCPT TO:<" TEST_EMAIL ">\r\n");
+	CLIENT_EXPECT(client1, "250");
+	SWRITE(client1, "DATA\r\n");
+	CLIENT_EXPECT(client1, "354");
+
+	/* Lengths include CR LF */
+	SWRITE(client1, "Content-Type: text/plain" ENDL); /* 26 bytes */
+	SWRITE(client1, "To: [email protected]" ENDL); /* 22 bytes */
+	SWRITE(client1, "From: [email protected]" ENDL); /* 24 bytes */
+	SWRITE(client1, "Subject: Test message" ENDL); /* 23 bytes */
+	SWRITE(client1, "Sun, 10 Nov 2024 19:58:17 -0500" ENDL); /* 33 bytes */
+	SWRITE(client1, ENDL); /* 2 bytes */
+	SWRITE(client1, "Test" ENDL); /* 6 bytes */
+
+	/* Messages end in CR LF, so only send . CR LF here */
+	SWRITE(client1, "." ENDL); /* EOM */
+	CLIENT_EXPECT(client1, "250");
+	return 0;
+
+cleanup:
+	return -1;
+}
+
 static int make_messages(void)
 {
 	int clientfd;
 	int res = 0;
 
+	send_count = 0;
 	clientfd = test_make_socket(25);
 	if (clientfd < 0) {
 		return -1;
@@ -138,6 +175,7 @@ static int make_messages(void)
 	res |= send_message(clientfd, "multipart.eml");
 	res |= send_message(clientfd, "multipart2.eml");
 	res |= send_message(clientfd, "alternative.eml");
+	res |= send_short_message(clientfd);
 
 	close(clientfd); /* Close SMTP connection */
 	return res;
@@ -344,6 +382,18 @@ static int run(void)
 	SWRITE(client1, "g13 FETCH 2 (BODY.PEEK[3.HEADER.FIELDS (Content-Type)]<1.12>)" ENDL);
 	CLIENT_EXPECT_EVENTUALLY(client1, "ontent-Type:");
 
+	/* Request RFC822.HEADER ([LBBS-85] bug fix) */
+	CLIENT_DRAIN(client1);
+	SWRITE(client1, "h1 FETCH 4 (UID RFC822.HEADER)" ENDL);
+	CLIENT_EXPECT(client1, "* 4 FETCH (UID 4 RFC822.HEADER {130}");
+
+	/* Ideally, we would be able to confirm that 130 bytes are actually received here...
+	 * Since we can't, repeat, and ensure it's the headers, not the body. */
+
+	CLIENT_DRAIN(client1);
+	SWRITE(client1, "h2 FETCH 4 (UID RFC822.HEADER)" ENDL);
+	CLIENT_EXPECT_EVENTUALLY(client1, "Content-Type");
+
 	SWRITE(client1, "z999 LOGOUT" ENDL);
 	CLIENT_EXPECT_EVENTUALLY(client1, "* BYE");
 	res = 0;