net_imap: Avoid sending multiple tagged responses on cross-server COP…

…Y/MOVE.

A regression was introduced in commit 5228300,
which passes along all untagged responses received while waiting for
the tagged response from a remote server. This is generally an
improvement, since we want to pass the untagged responses along, but
not the tagged response in cases where we generate our own tagged
response. Doing so could cause clients like libetpan to lose
synchronization since they would receive a tagged response from the
remote server followed by the untagged response that we generate.

For these cases, pass through all untagged responses received, but not
the final tagged response from the remote server. Additional checks have
also been added to trigger assertions if multiple tagged responses are
sent to the client, to help catch any recurring instances of this bug.

nets/net_imap.c

@@ -3012,7 +3012,7 @@ static int handle_remote_move(struct imap_session *imap, char *dest, const char
 						goto cleanup;
 					}
 					/* Could get an untagged EXISTS at this point */
-					if (imap_client_wait_response(destclient, -1, SEC_MS(5))) { /* tagged OK */
+					if (imap_client_wait_response_noechotag(destclient, -1, SEC_MS(5))) { /* wait for tagged OK, don't echo that, but echo everything else */
 						goto cleanup;
 					}
 				}
@@ -3040,8 +3040,10 @@ static int handle_remote_move(struct imap_session *imap, char *dest, const char
 			 * because any untagged data we receive prior to that should pass through directly to the client
 			 * (rather than being silently ignored/discarded as IMAP_CLIENT_EXPECT_EVENTUALLY does).
 			 * This way, the client can stay synchronized with the remote server, which is important
-			 * since APPEND can trigger untagged EXISTS's, etc. */
-			if (imap_client_wait_response(destclient, -1, SEC_MS(5))) {
+			 * since APPEND can trigger untagged EXISTS's, etc.
+			 *
+			 * At the same time, we don't want to relay the tagged response! */
+			if (imap_client_wait_response_noechotag(destclient, -1, SEC_MS(5))) {
 				goto cleanup;
 			}
 		}
@@ -4392,6 +4394,8 @@ static int imap_process(struct imap_session *imap, char *s)
 	char *command = NULL; /* XXX Should not need to be initialized, but gcc 12 complains if it's not */
 	int res = 0;
 
+	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. */

nets/net_imap/imap.h

@@ -89,12 +89,13 @@ struct imap_session {
 	unsigned int readonly:1;	/* SELECT vs EXAMINE */
 	unsigned int inauth:1;
 	unsigned int idle:1;		/* Whether IDLE is active */
-	unsigned int dnd:1;			/* Do Not Disturb: Whether client is executing a FETCH, STORE, or SEARCH command (EXPUNGE responses are not allowed) */
+	unsigned int dnd:1;			/* Do Not Disturb: Whether client is executing a FETCH, STORE, or SEARCH command (EXPUNGE responses are not allowed). XXX Unused? */
 	unsigned int pending:1;		/* Delayed output is pending in pfd pipe */
 	unsigned int expungepending:1;	/* EXPUNGE updates pending in pipe */
 	unsigned int alerted:2;		/* An alert has been delivered to this client */
 	unsigned int condstore:1;	/* Whether a client has issue a CONDSTORE enabling command, and should be sent MODSEQ updates in untagged FETCH responses */
 	unsigned int qresync:1;		/* Whether a client has enabled the QRESYNC capability */
+	unsigned int finalized_response:1;	/* Whether tagged response to this command has been sent already */
 	struct imap_notify *notify;	/* NOTIFY events */
 	bbs_mutex_t lock;		/* Lock for IMAP session */
 	RWLIST_ENTRY(imap_session) entry;	/* Next active session */
@@ -177,7 +178,11 @@ extern int imap_debug_level;
 #define _imap_reply(imap, fmt, ...) _imap_reply_nolock(imap, fmt, ## __VA_ARGS__)
 #define imap_send_nocrlf(imap, fmt, ...) _imap_reply_nolock_fd_lognewline(imap, imap->node->wfd, fmt, ## __VA_ARGS__);
 #define imap_send(imap, fmt, ...) _imap_reply(imap, "%s " fmt "\r\n", "*", ## __VA_ARGS__)
-#define imap_reply(imap, fmt, ...) _imap_reply(imap, "%s " fmt "\r\n", imap->tag, ## __VA_ARGS__)
+#define imap_reply(imap, fmt, ...) do { \
+	bbs_assert(!imap->finalized_response); \
+	imap->finalized_response = 1; \
+	_imap_reply(imap, "%s " fmt "\r\n", imap->tag, ## __VA_ARGS__); \
+} while (0);
 
 #define imap_parallel_send(imap, fmt, ...) _imap_parallel_reply(imap, "%s " fmt "\r\n", "*", ## __VA_ARGS__)
 

nets/net_imap/imap_client.c

@@ -482,6 +482,7 @@ static ssize_t client_command_passthru(struct imap_client *client, int fd, const
 
 	for (;;) {
 		int cbres = 0;
+		int is_tagged_response, do_echo;
 		char *buf = client->buf;
 		if (fd != -1) {
 			res = bbs_multi_poll(pfds, 2, ms); /* If returns 1, client->rfd had activity, if 2, it was fd */
@@ -517,7 +518,12 @@ static ssize_t client_command_passthru(struct imap_client *client, int fd, const
 				break;
 			}
 		}
-		if (echo) {
+		/* If echo is 0, don't echo (relay back to local client)
+		 * If 1, unconditionally echo.
+		 * If 2, echo as long as it is not the tagged response. */
+		is_tagged_response = !strncmp(buf, tag, (size_t) taglen);
+		do_echo = echo == 1 || (echo == 2 && !is_tagged_response);
+		if (do_echo) {
 			/* Go ahead and relay it */
 			if (res > 0) { /* If it was just an empty line, don't bother calling write() with 0 bytes */
 				bbs_write(imap->node->wfd, buf, (unsigned int) res);
@@ -527,16 +533,22 @@ static ssize_t client_command_passthru(struct imap_client *client, int fd, const
 				break;
 			}
 		}
+		/* Allow easily differentiating replies that we silently discard and those passed through
+		 * <= if passed through, <- if not */
 #ifdef DEBUG_REMOTE_RESPONSES
 		/* NEVER enable this in production because this will be a huge volume of data */
-		imap_debug(10, "<= %.*s\n", (int) res, buf);
+		imap_debug(10, "<%c %.*s\n", do_echo ? '=' : '-', (int) res, buf);
 #else
 		if (c++ < 15 && res > 2 && !strncmp(buf, "* ", STRLEN("* "))) {
-			imap_debug(7, "<= %.*s\n", (int) res, buf);
+			imap_debug(7, "<%c %.*s\n", do_echo ? '=' : '-', (int) res, buf);
 		}
 #endif
-		if (!strncmp(buf, tag, (size_t) taglen)) {
-			imap_debug(10, "<= %.*s\n", (int) res, buf);
+		if (is_tagged_response) {
+			if (do_echo) {
+				/* We just relayed the tagged response from the remote server to local client */
+				imap->finalized_response = 1;
+			}
+			imap_debug(10, "<%c %.*s\n", do_echo ? '=' : '-', (int) res, buf);
 			if (!STARTS_WITH(buf + taglen, "OK")) { /* BAD or NO */
 				/* We did something we shouldn't have, oops */
 				if (cmd) {

nets/net_imap/imap_client.h

@@ -93,6 +93,8 @@ ssize_t __attribute__ ((format (gnu_printf, 3, 4))) __imap_client_send_log(struc
 #define imap_client_send_log(client, fmt, ...) __imap_client_send_log(client, 1, fmt, ## __VA_ARGS__)
 
 #define imap_client_wait_response(client, fd, ms) __imap_client_wait_response(client, fd, ms, 1, __LINE__, NULL, NULL)
+#define imap_client_wait_response_noecho(client, fd, ms) __imap_client_wait_response(client, fd, ms, 0, __LINE__, NULL, NULL)
+#define imap_client_wait_response_noechotag(client, fd, ms) __imap_client_wait_response(client, fd, ms, 2, __LINE__, NULL, NULL)
 
 #define imap_client_send_wait_response(client, fd, ms, fmt, ...) __imap_client_send_wait_response(client, fd, ms, 1, __LINE__, NULL, NULL, fmt, ## __VA_ARGS__)
 #define imap_client_send_wait_response_cb(client, fd, ms, cb, cbdata, fmt, ...) __imap_client_send_wait_response(client, fd, ms, 1, __LINE__, cb, cbdata, fmt, ## __VA_ARGS__)
@@ -107,7 +109,7 @@ int __imap_client_wait_response(struct imap_client *client, int fd, int ms, int
  * \param client
  * \param fd Additional file descriptor on which to poll(used only for IDLE, -1 otherwise)
  * \param ms Max time to wait for poll / bbs_readline
- * \param echo Whether to relay output from remote server to our local client
+ * \param echo 1 to relay output from remote server to our local client, 2 to relay output except tagged repsonse, 0 to not relay any output
  * \param lineno
  * \param cb Custom callback to run for each line received from remote server. Returning -1 from callback will terminate wait loop
  * \param cbdata Callback data for callback function