Mercurial > dovecot > core-2.2
changeset 22823:c2133393765c
lib-storage: Lock mailbox_list for mailbox create/delete/rename
This is only required for mailbox creation to fix a race condition with
LAYOUT=index: If INBOX doesn't exist it will rescan the mailboxes to
find out if there are any missing ones. If INBOX creation isn't locked,
it's possible that the first process hasn't finished creating INBOX
before the second process find it and attempts to open it.
The delete and rename locking are probably useful to guard against race
conditions when clients intentionally issues create/delete/rename commands
concurrently.
| author | Timo Sirainen <timo.sirainen@dovecot.fi> |
|---|---|
| date | Tue, 09 Jan 2018 15:37:25 -0500 |
| parents | 9ffa1a9b4d80 |
| children | bd5cea4da2ce |
| files | src/lib-storage/mail-storage.c |
| diffstat | 1 files changed, 32 insertions(+), 2 deletions(-) [+] |
line wrap: on
line diff
--- a/src/lib-storage/mail-storage.c Tue Jan 09 15:36:58 2018 -0500 +++ b/src/lib-storage/mail-storage.c Tue Jan 09 15:37:25 2018 -0500 @@ -1467,9 +1467,20 @@ if (mailbox_verify_create_name(box) < 0) return -1; + /* Avoid race conditions by keeping mailbox list locked during changes. + This especially fixes a race during INBOX creation with LAYOUT=index + because it scans for missing mailboxes if INBOX doesn't exist. The + second process's scan can find a half-created INBOX and add it, + causing the first process to become confused. */ + if (mailbox_list_lock(box->list) < 0) { + mail_storage_copy_list_error(box->storage, box->list); + return -1; + } box->creating = TRUE; ret = box->v.create_box(box, update, directory); box->creating = FALSE; + mailbox_list_unlock(box->list); + if (ret == 0) { box->list->guid_cache_updated = TRUE; if (!box->inbox_any) @@ -1556,6 +1567,7 @@ int mailbox_delete(struct mailbox *box) { + bool list_locked; int ret; if (*box->name == '\0') { @@ -1572,13 +1584,22 @@ /* might be a \noselect mailbox, so continue deletion */ } - ret = box->v.delete_box(box); + if (mailbox_list_lock(box->list) < 0) { + mail_storage_copy_list_error(box->storage, box->list); + list_locked = FALSE; + ret = -1; + } else { + list_locked = TRUE; + ret = box->v.delete_box(box); + } if (ret < 0 && box->marked_deleted) { /* deletion failed. revert the mark so it can maybe be tried again later. */ if (mailbox_mark_index_deleted(box, FALSE) < 0) ret = -1; } + if (list_locked) + mailbox_list_unlock(box->list); box->deleting = FALSE; mailbox_close(box); @@ -1729,7 +1750,16 @@ return -1; } - if (src->v.rename_box(src, dest) < 0) + /* It would be safer to lock both source and destination, but that + could lead to deadlocks. So at least for now lets just lock only the + destination list. */ + if (mailbox_list_lock(dest->list) < 0) { + mail_storage_copy_list_error(src->storage, dest->list); + return -1; + } + int ret = src->v.rename_box(src, dest); + mailbox_list_unlock(dest->list); + if (ret < 0) return -1; src->list->guid_cache_invalidated = TRUE; dest->list->guid_cache_invalidated = TRUE;