From e7333ee4aca8cbd866eb2b5a6420a56f0b956ead Mon Sep 17 00:00:00 2001
From: Timo Sirainen <timo.sirainen@dovecot.fi>
Date: Tue, 9 Jan 2018 15:37:25 -0500
Subject: [PATCH] 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.
---
 src/lib-storage/mail-storage.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/src/lib-storage/mail-storage.c b/src/lib-storage/mail-storage.c
index 871cb02f63..fd2fe67b7a 100644
--- a/src/lib-storage/mail-storage.c
+++ b/src/lib-storage/mail-storage.c
@@ -1467,9 +1467,20 @@ int mailbox_create(struct mailbox *box, const struct mailbox_update *update,
 	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 @@ static void mailbox_close_reset_path(struct mailbox *box)
 
 int mailbox_delete(struct mailbox *box)
 {
+	bool list_locked;
 	int ret;
 
 	if (*box->name == '\0') {
@@ -1572,13 +1584,22 @@ int mailbox_delete(struct mailbox *box)
 		/* 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 @@ int mailbox_rename(struct mailbox *src, struct mailbox *dest)
 		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;