net_imap: Improve robustness of remote IMAP client recreation.

Proxied IMAP clients are not always recreated and restored to the same
state that they were in originally. This manifests itself as receiving
"BAD Command received in Invalid state" or potentially "Must select
a mailbox first" when attempting to FETCH a message on a remote IMAP
server where the currently selected mailbox has been idle for some time.

To fix this:
* Restore the selected folder of the foreground client, so whatever
  mailbox was selected originally is reselected when recreated.
  The SELECT happens in the background, transparently.
* Fix detection of idling client when attempting to send command.
  This should never be okay so always trigger a soft assertion.

Author: InterLinked1

nets/net_imap.c

@@ -410,9 +410,8 @@ static int generate_mailbox_name(struct imap_session *imap, const char *restrict
 		fulldir++;
 	}
 
-	if (strlen_zero(fulldir)) {
+	if (bbs_assertion_failed(!strlen_zero(fulldir))) {
 		bbs_error("Empty maildir? (%s)\n", maildir);
-		bbs_soft_assert(0);
 		return -1;
 	}
 
@@ -1434,7 +1433,7 @@ static int handle_select(struct imap_session *imap, char *s, enum select_type re
 	}
 	if (imap->client) {
 		int modseq = 0;
-		char *remotename = remote_mailbox_name(imap->client, mailbox);
+		const char *remotename = remote_mailbox_name(imap->client, mailbox);
 		REPLACE(imap->folder, mailbox);
 		/* Reconstruct the SELECT, fortunately this is not too bad */
 		return imap_client_send_wait_response_cb(imap->client, -1, 5000, remote_select_cb, &modseq, "%s \"%s\"\r\n", readonly == CMD_SELECT ? "SELECT" : "EXAMINE", remotename);
@@ -1530,7 +1529,7 @@ static int handle_status(struct imap_session *imap, char *s)
 		return 0;
 	}
 	if (traversal.client) {
-		char *remotename = remote_mailbox_name(traversal.client, mailbox);
+		const char *remotename = remote_mailbox_name(traversal.client, mailbox);
 		int wantsize = strstr(s, "SIZE") ? 1 : 0;
 		/* If the client wants SIZE but the remote server doesn't support it, we'll have to fake it by translating */
 		if (wantsize && !(traversal.client->virtcapabilities & IMAP_CAPABILITY_STATUS_SIZE)) {
@@ -4007,7 +4006,15 @@ static int test_remote_mailbox_substitution(void)
 static int idle_stop(struct imap_session *imap)
 {
 	imap->idle = 0;
-	/* IDLE for virtual mailboxes (proxied) is handled in the IDLE command itself */
+	/* IDLE for virtual mailboxes (proxied) is handled in the IDLE command itself,
+	 * so it shouldn't be idling at this point. */
+	if (bbs_assertion_failed(!imap->client || !imap->client->idling)) {
+		/* We're not supposed to be idling, but if we are somehow, stop,
+		 * or successive commands will fail. */
+		if (imap_client_idle_stop(imap->client)) {
+			return -1;
+		}
+	}
 	_imap_reply(imap, "%s OK IDLE terminated\r\n", imap->savedtag); /* Use tag from IDLE request */
 	free_if(imap->savedtag);
 	return 0;
@@ -4207,6 +4214,9 @@ static int handle_idle(struct imap_session *imap)
 
 			if (!client) {
 				if (imap->client) {
+					/* The client wants to do something with the foreground remote client connection.
+					 * We probably just received a "DONE" from the client,
+					 * so go ahead and terminate idling before we return. */
 					imap_client_integrity_check(imap, imap->client);
 					/* Stop idling on the selected mailbox (remote) */
 					if (imap_client_idle_stop(imap->client)) {
@@ -4231,10 +4241,15 @@ static int handle_idle(struct imap_session *imap)
 						 * This here is the case where our local user did something,
 						 * the case of getting activity on a particular client,
 						 * and handling its disconnect in realtime, is handled below. */
+
+						/* Because we were trying to stop IDLE, ensure the client isn't recreated to idling state, since we want to use it.
+						 * imap_recreate_client will enable IDLE if it was previously set, so manually unset it first. */
+						imap->client->idling = 0;
 						imap_recreate_client(imap, imap->client);
+						imap_clients_renew_idle(imap, imap->client); /* No need to renew IDLE on the foreground client, we just recreated it */
 					}
 				}
-				imap_clients_renew_idle(imap); /* In case some of them are close to expiring, renew them now before returning */
+				imap_clients_renew_idle(imap, NULL); /* In case some of them are close to expiring, renew them now before returning */
 				break; /* Client terminated the idle. Stop idling and return to read the next command. */
 			}
 			/* For remote NOTIFY: Some remote IMAP server sent us something.
@@ -4322,7 +4337,7 @@ static int handle_idle(struct imap_session *imap)
 					}
 				}
 			}
-			imap_clients_renew_idle(imap);
+			imap_clients_renew_idle(imap, NULL);
 		} else {
 			/* Nothing yet. Send an "IDLE ping" to check in... */
 			idleleft -= pollms;
@@ -4343,7 +4358,7 @@ static int handle_idle(struct imap_session *imap)
 					imap_send(imap, "OK Still here");
 					lastactivity = now;
 				}
-				imap_clients_renew_idle(imap);
+				imap_clients_renew_idle(imap, NULL);
 			}
 		}
 	}
@@ -4397,8 +4412,9 @@ static int imap_process(struct imap_session *imap, char *s)
 	imap->finalized_response = 0; /* New command, reset */
 
 	if (imap->idle || (imap->alerted == 1 && !strcasecmp(s, "DONE"))) {
-		/* Thunderbird clients will still send "DONE" if we send a tagged reply during the IDLE,
-		 * but Microsoft Outlook will not, so handle both cases, i.e. tolerate the redundant DONE. */
+		/* Mozilla clients will still send "DONE" if we send a tagged reply during the IDLE,
+		 * but Microsoft Outlook will not, so handle both cases, i.e. tolerate the redundant DONE (2nd condition). */
+		bbs_debug(5, "Received %sIDLE termination\n", imap->idle ? "" : "redundant ");
 		return idle_stop(imap);
 	} else if (imap->alerted == 1) {
 		imap->alerted = 2;

nets/net_imap/imap_client.c

@@ -31,6 +31,9 @@
 #include "nets/net_imap/imap.h"
 #include "nets/net_imap/imap_client.h"
 
+/* Per the RFC, don't idle for more than 30 minutes at a time */
+#define MAX_IDLE_SECS 1800
+
 extern unsigned int maxuserproxies;
 
 /* Check client pointers for integrity before/after various operations.
@@ -204,6 +207,11 @@ int imap_poll(struct imap_session *imap, int ms, struct imap_client **clientout)
 
 int imap_client_idle_start(struct imap_client *client)
 {
+	bbs_debug(5, "Starting IDLE for client %s\n", client->name);
+	if (bbs_assertion_failed(!client->idling)) {
+		bbs_warning("Client %s already idling\n", client->name);
+		return -1;
+	}
 	/* Now, IDLE on it so we get updates for that mailbox */
 	if (SWRITE(client->client.wfd, "idle IDLE\r\n") < 0) {
 		return -1;
@@ -220,6 +228,11 @@ int imap_client_idle_start(struct imap_client *client)
 
 int imap_client_idle_stop(struct imap_client *client)
 {
+	bbs_debug(5, "Stopping IDLE for client %s\n", client->name);
+	if (bbs_assertion_failed(client->idling)) {
+		bbs_warning("Client %s not currently idling\n", client->name);
+		return -1;
+	}
 	if (SWRITE(client->client.wfd, "DONE\r\n") < 0) {
 		bbs_error("Failed to write to idling client '%s', must be dead\n", client->name);
 		return -1;
@@ -283,6 +296,8 @@ static struct imap_client *create_client_after_purge(struct imap_session *imap,
 		 * in which case, this will fail. In that case, just let it go. */
 		if (exists) {
 			bbs_warning("Failed to recreate %s client %s\n", foreground ? "foreground" : "background", name);
+		} else {
+			bbs_debug(3, "Client '%s' does not exist any more in configuration\n", name);
 		}
 		return NULL;
 	}
@@ -299,24 +314,47 @@ int imap_recreate_client(struct imap_session *imap, struct imap_client *client)
 	int was_idling = client->idling;
 	int foreground = imap->client == client;
 	char name[256];
+	char mb_name[256];
 
 	safe_strncpy(name, client->name, sizeof(name)); /* Store name before destroying the client */
 
+	/* If a particular mailbox is selected currently, we need to re-SELECT it after recreation */
+	if (foreground) {
+		const char *remotename = remote_mailbox_name(imap->client, imap->folder);
+		safe_strncpy(mb_name, remotename, sizeof(remotename));
+	}
+
 	/* Destroy old client before creating it again. */
 	if (foreground) {
 		imap->client = NULL;
 	}
 	imap_client_unlink(imap, client); /* returns void, so can't check success */
+	client = NULL;
 
 	/* Now, create it again. */
-	newclient = create_client_after_purge(imap, name, foreground, was_idling);
-	if (newclient && foreground) {
+	newclient = create_client_after_purge(imap, name, foreground, 0); /* Don't resume idling just yet, if we need to SELECT. Do so at the end. */
+	if (!newclient) {
+		bbs_warning("Failed to recreate client %s\n", name);
+		return -1;
+	}
+	if (foreground) {
+		/* Reselect the mailbox that was created.
+		 * Since the client is being recreated transparently, the actual local client may go ahead
+		 * and perform a mailbox operation (e.g. FETCH), so we need to make sure the server is in the
+		 * same state after the recreation as it was before. */
+		bbs_debug(5, "Reselecting previously selected mailbox '%s'\n", mb_name);
+		/* Do not pass anything through from the remote server, since this is supposed to completely transparent.
+		 * None of the untagged SELECT responses should pass through, prior to the tagged IDLE response. */
+		imap_client_send_wait_response_noecho(newclient, -1, 5000, "%s \"%s\"\r\n", "SELECT", mb_name);
 		imap->client = newclient; /* At this point, the new client has been swapped in, and the rest of the module is none the wiser. */
 	}
-	return newclient ? 0 : -1;
+	if (was_idling) {
+		imap_client_idle_start(newclient); /* Resume idling, if that's what it was doing before */
+	}
+	return 0;
 }
 
-void imap_clients_renew_idle(struct imap_session *imap)
+void imap_clients_renew_idle(struct imap_session *imap, struct imap_client *except)
 {
 	struct stringlist deadclients;
 	char *clientname;
@@ -330,7 +368,7 @@ void imap_clients_renew_idle(struct imap_session *imap)
 	RWLIST_WRLOCK(&imap->clients);
 	RWLIST_TRAVERSE_SAFE_BEGIN(&imap->clients, client, entry) {
 		time_t maxage;
-		if (!client->idling || client->dead) {
+		if (!client->idling || client->dead || client == except) {
 			continue;
 		}
 		/* This is when the connection may be terminated.
@@ -348,7 +386,10 @@ void imap_clients_renew_idle(struct imap_session *imap)
 					/* The list is locked at the moment,
 					 * and creating a client entails acquiring that lock,
 					 * so for now, just keep track of what clients we removed,
-					 * and recreate them afterwards. */
+					 * and recreate them afterwards.
+					 *
+					 * The selected mailbox only matters for the foreground client,
+					 * so we don't need to care about it for these. */
 					stringlist_push_tail(&deadclients, client->name);
 					client_destroy(client);
 				}
@@ -582,9 +623,8 @@ ssize_t __imap_client_send_log(struct imap_client *client, int log, const char *
 		return -1;
 	}
 
-	if (client->idling) {
+	if (bbs_assertion_failed(!client->idling)) { /* Could stop idle now if this were to happen, but that would just mask a bug */
 		bbs_warning("Client is currently idling while attempting to write '%.*s'", len, buf);
-		bbs_soft_assert(0); /* Could stop idle now if this were to happen, but that would just mask a bug */
 	}
 
 	if (log) {
@@ -600,12 +640,10 @@ int __imap_client_wait_response(struct imap_client *client, int fd, int ms, int
 	int taglen;
 	const char *tag = "tag";
 
-	if (!client->imap) {
+	if (bbs_assertion_failed(client->imap != NULL)) {
 		bbs_warning("No active IMAP client?\n"); /* Shouldn't happen... */
-		bbs_soft_assert(0);
-	} else if (strlen_zero(client->imap->tag)) {
+	} else if (bbs_assertion_failed(!strlen_zero(client->imap->tag))) {
 		bbs_warning("No active IMAP tag, using generic one\n");
-		bbs_soft_assert(0);
 	} else {
 		tag = client->imap->tag;
 	}
@@ -627,12 +665,10 @@ int __imap_client_send_wait_response(struct imap_client *client, int fd, int ms,
 	va_list ap;
 	const char *tag = "tag";
 
-	if (!client->imap) {
+	if (bbs_assertion_failed(client->imap != NULL)) {
 		bbs_warning("No active IMAP client?\n"); /* Shouldn't happen... */
-		bbs_soft_assert(0);
-	} else if (strlen_zero(client->imap->tag)) {
+	} else if (bbs_assertion_failed(!strlen_zero(client->imap->tag))) {
 		bbs_warning("No active IMAP tag, using generic one\n");
-		bbs_soft_assert(0);
 	} else {
 		tag = client->imap->tag;
 	}
@@ -652,17 +688,26 @@ int __imap_client_send_wait_response(struct imap_client *client, int fd, int ms,
 
 #if 0
 	/* Somewhat redundant since there's another debug right after */
-	bbs_debug(6, "Passing through command %s (line %d) to remotely mapped '%s'\n", tag, lineno, client->virtprefix);
+	bbs_debug(8, "Passing through command %s (line %d) to remotely mapped '%s'\n", tag, lineno, client->virtprefix);
 #else
 	UNUSED(lineno);
 #endif
 
-	/* Only include this check if it's not the active client,
-	 * since a lot of pass through command code uses imap_client_send,
-	 * in which case this would be a false positive otherwise. */
-	if (client != client->imap->client && client->idling) {
-		bbs_warning("Client is currently idling while attempting to write '%s%s'", tagbuf, buf);
-		bbs_soft_assert(0); /* Could stop idle now if this were to happen, but that would just mask a bug */
+	if (bbs_assertion_failed(!client->idling)) {
+		/* We shouldn't be idling. If we are, it means we're trying to send a command while IDLE,
+		 * which will fail since the only valid input to the server is "DONE".
+		 * For background clients, it's very clear it shouldn't, as it means we failed to call imap_client_idle_stop somewhere.
+		 * Even for foreground clients, it shouldn't happen. Although we do passthrough using imap_client_send to some extent,
+		 * imap_client_idle_start and imap_client_idle_stop are still called on paths where it's the foreground client,
+		 * instead of directly proxying it. The only time this should be true is after imap_poll returns. */
+		bbs_warning("%s client %s was idling while attempting to write '%s%s'",
+			client == client->imap->client ? "Foreground" : "Background", client->virtprefix, tagbuf, buf);
+		/* Could simply stop idle now if this were to happen, but that would just mask a bug,
+		 * hence the assertion, but afterwards, try to rectify and fix so we can proceed. */
+		if (imap_client_idle_stop(client)) {
+			return -1;
+		}
+		/* Okay, now the client is no longer idling for sure */
 	}
 
 	if (bbs_write(client->client.wfd, tagbuf, (unsigned int) taglen) < 0) {
@@ -924,7 +969,7 @@ struct imap_client *__imap_client_get_by_url(struct imap_session *imap, const ch
 		 * to keep these connections alive... */
 		client->maxidlesec = 65; /* ~1 minute */
 	} else {
-		client->maxidlesec = 1800; /* 30 minutes */
+		client->maxidlesec = MAX_IDLE_SECS; /* 30 minutes */
 	}
 
 	client->lastactive = time(NULL); /* Mark as active since we just successfully did I/O with it */
@@ -1118,7 +1163,7 @@ int mailbox_remotely_mapped(struct imap_session *imap, const char *path)
 	return exists;
 }
 
-char *remote_mailbox_name(struct imap_client *client, char *restrict mailbox)
+const char *remote_mailbox_name(struct imap_client *client, char *restrict mailbox)
 {
 	char *tmp, *remotename = mailbox + client->virtprefixlen + 1;
 	/* This is some other server's problem to handle.

nets/net_imap/imap_client.h

@@ -65,8 +65,12 @@ int imap_clients_next_idle_expiration(struct imap_session *imap);
  */
 int imap_recreate_client(struct imap_session *imap, struct imap_client *client);
 
-/*! \brief Renew IDLE on all idling clients that are close to expiring */
-void imap_clients_renew_idle(struct imap_session *imap);
+/*!
+ * \brief Renew IDLE on all idling clients that are close to expiring
+ * \param imap
+ * \param except Renew IDLE on all clients except this one, if non-NULL
+ */
+void imap_clients_renew_idle(struct imap_session *imap, struct imap_client *except);
 
 void imap_client_idle_notify(struct imap_client *client);
 
@@ -148,4 +152,4 @@ struct imap_client *load_virtual_mailbox(struct imap_session *imap, const char *
 int mailbox_remotely_mapped(struct imap_session *imap, const char *path);
 
 /*! \brief Convert a local mailbox name to its name on a remote server */
-char *remote_mailbox_name(struct imap_client *client, char *restrict mailbox);
+const char *remote_mailbox_name(struct imap_client *client, char *restrict mailbox);

nets/net_imap/imap_client_status.c

@@ -212,7 +212,7 @@ static int status_size_fetch_incremental(struct imap_client *client, const char
 		return -1;
 	}
 	if (oldv != newv) {
-		bbs_verb(4, "Remote UIDVALIDITY changed from %d to %d\n", oldv, newv);
+		bbs_verb(4, "Remote UIDVALIDITY (%s on %s) changed from %d to %d\n", remotename, client->name, oldv, newv);
 		return -1;
 	}
 	if (parse_status_item(old, "MESSAGES", &oldmessages) || parse_status_item(new, "MESSAGES", &newmessages)) {