From 301ccffc481669e79008279eed43d6bce5944e9c Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Fri, 20 Dec 2024 22:09:02 +1100 Subject: [PATCH 1/5] conversations: don't store basecid if the same This matches the logic in mailboxes.c, and makes audit tests pass --- imap/conversations.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/imap/conversations.c b/imap/conversations.c index 0e86cca7f6..00adc73526 100644 --- a/imap/conversations.c +++ b/imap/conversations.c @@ -2280,7 +2280,7 @@ static int conversations_guid_setitem(struct conversations_state *state, buf_appendbit32(&val, system_flags); buf_appendbit32(&val, internal_flags); buf_appendbit64(&val, (bit64)internaldate); - buf_appendbit64(&val, basecid); + buf_appendbit64(&val, basecid == cid ? 0 : basecid); } r = cyrusdb_store(state->db, buf_base(&key), buf_len(&key), From 25168e830d395ba06c7547cee10b80ef07a54a4b Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Fri, 20 Dec 2024 23:30:30 +1100 Subject: [PATCH 2/5] rename: create an INBOX record to find the conversationsdb This stops us writing to the wrong DB when copying the INBOX, which fixes a long standing bug with UUID mailboxes and renames. This fixes both the imapd rename, and the replication rename codepaths. --- imap/imapd.c | 21 +++++++++++++++++++ imap/sync_support.c | 49 ++++++++++++++++++++++++++++++++++++--------- 2 files changed, 61 insertions(+), 9 deletions(-) diff --git a/imap/imapd.c b/imap/imapd.c index 4d628faf30..59702d0fab 100644 --- a/imap/imapd.c +++ b/imap/imapd.c @@ -7875,6 +7875,25 @@ static void cmd_rename(char *tag, char *oldname, char *newname, char *location, imapd_userisadmin) { rename_user = 1; + + // we can't rename users if the new inbox already exists! + mbentry_t *destmbentry = NULL; + r = mboxlist_lookup_allow_all(newmailboxname, &destmbentry, NULL); + if (r != IMAP_MAILBOX_NONEXISTENT) { + mboxlist_entry_free(&destmbentry); + r = IMAP_MAILBOX_EXISTS; + goto respond; + } + + /* we need to create a reference for the uniqueid so we find the right + * conversations DB */ + mbentry_t *newmbentry = mboxlist_entry_copy(mbentry); + free(newmbentry->name); + newmbentry->name = xstrdup(newmailboxname); + newmbentry->mbtype |= MBTYPE_DELETED; + r = mboxlist_update_full(newmbentry, /*localonly*/1, /*silent*/1); + mboxlist_entry_free(&newmbentry); + if (r) goto respond; } /* if we're renaming something inside of something else, @@ -8037,6 +8056,8 @@ static void cmd_rename(char *tag, char *oldname, char *newname, char *location, if (r) { prot_printf(imapd_out, "%s NO %s\r\n", tag, error_message(r)); + // ensure the mboxlist entry gets fixed up + if (rename_user) mboxlist_update_full(mbentry, /*localonly*/1, /*silent*/1); } else { if (config_mupdate_server) kick_mupdate(); diff --git a/imap/sync_support.c b/imap/sync_support.c index 7d06ea69a9..d915da4da2 100644 --- a/imap/sync_support.c +++ b/imap/sync_support.c @@ -3853,6 +3853,7 @@ int sync_apply_rename(struct dlist *kin, struct sync_state *sstate) struct mboxlock *oldlock = NULL; struct mboxlock *newlock = NULL; + int rename_user = 0; /* make sure we grab these locks in a stable order! */ if (strcmpsafe(oldmboxname, newmboxname) < 0) { @@ -3866,17 +3867,47 @@ int sync_apply_rename(struct dlist *kin, struct sync_state *sstate) } r = mboxlist_lookup_allow_all(oldmboxname, &mbentry, 0); + if (r) goto done; + + if (mboxname_isusermailbox(oldmboxname, 1) && + mboxname_isusermailbox(newmboxname, 1)) { + // we can't rename users if the new inbox already exists! + mbentry_t *destmbentry = NULL; + r = mboxlist_lookup_allow_all(newmboxname, &destmbentry, NULL); + if (r != IMAP_MAILBOX_NONEXISTENT) { + mboxlist_entry_free(&destmbentry); + r = IMAP_MAILBOX_EXISTS; + goto done; + } + + rename_user = 1; + + /* we need to create a reference for the uniqueid so we find the right + * conversations DB */ + mbentry_t *newmbentry = mboxlist_entry_copy(mbentry); + free(newmbentry->name); + newmbentry->name = xstrdup(newmboxname); + newmbentry->mbtype |= MBTYPE_DELETED; + r = mboxlist_update_full(newmbentry, /*localonly*/1, /*silent*/1); + mboxlist_entry_free(&newmbentry); + if (r) goto done; + } + + r = mboxlist_renamemailbox(mbentry, newmboxname, partition, + uidvalidity, 1, sstate->userid, + sstate->authstate, NULL, + (sstate->flags & SYNC_FLAG_LOCALONLY), + 1/*forceuser*/, + 1/*ignorequota*/, + 1/*keep_intermediaries*/, + 0/*move_subscription*/, + 1/*silent*/); - if (!r) r = mboxlist_renamemailbox(mbentry, newmboxname, partition, - uidvalidity, 1, sstate->userid, - sstate->authstate, NULL, - (sstate->flags & SYNC_FLAG_LOCALONLY), - 1/*forceuser*/, - 1/*ignorequota*/, - 1/*keep_intermediaries*/, - 0/*move_subscription*/, - 1/*silent*/); + // undo the entry write if doing a user inbox + if (r && rename_user) + mboxlist_update_full(mbentry, /*localonly*/1, /*silent*/1); +done: mboxlist_entry_free(&mbentry); mboxname_release(&oldlock); mboxname_release(&newlock); From 4a0ab7e6e0b0099e09ad530b15f5e57660edd21d Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Fri, 20 Dec 2024 23:31:44 +1100 Subject: [PATCH 3/5] user: find the inbox uniqueid a bit more aggressively --- imap/user.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/imap/user.c b/imap/user.c index 3bdcf5286b..2bcb3c92fe 100644 --- a/imap/user.c +++ b/imap/user.c @@ -198,7 +198,7 @@ EXPORTED const char *user_sieve_path(const char *inuser) char *inboxname = mboxname_user_mbox(user, NULL); mbentry_t *mbentry = NULL; - int r = mboxlist_lookup(inboxname, &mbentry, NULL); + int r = mboxlist_lookup_allow_all(inboxname, &mbentry, NULL); free(inboxname); if (r || (mbentry->mbtype & MBTYPE_LEGACY_DIRS)) { @@ -633,7 +633,7 @@ EXPORTED char *user_hash_xapian(const char *userid, const char *root) char *basedir = NULL; int r; - r = mboxlist_lookup(inboxname, &mbentry, NULL); + r = mboxlist_lookup_allow_all(inboxname, &mbentry, NULL); if (r) goto out; mbname = mbname_from_intname(mbentry->name); From 438ccc4e67294e9c97801d4c363d7275edc73979 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Mon, 23 Dec 2024 12:18:27 +1100 Subject: [PATCH 4/5] imapd: fix indent on some lines This just makes it easier to read --- imap/imapd.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/imap/imapd.c b/imap/imapd.c index 59702d0fab..1223e76c8a 100644 --- a/imap/imapd.c +++ b/imap/imapd.c @@ -7938,7 +7938,7 @@ static void cmd_rename(char *tag, char *oldname, char *newname, char *location, rock.newuser = newuser; rock.partition = location; rock.rename_user = rename_user; - rock.noisy = noisy; + rock.noisy = noisy; /* Check mboxnames to ensure we can write them all BEFORE we start */ r = mboxlist_allmbox(ombn, checkmboxname, &rock, 0); @@ -8012,12 +8012,12 @@ static void cmd_rename(char *tag, char *oldname, char *newname, char *location, /* rename all mailboxes matching this */ if (!r && recursive_rename) { - if (noisy) { + if (noisy) { prot_printf(imapd_out, "* OK [INPROGRESS (\"%s\" NIL NIL)] rename %s %s\r\n", tag, oldextname, newextname); prot_flush(imapd_out); - } + } submboxes: strcat(oldmailboxname, "."); From 75509c3a31fe3753092fb046a4f4354255f8436a Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Mon, 23 Dec 2024 12:18:32 +1100 Subject: [PATCH 5/5] alh review - fix error handling on user rename codepaths --- imap/imapd.c | 45 ++++++++++++++++++++++++++++----------- imap/sync_support.c | 51 +++++++++++++++++++++++++++++++-------------- 2 files changed, 68 insertions(+), 28 deletions(-) diff --git a/imap/imapd.c b/imap/imapd.c index 1223e76c8a..b545715cd5 100644 --- a/imap/imapd.c +++ b/imap/imapd.c @@ -7605,6 +7605,8 @@ static void cmd_rename(char *tag, char *oldname, char *newname, char *location, int rename_user = 0; int mbtype = 0; mbentry_t *mbentry = NULL; + mbentry_t *olddestmbentry = NULL; + mbentry_t *newdestmbentry = NULL; struct renrock rock = {0}; const char *orig_oldname = oldname; const char *orig_newname = newname; @@ -7877,22 +7879,29 @@ static void cmd_rename(char *tag, char *oldname, char *newname, char *location, rename_user = 1; // we can't rename users if the new inbox already exists! - mbentry_t *destmbentry = NULL; - r = mboxlist_lookup_allow_all(newmailboxname, &destmbentry, NULL); - if (r != IMAP_MAILBOX_NONEXISTENT) { - mboxlist_entry_free(&destmbentry); + r = mboxlist_lookup_allow_all(newmailboxname, &olddestmbentry, NULL); + if (r == IMAP_MAILBOX_NONEXISTENT) { + // all good, there's nothing here + } else if (r) { + // any other error, abort - something bad with mailboxesdb + goto respond; + } else if (olddestmbentry->mbtype & MBTYPE_DELETED) { + // this is OK, we're replacing a tombstone - hold on to the old entry in case we need to recreate it + } + else { + // can't rename over an existing mailbox - abort + mboxlist_entry_free(&olddestmbentry); r = IMAP_MAILBOX_EXISTS; goto respond; } /* we need to create a reference for the uniqueid so we find the right * conversations DB */ - mbentry_t *newmbentry = mboxlist_entry_copy(mbentry); - free(newmbentry->name); - newmbentry->name = xstrdup(newmailboxname); - newmbentry->mbtype |= MBTYPE_DELETED; - r = mboxlist_update_full(newmbentry, /*localonly*/1, /*silent*/1); - mboxlist_entry_free(&newmbentry); + newdestmbentry = mboxlist_entry_copy(mbentry); + free(newdestmbentry->name); + newdestmbentry->name = xstrdup(newmailboxname); + newdestmbentry->mbtype |= MBTYPE_DELETED; + r = mboxlist_update_full(newdestmbentry, /*localonly*/1, /*silent*/1); if (r) goto respond; } @@ -8056,8 +8065,18 @@ static void cmd_rename(char *tag, char *oldname, char *newname, char *location, if (r) { prot_printf(imapd_out, "%s NO %s\r\n", tag, error_message(r)); - // ensure the mboxlist entry gets fixed up - if (rename_user) mboxlist_update_full(mbentry, /*localonly*/1, /*silent*/1); + // ensure the mboxlist entry gets fixed up or removed + if (olddestmbentry) { + int r2 = mboxlist_update_full(olddestmbentry, /*localonly*/1, /*silent*/1); + if (r2) + xsyslog(LOG_ERR, "IOERROR: replacing old destination tombstone after rename error", + "mboxname=<%s>, error=<%s>", olddestmbentry->name, error_message(r)); + } else if (newdestmbentry) { + int r2 = mboxlist_delete(newdestmbentry); + if (r2) + xsyslog(LOG_ERR, "IOERROR: removing temporary uniqueid tombstone after rename error", + "mboxname=<%s>, error=<%s>", newdestmbentry->name, error_message(r)); + } } else { if (config_mupdate_server) kick_mupdate(); @@ -8092,6 +8111,8 @@ static void cmd_rename(char *tag, char *oldname, char *newname, char *location, if (!r && rename_user) user_sharee_renameacls(&imapd_namespace, olduser, newuser); mboxlist_entry_free(&mbentry); + mboxlist_entry_free(&olddestmbentry); + mboxlist_entry_free(&newdestmbentry); if (oldname != orig_oldname) free(oldname); if (newname != orig_newname) free(newname); strarray_fini(&listargs.pat); diff --git a/imap/sync_support.c b/imap/sync_support.c index d915da4da2..123cbc360c 100644 --- a/imap/sync_support.c +++ b/imap/sync_support.c @@ -3853,7 +3853,8 @@ int sync_apply_rename(struct dlist *kin, struct sync_state *sstate) struct mboxlock *oldlock = NULL; struct mboxlock *newlock = NULL; - int rename_user = 0; + mbentry_t *olddestmbentry = NULL; + mbentry_t *newdestmbentry = NULL; /* make sure we grab these locks in a stable order! */ if (strcmpsafe(oldmboxname, newmboxname) < 0) { @@ -3872,24 +3873,29 @@ int sync_apply_rename(struct dlist *kin, struct sync_state *sstate) if (mboxname_isusermailbox(oldmboxname, 1) && mboxname_isusermailbox(newmboxname, 1)) { // we can't rename users if the new inbox already exists! - mbentry_t *destmbentry = NULL; - r = mboxlist_lookup_allow_all(newmboxname, &destmbentry, NULL); - if (r != IMAP_MAILBOX_NONEXISTENT) { - mboxlist_entry_free(&destmbentry); + r = mboxlist_lookup_allow_all(newmboxname, &olddestmbentry, NULL); + if (r == IMAP_MAILBOX_NONEXISTENT) { + // all good, there's nothing here + } else if (r) { + // any other error, abort - something bad with mailboxesdb + goto done; + } else if (olddestmbentry->mbtype & MBTYPE_DELETED) { + // this is OK, we're replacing a tombstone - hold on to the old entry in case we need to recreate it + } + else { + // can't rename over an existing mailbox - abort + mboxlist_entry_free(&olddestmbentry); r = IMAP_MAILBOX_EXISTS; goto done; } - rename_user = 1; - /* we need to create a reference for the uniqueid so we find the right * conversations DB */ - mbentry_t *newmbentry = mboxlist_entry_copy(mbentry); - free(newmbentry->name); - newmbentry->name = xstrdup(newmboxname); - newmbentry->mbtype |= MBTYPE_DELETED; - r = mboxlist_update_full(newmbentry, /*localonly*/1, /*silent*/1); - mboxlist_entry_free(&newmbentry); + newdestmbentry = mboxlist_entry_copy(mbentry); + free(newdestmbentry->name); + newdestmbentry->name = xstrdup(newmboxname); + newdestmbentry->mbtype |= MBTYPE_DELETED; + r = mboxlist_update_full(newdestmbentry, /*localonly*/1, /*silent*/1); if (r) goto done; } @@ -3903,12 +3909,25 @@ int sync_apply_rename(struct dlist *kin, struct sync_state *sstate) 0/*move_subscription*/, 1/*silent*/); - // undo the entry write if doing a user inbox - if (r && rename_user) - mboxlist_update_full(mbentry, /*localonly*/1, /*silent*/1); done: + if (r) { + // ensure the mboxlist entry gets fixed up or removed + if (olddestmbentry) { + int r2 = mboxlist_update_full(olddestmbentry, /*localonly*/1, /*silent*/1); + if (r2) + xsyslog(LOG_ERR, "IOERROR: replacing old destination tombstone after rename error", + "mboxname=<%s>, error=<%s>", olddestmbentry->name, error_message(r)); + } else if (newdestmbentry) { + int r2 = mboxlist_delete(newdestmbentry); + if (r2) + xsyslog(LOG_ERR, "IOERROR: removing temporary uniqueid tombstone after rename error", + "mboxname=<%s>, error=<%s>", newdestmbentry->name, error_message(r)); + } + } mboxlist_entry_free(&mbentry); + mboxlist_entry_free(&olddestmbentry); + mboxlist_entry_free(&newdestmbentry); mboxname_release(&oldlock); mboxname_release(&newlock);