From 5228300db18ca31588436d89c5d15795354856b7 Mon Sep 17 00:00:00 2001 From: InterLinked1 <24227567+InterLinked1@users.noreply.github.com> Date: Sun, 1 Dec 2024 21:14:43 -0500 Subject: [PATCH] net_imap: Properly wait for tagged response for proxied APPEND/MOVE. Previously, we were not properly handling untagged responses while waiting for the tagged response. Initially, not receiving the tagged response immediately would cause failure, since we were only checking if the first response read was expected. The remote MOVE/COPY handler was updated previously to tolerate untagged responses while waiting for the tagged response, but these were not passed along to the client, as they should be. A similar issue existed with APPENDs to a remote mailbox. Now, in both cases, we properly wait for the tagged response, while passing along everything received in the meantime to the client. --- nets/net_imap.c | 28 ++++++++++++++-------------- nets/net_imap/imap_client.c | 33 +++++++++++++++++++++++++++++++-- nets/net_imap/imap_client.h | 16 ++++++++++++++++ 3 files changed, 61 insertions(+), 16 deletions(-) diff --git a/nets/net_imap.c b/nets/net_imap.c index 8b210bd..cf80c12 100644 --- a/nets/net_imap.c +++ b/nets/net_imap.c @@ -2622,15 +2622,12 @@ static int handle_append(struct imap_session *imap, char *s) if (res < 0) { return -1; } - /* Relay response from remote, and no need to clean up */ - res = bbs_readline(imap->client->client.rfd, &imap->client->client.rldata, "\r\n", SEC_MS(5)); - if (res < 0) { - /* No need to clean up, since for remote server, - * we're not appending seqnos/UIDs to a uintlist */ - bbs_warning("APPEND tagged response not received from remote server?\n"); + /* Relay response from remote, and no need to clean up. + * We may (should) receive an untagged EXISTS for the uploaded message, + * so use imap_client_wait_response to wait for the actual tagged response. */ + if (imap_client_wait_response(imap->client, -1, SEC_MS(5))) { return -1; } - _imap_reply(imap, "%s\r\n", imap->client->client.buf); return 0; } @@ -3035,13 +3032,16 @@ static int handle_remote_move(struct imap_session *imap, char *dest, const char if (SWRITE(destclient->client.wfd, "\r\n") != STRLEN("\r\n")) { goto cleanup; } - /* Well, hopefully that all worked! - * Use IMAP_CLIENT_EXPECT_EVENTUALLY since we may also get untagged EXISTS's for the copied messages. */ - - /*! \todo BUGBUG For any IMAP_CLIENT_EXPECT_EVENTUALLY, all the untagged responses that we "ignore - * should actually pass through to the client directly, since an offline client will need those - * in order to be synchronized. */ - IMAP_CLIENT_EXPECT_EVENTUALLY(&destclient->client, appended + 1, " OK"); /* tagged OK */ + /* Well, hopefully that all worked! Now, wait for confirmation of a successful upload. + * + * Here, we use imap_client_wait_response instead of IMAP_CLIENT_EXPECT_EVENTUALLY to wait for the tagged OK, + * 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))) { + goto cleanup; + } } if (move) { /* Now that we copied everything to the destination mailbox, delete the source */ diff --git a/nets/net_imap/imap_client.c b/nets/net_imap/imap_client.c index 0c5f35c..aa324e9 100644 --- a/nets/net_imap/imap_client.c +++ b/nets/net_imap/imap_client.c @@ -440,6 +440,7 @@ static ssize_t client_command_passthru(struct imap_client *client, int fd, const } res = bbs_readline(tcpclient->rfd, &tcpclient->rldata, "\r\n", ms); if (res < 0) { /* Could include remote server disconnect */ + bbs_warning("Failed to receive expected response from remote server\n"); return res; } if (cb) { @@ -469,9 +470,13 @@ static ssize_t client_command_passthru(struct imap_client *client, int fd, const #endif if (!strncmp(buf, tag, (size_t) taglen)) { imap_debug(10, "<= %.*s\n", (int) res, buf); - if (STARTS_WITH(buf + taglen, "BAD")) { + if (!STARTS_WITH(buf + taglen, "OK")) { /* BAD or NO */ /* We did something we shouldn't have, oops */ - bbs_warning("Command '%.*s%.*s' failed: %s\n", taglen, tag, cmdlen > 2 ? cmdlen - 2 : cmdlen, cmd, buf); /* Don't include trailing CR LF */ + if (cmd) { + bbs_warning("Command '%.*s%.*s' failed: %s\n", taglen, tag, cmdlen > 2 ? cmdlen - 2 : cmdlen, cmd, buf); /* Don't include trailing CR LF */ + } else { + bbs_warning("Received negatory response: %s\n", buf); + } } client->lastactive = time(NULL); /* Successfully just got data from remote server */ break; /* That's all, folks! */ @@ -510,6 +515,30 @@ ssize_t __imap_client_send_log(struct imap_client *client, int log, const char * return bbs_write(client->client.wfd, buf, (size_t) len); } +int __imap_client_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) +{ + char tagbuf[15]; + int taglen; + const char *tag = "tag"; + + if (!client->imap) { + bbs_warning("No active IMAP client?\n"); /* Shouldn't happen... */ + bbs_soft_assert(0); + } else if (strlen_zero(client->imap->tag)) { + bbs_warning("No active IMAP tag, using generic one\n"); + bbs_soft_assert(0); + } else { + tag = client->imap->tag; + } + + taglen = snprintf(tagbuf, sizeof(tagbuf), "%s ", tag); /* Reuse the tag the client sent us, so we can just passthrough the response */ + + UNUSED(lineno); + + /* Read until we get the tagged respones */ + return client_command_passthru(client, fd, tagbuf, taglen, NULL, 0, ms, echo, cb, cbdata) <= 0; +} + 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; diff --git a/nets/net_imap/imap_client.h b/nets/net_imap/imap_client.h index 37f50a5..0072ade 100644 --- a/nets/net_imap/imap_client.h +++ b/nets/net_imap/imap_client.h @@ -86,11 +86,27 @@ ssize_t __attribute__ ((format (gnu_printf, 3, 4))) __imap_client_send_log(struc #define imap_client_send(client, fmt, ...) __imap_client_send_log(client, 0, fmt, ## __VA_ARGS__) #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_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__) #define imap_client_send_wait_response_noecho(client, fd, ms, fmt, ...) __imap_client_send_wait_response(client, fd, ms, 0, __LINE__, NULL, NULL, fmt, ## __VA_ARGS__) #define imap_client_send_wait_response_cb_noecho(client, fd, ms, cb, cbdata, fmt, ...) __imap_client_send_wait_response(client, fd, ms, 0, __LINE__, cb, cbdata, fmt, ## __VA_ARGS__) +/*! \brief Same as __imap_client_send_wait_response, but don't send a command initially, just wait for the tagged response */ +int __imap_client_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); + +/*! + * \brief Send a command to remote IMAP server and wait for a response, passing through any receive untagged responses inbetween directly back to the client + * \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 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 + * \param fmt printf-style fmt string + */ int __attribute__ ((format (gnu_printf, 8, 9))) __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, ...); /*! \retval Number of replacements made */