From: Timo Sirainen <tss@iki.fi>
Date: Thu, 3 Sep 2015 17:54:27 +0000 (+0300)
Subject: dict: Various reference counting and other fixes related to using freed memory.
X-Git-Tag: 2.2.19.rc1~113
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ac00e30516d93d289c71bd59f338c26693329e67;p=thirdparty%2Fdovecot%2Fcore.git

dict: Various reference counting and other fixes related to using freed memory.
---

diff --git a/src/dict/dict-commands.c b/src/dict/dict-commands.c
index 2fb43fc98e..5b73cc25fd 100644
--- a/src/dict/dict-commands.c
+++ b/src/dict/dict-commands.c
@@ -27,7 +27,7 @@ struct dict_connection_cmd {
 	struct dict_iterate_context *iter;
 	enum dict_iterate_flags iter_flags;
 
-	struct dict_connection_transaction *trans;
+	unsigned int trans_id;
 };
 
 static void dict_connection_cmd_output_more(struct dict_connection_cmd *cmd);
@@ -36,10 +36,10 @@ static void dict_connection_cmd_free(struct dict_connection_cmd *cmd)
 {
 	if (cmd->iter != NULL)
 		(void)dict_iterate_deinit(&cmd->iter);
-	array_delete(&cmd->conn->cmds, 0, 1);
 	i_free(cmd->reply);
 
-	dict_connection_continue_input(cmd->conn);
+	if (dict_connection_unref(cmd->conn))
+		dict_connection_continue_input(cmd->conn);
 	i_free(cmd);
 }
 
@@ -51,6 +51,7 @@ static void dict_connection_cmd_remove(struct dict_connection_cmd *cmd)
 	cmds = array_get(&cmd->conn->cmds, &count);
 	for (i = 0; i < count; i++) {
 		if (cmds[i] == cmd) {
+			array_delete(&cmd->conn->cmds, i, 1);
 			dict_connection_cmd_free(cmd);
 			return;
 		}
@@ -62,6 +63,7 @@ static void dict_connection_cmds_flush(struct dict_connection *conn)
 {
 	struct dict_connection_cmd *cmd, *const *first_cmdp;
 
+	dict_connection_ref(conn);
 	while (array_count(&conn->cmds) > 0) {
 		first_cmdp = array_idx(&conn->cmds, 0);
 		cmd = *first_cmdp;
@@ -74,16 +76,7 @@ static void dict_connection_cmds_flush(struct dict_connection *conn)
 		o_stream_nsend_str(conn->output, cmd->reply);
 		dict_connection_cmd_remove(cmd);
 	}
-}
-
-void dict_connection_cmds_free(struct dict_connection *conn)
-{
-	struct dict_connection_cmd *const *first_cmdp;
-
-	while (array_count(&conn->cmds) > 0) {
-		first_cmdp = array_idx(&conn->cmds, 0);
-		dict_connection_cmd_remove(*first_cmdp);
-	}
+	dict_connection_unref(conn);
 }
 
 static void
@@ -201,20 +194,20 @@ dict_connection_transaction_lookup(struct dict_connection *conn,
 
 static void
 dict_connection_transaction_array_remove(struct dict_connection *conn,
-					 struct dict_connection_transaction *trans)
+					 unsigned int id)
 {
 	const struct dict_connection_transaction *transactions;
 	unsigned int i, count;
 
-	i_assert(trans->ctx == NULL);
-
 	transactions = array_get(&conn->transactions, &count);
 	for (i = 0; i < count; i++) {
-		if (&transactions[i] == trans) {
+		if (transactions[i].id == id) {
+			i_assert(transactions[i].ctx == NULL);
 			array_delete(&conn->transactions, i, 1);
-			break;
+			return;
 		}
 	}
+	i_unreached();
 }
 
 static int cmd_begin(struct dict_connection_cmd *cmd, const char *line)
@@ -279,11 +272,11 @@ cmd_commit_finish(struct dict_connection_cmd *cmd, int ret, bool async)
 	}
 	if (async) {
 		cmd->reply = i_strdup_printf("%c%c%u\n",
-			DICT_PROTOCOL_REPLY_ASYNC_COMMIT, chr, cmd->trans->id);
+			DICT_PROTOCOL_REPLY_ASYNC_COMMIT, chr, cmd->trans_id);
 	} else {
-		cmd->reply = i_strdup_printf("%c%u\n", chr, cmd->trans->id);
+		cmd->reply = i_strdup_printf("%c%u\n", chr, cmd->trans_id);
 	}
-	dict_connection_transaction_array_remove(cmd->conn, cmd->trans);
+	dict_connection_transaction_array_remove(cmd->conn, cmd->trans_id);
 	dict_connection_cmds_flush(cmd->conn);
 }
 
@@ -304,20 +297,26 @@ static void cmd_commit_callback_async(int ret, void *context)
 static int
 cmd_commit(struct dict_connection_cmd *cmd, const char *line)
 {
-	if (dict_connection_transaction_lookup_parse(cmd->conn, line, &cmd->trans) < 0)
+	struct dict_connection_transaction *trans;
+
+	if (dict_connection_transaction_lookup_parse(cmd->conn, line, &trans) < 0)
 		return -1;
+	cmd->trans_id = trans->id;
 
-	dict_transaction_commit_async(&cmd->trans->ctx, cmd_commit_callback, cmd);
+	dict_transaction_commit_async(&trans->ctx, cmd_commit_callback, cmd);
 	return 1;
 }
 
 static int
 cmd_commit_async(struct dict_connection_cmd *cmd, const char *line)
 {
-	if (dict_connection_transaction_lookup_parse(cmd->conn, line, &cmd->trans) < 0)
+	struct dict_connection_transaction *trans;
+
+	if (dict_connection_transaction_lookup_parse(cmd->conn, line, &trans) < 0)
 		return -1;
+	cmd->trans_id = trans->id;
 
-	dict_transaction_commit_async(&cmd->trans->ctx, cmd_commit_callback_async, cmd);
+	dict_transaction_commit_async(&trans->ctx, cmd_commit_callback_async, cmd);
 	return 1;
 }
 
@@ -329,7 +328,7 @@ static int cmd_rollback(struct dict_connection_cmd *cmd, const char *line)
 		return -1;
 
 	dict_transaction_rollback(&trans->ctx);
-	dict_connection_transaction_array_remove(cmd->conn, trans);
+	dict_connection_transaction_array_remove(cmd->conn, trans->id);
 	return 0;
 }
 
@@ -451,6 +450,7 @@ int dict_command_input(struct dict_connection *conn, const char *line)
 	cmd->conn = conn;
 	cmd->cmd = cmd_func;
 	array_append(&conn->cmds, &cmd, 1);
+	dict_connection_ref(conn);
 	if ((ret = cmd_func->func(cmd, line + 1)) <= 0) {
 		dict_connection_cmd_remove(cmd);
 		return ret;
diff --git a/src/dict/dict-commands.h b/src/dict/dict-commands.h
index 6fe32b2de0..03214c4e52 100644
--- a/src/dict/dict-commands.h
+++ b/src/dict/dict-commands.h
@@ -6,6 +6,5 @@ struct dict_connection;
 int dict_command_input(struct dict_connection *conn, const char *line);
 
 void dict_connection_cmds_output_more(struct dict_connection *conn);
-void dict_connection_cmds_free(struct dict_connection *conn);
 
 #endif
diff --git a/src/dict/dict-connection.c b/src/dict/dict-connection.c
index cd13cb643b..b2b3499d3c 100644
--- a/src/dict/dict-connection.c
+++ b/src/dict/dict-connection.c
@@ -157,7 +157,7 @@ static void dict_connection_input(struct dict_connection *conn)
 
 void dict_connection_continue_input(struct dict_connection *conn)
 {
-	if (conn->io != NULL)
+	if (conn->io != NULL || conn->destroyed)
 		return;
 
 	conn->io = io_add(conn->fd, IO_READ, dict_connection_input, conn);
@@ -183,6 +183,7 @@ struct dict_connection *dict_connection_create(int fd)
 	struct dict_connection *conn;
 
 	conn = i_new(struct dict_connection, 1);
+	conn->refcount = 1;
 	conn->fd = fd;
 	conn->input = i_stream_create_fd(fd, DICT_CLIENT_MAX_LINE_LENGTH,
 					 FALSE);
@@ -195,41 +196,73 @@ struct dict_connection *dict_connection_create(int fd)
 	return conn;
 }
 
-void dict_connection_destroy(struct dict_connection *conn)
+void dict_connection_ref(struct dict_connection *conn)
+{
+	i_assert(conn->refcount > 0);
+	conn->refcount++;
+}
+
+bool dict_connection_unref(struct dict_connection *conn)
 {
 	struct dict_connection_transaction *transaction;
 
-	DLLIST_REMOVE(&dict_connections, conn);
+	i_assert(conn->refcount > 0);
+	if (--conn->refcount > 0)
+		return TRUE;
+
+	i_assert(array_count(&conn->cmds) == 0);
+
+	/* we should have only transactions that haven't been committed or
+	   rollbacked yet. close those before dict is deinitialized. */
+	if (array_is_created(&conn->transactions)) {
+		array_foreach_modifiable(&conn->transactions, transaction) {
+			if (transaction->ctx != NULL)
+				dict_transaction_rollback(&transaction->ctx);
+		}
+	}
 
-	/* deinit dict before anything else, so any pending dict operations
-	   are aborted and their callbacks called. */
 	if (conn->dict != NULL)
 		dict_deinit(&conn->dict);
 
-	if (array_is_created(&conn->transactions)) {
-		array_foreach_modifiable(&conn->transactions, transaction)
-			dict_transaction_rollback(&transaction->ctx);
+	if (array_is_created(&conn->transactions))
 		array_free(&conn->transactions);
-	}
 
-	/* this may end up adding conn->io back, so keep this early */
-	dict_connection_cmds_free(conn);
+	i_stream_destroy(&conn->input);
+	o_stream_destroy(&conn->output);
+
 	array_free(&conn->cmds);
+	i_free(conn->name);
+	i_free(conn->username);
+	i_free(conn);
+
+	master_service_client_connection_destroyed(master_service);
+	return FALSE;
+}
+
+void dict_connection_destroy(struct dict_connection *conn)
+{
+	conn->destroyed = TRUE;
+	DLLIST_REMOVE(&dict_connections, conn);
 
 	if (conn->to_input != NULL)
 		timeout_remove(&conn->to_input);
 	if (conn->io != NULL)
 		io_remove(&conn->io);
-	i_stream_destroy(&conn->input);
-	o_stream_destroy(&conn->output);
+	i_stream_close(conn->input);
+	o_stream_close(conn->output);
 	if (close(conn->fd) < 0)
 		i_error("close(dict client) failed: %m");
+	conn->fd = -1;
 
-	i_free(conn->name);
-	i_free(conn->username);
-	i_free(conn);
+	/* the connection is closed, but there may still be commands left
+	   running. finish them, even if the calling client can't be notified
+	   about whether they succeeded (clients may not even care).
 
-	master_service_client_connection_destroyed(master_service);
+	   flush the command output here in case we were waiting on iteration
+	   output. */
+	dict_connection_cmds_output_more(conn);
+
+	dict_connection_unref(conn);
 }
 
 void dict_connections_destroy_all(void)
diff --git a/src/dict/dict-connection.h b/src/dict/dict-connection.h
index 3d437d7e49..7ab6cef45f 100644
--- a/src/dict/dict-connection.h
+++ b/src/dict/dict-connection.h
@@ -12,6 +12,7 @@ struct dict_connection_transaction {
 struct dict_connection {
 	struct dict_connection *prev, *next;
 	struct dict_server *server;
+	int refcount;
 
 	char *username;
 	char *name;
@@ -28,11 +29,16 @@ struct dict_connection {
 	   array is fast enough */
 	ARRAY(struct dict_connection_transaction) transactions;
 	ARRAY(struct dict_connection_cmd *) cmds;
+
+	unsigned int destroyed:1;
 };
 
 struct dict_connection *dict_connection_create(int fd);
 void dict_connection_destroy(struct dict_connection *conn);
 
+void dict_connection_ref(struct dict_connection *conn);
+bool dict_connection_unref(struct dict_connection *conn);
+
 void dict_connection_continue_input(struct dict_connection *conn);
 
 void dict_connections_destroy_all(void);