Skip to content

Commit

Permalink
net_imap: Fix missing untagged notifications on MOVE/COPY/STORE.
Browse files Browse the repository at this point in the history 
* Fix construction of Other Users mailbox path with extra erroneous
  leading hierarchy delimiter.
* Fix not getting notifications of flag changes with NOTIFY enabled,
  caused by wrong maildir path being passed to send_untagged_fetch.
* Fix missing EXPUNGE notifications on cross-server MOVE from local
  server to remote server.
* Fix missing EXISTS notifications on COPY operations.
* Fix missing EXISTS notifications on MOVE operations.
* Don't send untagged updates after tagged response.
* Fix logic for UIDVALIDITY rotation.
* mod_webmail: Don't automark seen when requesting raw message,
  to prevent message downloads from automarking seen.
  • Loading branch information
@InterLinked1
InterLinked1 committed Dec 1, 2024
1 parent 74ee034 commit b41845b
Show file tree
Hide file tree
Showing 12 changed files with 311 additions and 29 deletions.
49 changes: 45 additions & 4 deletions modules/mod_mail.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -987,7 +987,7 @@ static int parse_uidfile(FILE *fp, const char *uidfile, unsigned int *uidvalidit
char *uidvaliditystr, *tmp;

if (fread(uidvalidity, sizeof(unsigned int), 1, fp) != 1) {
bbs_debug(4, "Failed to read UIDVALIDITY from %s (empty file?)\n", uidfile); /* If we just created the file, it's empty, so this could happen */
bbs_debug(2, "Failed to read UIDVALIDITY from %s: %s (empty file?)\n", uidfile, strerror(errno)); /* If we just created the file, it's empty, so this could happen */
return 1;
} else if (fread(uidnext, sizeof(unsigned int), 1, fp) != 1) {
bbs_error("Failed to read UID from %s\n", uidfile);
Expand Down Expand Up @@ -1030,7 +1030,7 @@ unsigned int mailbox_get_next_uid(struct mailbox *mbox, struct bbs_node *node, c
{
FILE *fp = NULL;
char uidfile[256];
int uidvchange = 0;
int uidvchange = 0, uidfile_existed = 1;
unsigned int uidvalidity = 0, uidnext = 0;
int ascii = 0;

Expand Down Expand Up @@ -1077,6 +1077,7 @@ unsigned int mailbox_get_next_uid(struct mailbox *mbox, struct bbs_node *node, c
fp = fopen(uidfile, "r+"); /* Open for reading and writing, don't truncate it, and create if it doesn't exist */
if (!fp) {
/* Assume the file doesn't yet exist. */
uidfile_existed = bbs_file_exists(uidfile);
fp = fopen(uidfile, "a");
if (unlikely(!fp)) {
bbs_error("fopen(%s) failed: %s\n", uidfile, strerror(errno));
Expand Down Expand Up @@ -1104,7 +1105,22 @@ unsigned int mailbox_get_next_uid(struct mailbox *mbox, struct bbs_node *node, c
parse_uidfile(fp, uidfile, &uidvalidity, &uidnext, &ascii);
}

if (!uidvalidity || !uidnext || !fp) {
if (!uidvalidity || !fp) { /* uidnext can be 0 here */
struct stat st;
/* If there's no uidvalidity currently, not a big deal, that's expected and we'll create one now.
* If there is one and we failed, then this is really, really bad. */
if (uidfile_existed) {
bbs_error("Couldn't read already existing uidvalidity file (%s)\n", uidfile);
}
if (!stat(uidfile, &st)) {
if (st.st_size > 0) {
bbs_error("Failed to read current UID file %s\n", uidfile);
} else if (uidfile_existed) {
bbs_debug(3, "File %s is currently empty\n", uidfile);
}
} else {
bbs_debug(3, "Failed to stat(%s): %s\n", uidfile, strerror(errno));
}
/* UIDVALIDITY must be strictly increasing, so time is a good thing to use. */
uidvalidity = (unsigned int) time(NULL); /* If this isn't the first access to this folder, this will invalidate the client's cache of this entire folder. */
/* Since we're starting over, we must broadcast the new UIDVALIDITY value (we always do for SELECTs). */
Expand Down Expand Up @@ -1177,9 +1193,10 @@ unsigned int mailbox_get_next_uid(struct mailbox *mbox, struct bbs_node *node, c
*newuidnext = uidnext;
mailbox_uid_unlock(mbox);

if (uidvchange) {
if (uidvchange && uidfile_existed) {
/* Don't do this while mailbox UID lock is held, or we could cause a deadlock
* if an event callback is triggered that tries to grab that lock. */
bbs_debug(2, "UIDVALIDITY has changed (UIDVALIDITY=%d,UIDNEXT=%d)\n", uidvalidity, uidnext);
mailbox_dispatch_event_basic(EVENT_MAILBOX_UIDVALIDITY_CHANGE, node, mbox, directory);
}

Expand Down Expand Up @@ -1626,6 +1643,28 @@ static int gen_newname(struct mailbox *mbox, struct bbs_node *node, const char *
return (int) uid;
}

static int copy_move_untagged_exists(struct bbs_node *node, struct mailbox *mbox, const char *newpath, size_t size)
{
char maildir[256];
char *tmp;

safe_strncpy(maildir, newpath, sizeof(maildir));

tmp = strrchr(maildir, '/'); /* newpath can be mutiliated at this point */
if (!tmp) {
return -1;
}
*tmp = '\0'; /* Truncating once gets rid of the filename, need to do it again to get the maildir without /new or /cur at the end */
/* Since we're already near the end of the string, it's more efficient to back up from here than start from the beginning a second time */
while (tmp > maildir && *tmp != '/') {
tmp--;
}
*tmp = '\0';

mailbox_notify_new_message(node, mbox, maildir, newpath, size);
return 0;
}

int maildir_move_msg(struct mailbox *mbox, struct bbs_node *node, const char *curfile, const char *curfilename, const char *destmaildir, unsigned int *uidvalidity, unsigned int *uidnext)
{
return maildir_move_msg_filename(mbox, node, curfile, curfilename, destmaildir, uidvalidity, uidnext, NULL, 0);
Expand All @@ -1645,6 +1684,7 @@ int maildir_move_msg_filename(struct mailbox *mbox, struct bbs_node *node, const
return -1;
}
bbs_debug(6, "Renamed %s -> %s\n", curfile, newpath);
copy_move_untagged_exists(node, mbox, newpath, len);
if (newfile) {
safe_strncpy(newfile, newpath, len);
}
Expand Down Expand Up @@ -1699,6 +1739,7 @@ int maildir_copy_msg_filename(struct mailbox *mbox, struct bbs_node *node, const
}
/* Rather than invalidating quota usage for no reason, just update it so it stays in sync */
mailbox_quota_adjust_usage(mbox, copied);
copy_move_untagged_exists(node, mbox, newpath, len);
return (int) uid;
}

Expand Down
38 changes: 36 additions & 2 deletions modules/mod_webmail.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2913,11 +2913,26 @@ static int handle_fetch(struct ws_session *ws, struct imap_client *client, uint3
fetch_att = mailimap_fetch_att_new_internaldate();
mailimap_fetch_type_new_fetch_att_list_add(fetch_type, fetch_att);
section = mailimap_section_new(NULL);
if (raw) {
/* The client requests the raw message in two circumstances:
* 1. The user actually wants to view the raw message source.
* 2. The user downloads the message.
* For #2, we should NOT necessarily mark the message seen,
* because the user may not have really "viewed" the messages,
* i.e. user should be able to download without marking seen.
* For #1, the user has likely already clicked on the message
* to view it BEFORE toggling to raw. Not necessarily, but
* pretty likely.
* So, if requesting raw body, don't automark seen,
* even if we would normally. */
fetch_att = mailimap_fetch_att_new_body_peek_section(section);
} else {
#ifdef AUTO_MARK_SEEN
fetch_att = mailimap_fetch_att_new_body_section(section);
fetch_att = mailimap_fetch_att_new_body_section(section);
#else
fetch_att = mailimap_fetch_att_new_body_peek_section(section);
fetch_att = mailimap_fetch_att_new_body_peek_section(section);
#endif
}
mailimap_fetch_type_new_fetch_att_list_add(fetch_type, fetch_att);

/* Fetch by UID */
Expand Down Expand Up @@ -3409,6 +3424,10 @@ static int process_idle(struct imap_client *client, char *s)
} else if (STARTS_WITH(tmp, "RECENT")) {
/* RECENT is basically always accompanied by EXISTS, so this is almost academic,
* since we don't currently use this flag for anything, but for sake of completeness: */
/*! \todo We should use this, because not all EXISTS are accompanied by EXISTS.
* For example, if a seen message is copied to this folder.
* We should provide explicit information to the client about whether this
* message is seen or not, using this information. */
client->idlerefresh |= IDLE_REFRESH_RECENT;
} else if (STARTS_WITH(tmp, "EXPUNGE")) {
if (client->messages) {
Expand All @@ -3430,6 +3449,21 @@ static int process_idle(struct imap_client *client, char *s)
} else {
bbs_debug(6, "Ignoring FETCH update since not visible on current page\n");
}
/*! \todo XXX
* If the message was previously \Seen but now isn't, or wasn't, but now is,
* then we should also adjust the number of unseen messages in the folder.
* However, we don't actually keep track of the original flags for this message,
* so we can't do that. Even the frontend only knows the flags for the messages
* that are on the current page, but not other messages.
* As such, the correct count will "drift" currently. Workarounds would be:
* - Store a bit for each message, seen/unseen, and compare before/after (lots of inefficient accounting)
* - Issue a STATUS to get the current number of unseen messages. Slow/clunky from an IMAP perspective,
* but more efficient from a bookkeeping one for us.
*
* We have a similar problem for EXISTS and EXPUNGE, actually.
* For EXPUNGE, we correctly decrement client->messages, but we don't know what the new mailbox size is.
* For EXISTS, we correctly increment client->messages, but we don't know what the new mailbox size is.
*/
} else {
bbs_debug(3, "Ignoring IDLE data: %s", s); /* Already ends in LF */
}
Expand Down
56 changes: 51 additions & 5 deletions nets/net_imap.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -437,10 +437,14 @@ static int generate_mailbox_name(struct imap_session *imap, const char *restrict
if (atoi(bname)) {
/* INBOX */
safe_strncpy(buf, "INBOX", len);
} else {
} else if (*bname == '.') {
/* Skip leading . for maildir++ */
bname++; /* Skip leading . */
safe_strncpy(buf, bname, len);
} else {
strcpy(buf, "."); /* Copy something that will never match a real maildir name */
bbs_warning("Unexpected base name: %s\n", bname);
bbs_soft_assert(0);
}
/* No need to check our own ACL, we should have full permission */
} else {
Expand Down Expand Up @@ -495,7 +499,7 @@ void send_untagged_fetch(struct imap_session *imap, const char *maildir, int seq
RWLIST_TRAVERSE(&sessions, s, entry) {
int res;
char mboxname[256];
if (s == imap) { /* Skip if the update was caused by the client */
if (s == imap) { /* Skip if the update was caused by the client. The STORE handler already sent a response to this client if needed. */
continue;
}
/* imap->folder is maybe not the same name for s.
Expand Down Expand Up @@ -750,7 +754,7 @@ static void send_untagged_uidvalidity(struct mailbox *mbox, const char *maildir,
size_t len;
struct imap_session *s;

/* This should never happen, but if the UIDVALIDITY of a mailbox changes,
/* This should never happen, except for the first message, but if the UIDVALIDITY of a mailbox changes,
* we MUST notify any clients currently using it.
* Use same format as UIDVALIDITY response to SELECT/EXAMINE (RFC 2683 3.4.3) */
len = (size_t) snprintf(buf, sizeof(buf), "* OK [UIDVALIDITY %u] New UIDVALIDITY value!\r\n", uidvalidity);
Expand Down Expand Up @@ -790,6 +794,11 @@ static void imap_mbox_watcher(struct mailbox_event *event)
case EVENT_MESSAGE_EXPIRE:
send_untagged_expunge(event->node, event->mbox, event->maildir, event->expungesilent, event->uids, event->seqnos, event->numuids);
break;
case EVENT_FLAGS_SET:
case EVENT_FLAGS_CLEAR:
/* Do nothing here.
* send_untagged_fetch is already called in maildir_msg_setflags_modseq() */
break;
case EVENT_MAILBOX_CREATE:
case EVENT_MAILBOX_DELETE:
case EVENT_MAILBOX_RENAME:
Expand Down Expand Up @@ -1323,7 +1332,7 @@ static int select_examine_response(struct imap_session *imap, enum select_type r

if (was_selected) {
/* Best practice to always include some response text after [] */
imap_send(imap, "OK [CLOSED] Closed"); /* RFC 7162 3.2.11, example in 3.2.5.1 */
imap_send(imap, "OK [CLOSED] Previous mailbox closed"); /* RFC 7162 3.2.11, example in 3.2.5.1 */
}

imap_send(imap, "FLAGS (%s%s)", IMAP_FLAGS, numkeywords ? keywords : "");
Expand Down Expand Up @@ -2092,6 +2101,8 @@ static int handle_copy(struct imap_session *imap, const char *sequences, const c
} else if (!numcopies && quotaleft <= 0) {
imap_reply(imap, "NO [OVERQUOTA] Insufficient quota remaining");
} else {
/* maildir_copy_msg_filename sends the untagged EXISTS for the new message in the destination mailbox.
* No untagged EXISTS should be sent to the client running the command. */
if (IMAP_HAS_ACL(imap->acl, IMAP_ACL_READ)) {
imap_reply(imap, "OK [COPYUID %u %s %s] COPY completed", uidvalidity, S_IF(olduidstr), S_IF(newuidstr));
} else {
Expand Down Expand Up @@ -2205,6 +2216,8 @@ static int handle_move(struct imap_session *imap, const char *sequences, const c
if (expunged) {
imap->highestmodseq = maildir_indicate_expunged(EVENT_MESSAGE_EXPUNGE, imap->node, imap->mbox, imap->curdir, expunged, expungedseqs, exp_lengths, 0);
free(expunged);
/* maildir_move_msg_filename sends the untagged EXISTS for the new message in the destination mailbox.
* No untagged EXISTS should be sent to the client running the command. */
}

mailbox_unlock(imap->mbox);
Expand Down Expand Up @@ -2632,6 +2645,13 @@ static int handle_append(struct imap_session *imap, char *s)
return -1;
}

struct uintlist_metadata {
unsigned int *uids;
unsigned int *seqs;
int lengths;
int allocsizes;
};

struct copy_append {
struct imap_session *imap;
struct imap_client *appendclient;
Expand All @@ -2642,11 +2662,14 @@ struct copy_append {
unsigned int synchronizing:1;
unsigned int multiappend:1;
int appended;
struct uintlist_metadata *meta;
};

/*! \brief Part 2: Delete the source messages after a MOVE operation from local mailbox to a remote IMAP server */
static int copy_append_move_cb(const char *dir_name, const char *filename, int seqno, void *obj)
{
struct copy_append *ca = obj;
struct uintlist_metadata *meta = ca->meta;
unsigned int msguid;
char fullname[256];
int error = 0;
Expand All @@ -2662,10 +2685,13 @@ static int copy_append_move_cb(const char *dir_name, const char *filename, int s
snprintf(fullname, sizeof(fullname), "%s/%s", dir_name, filename);
if (unlink(fullname)) {
bbs_error("unlink(%s) failed: %s\n", fullname, strerror(errno));
} else {
uintlist_append2(&meta->uids, &meta->seqs, &meta->lengths, &meta->allocsizes, msguid, (unsigned int) seqno); /* store UID and seqno */
}
return 0;
}

/*! \brief Part 1: MOVE a message from a local mailbox to a remote IMAP server by APPEND'ing it */
static int copy_append_cb(const char *dir_name, const char *filename, int seqno, void *obj)
{
struct copy_append *ca = obj;
Expand Down Expand Up @@ -3058,8 +3084,17 @@ static int handle_remote_move(struct imap_session *imap, char *dest, const char
IMAP_CLIENT_EXPECT(&destclient->client, " OK"); /* tagged OK */
}
if (move) {
struct uintlist_metadata meta;
memset(&meta, 0, sizeof(meta));
ca.meta = &meta;
/* Now, delete all the source messages if this was a move operation */
maildir_ordered_traverse(imap->curdir, copy_append_move_cb, &ca); /* Delete */
if (meta.uids) {
/* We need to send an untagged EXPUNGE for all messages that no longer exist locally. */
imap->highestmodseq = maildir_indicate_expunged(EVENT_MESSAGE_EXPUNGE, imap->node, imap->mbox, imap->curdir, meta.uids, meta.seqs, meta.lengths, 0);
free(meta.uids);
}
free_if(meta.seqs);
}
imap_reply(imap, "OK %s completed", move ? "MOVE" : "COPY");
} else {
Expand Down Expand Up @@ -4847,10 +4882,21 @@ static int imap_process(struct imap_session *imap, char *s)
done:
if (res) {
bbs_debug(4, "%s command returned %d\n", command, res);
} else if (!strlen_zero(command)) {
}
#if 0
/* Can't call flush_updates here, the command has already completed
* (this is when we are "supposed" to send updates to the client, but that is
* BEFORE the tagged response, not AFTER.
*
* \todo XXX What we SHOULD be doing is calling flush_updates
* right before every command sends an untagged response;
* unfortunately, there is no easy way to do that besides
* copying that to every command's function. */
else if (!strlen_zero(command)) {
res = flush_updates(imap, command, NULL);
res = res < 0 ? -1 : 0;
}
#endif
return res;
}

Expand Down
3 changes: 3 additions & 0 deletions nets/net_imap/imap.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,9 @@ struct imap_traversal {
char curdir[260];
};

/* Note: Although these are used frequently,
* the hierarchy delimiter has become hardcoded in several places
* that should be updated to use this macro. */
#define HIERARCHY_DELIMITER "."
#define HIERARCHY_DELIMITER_CHAR '.'

Expand Down
9 changes: 9 additions & 0 deletions nets/net_imap/imap_server_flags.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -498,8 +498,17 @@ int maildir_msg_setflags_modseq(struct imap_session *imap, int seqno, const char
/* Send unilateral untagged FETCH responses to everyone except this session, to notify of the new flags */
generate_flag_names_full(imap, newflagletters, newflags, sizeof(newflags), &newbuf, &newlen);
if (seqno) { /* Skip for merely translating flag mappings between maildirs */
char *end;
unsigned int uid;
maildir_parse_uid_from_filename(filename, &uid);
/* Right now, dir ends in '/cur', since it's the physical maildir cur dir path,
* here, we want just the base maildir path (without '/cur' at the end). */
end = strrchr(dirpath, '/');
if (end) {
*end = '\0';
} else {
bbs_soft_assert(0);
}
send_untagged_fetch(imap, dirpath, seqno, uid, modseq, newflags);
}
return 0;
Expand Down
6 changes: 5 additions & 1 deletion nets/net_imap/imap_server_list.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,7 @@ int list_iterate(struct imap_session *imap, struct list_command *lcmd, int level
char fulldir[257];
char mailboxbuf[256];
char relativepath[512];
char *relpath = relativepath;
const char *mailboxname = entry->d_name;

/* Only care about directories, not files. */
Expand Down Expand Up @@ -416,8 +417,11 @@ int list_iterate(struct imap_session *imap, struct list_command *lcmd, int level
if (*mailboxname == HIERARCHY_DELIMITER_CHAR) {
mailboxname++;
}
if (*relpath == HIERARCHY_DELIMITER_CHAR) {
relpath++;
}

if (cb(imap, lcmd, relativepath, data)) {
if (cb(imap, lcmd, relpath, data)) {
goto cleanup;
}

Expand Down
Loading

0 comments on commit b41845b

Please sign in to comment.