Mailbox deletion race: folders and files are never deleted

[Jan Parcel]

On 2/26/15 7:51 AM, Samir Aguiar wrote:
> 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
Not likely to be popular with adminstrators of multi-terrabyte systems 
with thousands of users.