Skip to content

Commit

Permalink
alh review - fix error handling on user rename codepaths
Browse files Browse the repository at this point in the history
  • Loading branch information
@brong
brong committed Dec 23, 2024
1 parent 438ccc4 commit 75509c3
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 28 deletions.
45 changes: 33 additions & 12 deletions imap/imapd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand Down
51 changes: 35 additions & 16 deletions imap/sync_support.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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;
}

Expand All @@ -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);

Expand Down

0 comments on commit 75509c3

Please sign in to comment.