Mailbox deletion race: folders and files are never deleted

[Samir Aguiar]

Hi,

we found a race condition problem in Cyrus IMAP 2.4.17 where mailbox folders 
(and the files inside them) are not removed when deleting a mailbox with delete 
mode set to "immediate".

The steps to reproduce are:
- Open a mailbox in one session (session A);
- Delete that mailbox with another session (session B).

Session B updates the index with the OPT_MAILBOX_DELETED flag and removes the 
mailbox from the database. It expects session A to wipe out the files from the 
filesystem later in mailbox_close().

Because the database now has no record of the mailbox, no further sessions 
will be able to clean up those files. Neither will cyr_expire, because that 
mailbox won't be listed by the mboxlist* functions. So session A is the one 
that should remove the leftovers.

However, when closing the mailbox, session A executes the following piece of 
code:

    /* drop the index lock here because we'll lose our right to it
     * when try to upgrade the mboxlock anyway. */
    mailbox_unlock_index(mailbox, NULL);

    /* do we need to try and clean up? (not if doing a shutdown,
     * speed is probably more important!) */
    if (!in_shutdown && (mailbox->i.options & MAILBOX_CLEANUP_MASK)) {
        int r = mailbox_mboxlock_reopen(listitem, LOCK_NONBLOCKING);
        if (!r) r = mailbox_open_index(mailbox);
        if (!r) r = mailbox_lock_index(mailbox, LOCK_EXCLUSIVE);
        if (!r) {
            /* finish cleaning up */
            if (mailbox->i.options & OPT_MAILBOX_DELETED)
            mailbox_delete_cleanup(mailbox->part, mailbox->name);
            else if (mailbox->i.options & OPT_MAILBOX_NEEDS_REPACK)
            mailbox_index_repack(mailbox);
            else if (mailbox->i.options & OPT_MAILBOX_NEEDS_UNLINK)
            mailbox_index_unlink(mailbox);
            /* or we missed out - someone else beat us to it */
        }
        /* otherwise someone else has the mailbox locked
         * already, so they can handle the cleanup in
         * THEIR mailbox_close call */
    }

There are two problems here:
- if the "in_shutdown" flag is set, session A will exit and the files will be 
forgotten forever;
- the index file is only reloaded by the call to mailbox_open_index, which is 
inside the if. That means that session A will be unaware of the clean up mask 
set by session B when checking this if and again the files will never be 
cleaned.

Proposed solution:
- Patch the mailbox_close() function to reload the index before trying to 
clean the files (and merge the current index changes with the ones found)
- Because the above would still be skipped when shutting down Cyrus: expand 
cyr_expire to find mailboxes with the deleted flag set (by looping through the 
filesystem, and not by using the database) and try to remove them. This could 
then be run periodically by administrators.

What do you think?

Best regards,
Samir Aguiar