From: Timo Sirainen <timo.sirainen@dovecot.fi>
Date: Mon, 24 Oct 2016 21:22:20 +0000 (+0300)
Subject: director: Prevent race conditions by adding USER_KILL_STATE_FLUSHING
X-Git-Tag: 2.2.26~21
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=45f7ffde889165c7b330fc98708fcbe595c78c05;p=thirdparty%2Fdovecot%2Fcore.git

director: Prevent race conditions by adding USER_KILL_STATE_FLUSHING

In theory it's possible that a user is freed during a flush and added back
before flush is finished, possibly even being moved again. This check makes
sure that we don't finish such move unless we're actually at the correct
flushing state. (If there's another flush also running for the user it'll
be ignored.)

This is also useful for logging purposes.
---

diff --git a/src/director/director.c b/src/director/director.c
index cfac4e60c1..9242979383 100644
--- a/src/director/director.c
+++ b/src/director/director.c
@@ -717,9 +717,18 @@ director_flush_user_continue(int result,
 	struct user *user =
 		user_directory_lookup(ctx->dir->users, ctx->username_hash);
 
-	if (user != NULL)
+	if (user == NULL) {
+		dir_debug("User %u freed while flushing, result=%d",
+			  ctx->username_hash, result);
+	} else if (user->kill_state != USER_KILL_STATE_FLUSHING) {
+		dir_debug("User %u move state changed while flushing, result=%d",
+			  ctx->username_hash, result);
+	} else {
+		dir_debug("Flushing user %u finished, result=%d",
+			  ctx->username_hash, result);
 		director_user_kill_finish_delayed(ctx->dir, user,
 						  result == 1);
+	}
 	if (result == 0) {
 		struct istream *is = iostream_temp_finish(&ctx->reply, (size_t)-1);
 		char *data;
@@ -781,6 +790,8 @@ director_flush_user(struct director *dir, struct user *user)
 	const char *const args[] = {"FLUSH",
 		t_strdup_printf("%u", user->username_hash), NULL};
 
+	user->kill_state = USER_KILL_STATE_FLUSHING;
+
 	if ((program_client_create(ctx->socket_path, args, &set, FALSE,
 				   &ctx->pclient, &error)) != 0) {
 		i_error("%s: Failed to flush user hash %u in host %s: %s",
@@ -857,6 +868,7 @@ struct director_kill_context {
 static void
 director_finish_user_kill(struct director *dir, struct user *user, bool self)
 {
+	i_assert(user->kill_state != USER_KILL_STATE_FLUSHING);
 	i_assert(user->kill_state != USER_KILL_STATE_DELAY);
 
 	if (dir->right == NULL) {
@@ -925,6 +937,7 @@ static void director_user_move_throttled(unsigned int new_events_count,
 
 static void director_user_move_timeout(struct user *user)
 {
+	i_assert(user->kill_state != USER_KILL_STATE_FLUSHING);
 	i_assert(user->kill_state != USER_KILL_STATE_DELAY);
 
 	if (log_throttle_accept(user_move_throttle)) {
@@ -1095,6 +1108,7 @@ void director_user_killed(struct director *dir, unsigned int username_hash)
 		director_finish_user_kill(dir, user, TRUE);
 		break;
 	case USER_KILL_STATE_NONE:
+	case USER_KILL_STATE_FLUSHING:
 	case USER_KILL_STATE_DELAY:
 	case USER_KILL_STATE_KILLING_NOTIFY_RECEIVED:
 		break;
diff --git a/src/director/user-directory.c b/src/director/user-directory.c
index 0dd8e29483..f516d4c565 100644
--- a/src/director/user-directory.c
+++ b/src/director/user-directory.c
@@ -42,6 +42,7 @@ const char *user_kill_state_names[USER_KILL_STATE_DELAY+1] = {
 	"notify-received",
 	"waiting-for-notify",
 	"waiting-for-everyone",
+	"flushing",
 	"delay",
 };
 
diff --git a/src/director/user-directory.h b/src/director/user-directory.h
index b68d7e14bc..fe15babf5e 100644
--- a/src/director/user-directory.h
+++ b/src/director/user-directory.h
@@ -16,6 +16,8 @@ enum user_kill_state {
 	/* We're done killing, but waiting for USER-KILLED-EVERYWHERE
 	   notification until this state gets reset. */
 	USER_KILL_STATE_KILLED_WAITING_FOR_EVERYONE,
+	/* Waiting for the flush socket to finish. */
+	USER_KILL_STATE_FLUSHING,
 	/* Wait for a while for the user connections to actually die. Note that
 	   only at this stage we can be sure that all the directors know about
 	   the user move (although it could be earlier if we added a new