[Tarantool-patches] [PATCH v2 1/1] iproto: don't destroy a session during disconnect

[Tarantool-patches] [PATCH v2 1/1] iproto: don't destroy a session during disconnect

[Vladislav Shpilevoy]

Binary session disconnect trigger yield could lead to use after
free of the session object. That happened because iproto thread
sent two requests to TX thread at disconnect:

    - Close the session and run its on disconnect triggers;

    - If all requests are handled, destroy the session.

When a connection is idle, all requests are handled, so both these
requests are sent. If the first one yielded in TX thread, the
second one arrived and destroyed the session right under the feet
of the first one.

This can be solved in two ways - in TX thread, and in iproto
thread.

Iproto thread solution (which is chosen in the patch): just don't
send destroy request until disconnect returns back to iproto
thread.

TX thread solution (alternative): add a flag which says whether
disconnect is processed by TX. When destroy request arrives, it
checks the flag. If disconnect is not done, the destroy request
waits on a condition variable until it is.

The iproto is a bit tricker to implement, but it looks more
correct.

Closes #4627
---
Branch: https://github.com/tarantool/tarantool/tree/gerold103/gh-4627-session-use-after-free-v2
Issue: https://github.com/tarantool/tarantool/issues/4627

Changes in V2:
- Tried to fix from iproto thread side.

 src/box/iproto.cc | 68 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 50 insertions(+), 18 deletions(-)

diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 34c8f469a..fc5bb9026 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -296,8 +296,13 @@ static const struct cmsg_hop destroy_route[] = {
 static void
 tx_process_disconnect(struct cmsg *m);
 
+/** Send destroy message to tx thread. */
+static void
+net_finish_disconnect(struct cmsg *m);
+
 static const struct cmsg_hop disconnect_route[] = {
-	{ tx_process_disconnect, NULL }
+	{ tx_process_disconnect, &net_pipe },
+	{ net_finish_disconnect, NULL }
 };
 
 /**
@@ -432,6 +437,13 @@ struct iproto_connection
 	struct cmsg destroy_msg;
 	/** True if destroy message is sent. Debug-only. */
 	bool is_destroy_sent;
+	/**
+	 * True, if disconnect is done in both iproto and TX
+	 * threads, but destroy is not, because there were
+	 * unfinished requests. Then destroy should be sent on the
+	 * last close, when the last request is done.
+	 */
+	bool do_destroy_on_close;
 	struct rlist in_stop_list;
 	/**
 	 * Kharon is used to implement box.session.push().
@@ -572,6 +584,32 @@ iproto_connection_stop_msg_max_limit(struct iproto_connection *con)
 	rlist_add_tail(&stopped_connections, &con->in_stop_list);
 }
 
+/**
+ * Send a destroy message to TX thread in case all requests are
+ * finished.
+ */
+static inline bool
+iproto_connection_try_to_start_destroy(struct iproto_connection *con)
+{
+	if (!iproto_connection_is_idle(con))
+		return false;
+	/*
+	 * If the connection has no outstanding requests in the
+	 * input buffer, then no one (e.g. tx thread) is referring
+	 * to it, so it must be destroyed. Firstly queue a msg to
+	 * destroy the session and other resources owned by TX
+	 * thread. When it is done, iproto thread will destroy
+	 * other parts of the connection.
+	 *
+	 * Otherwise, it will be destroyed by the last request on
+	 * this connection that has finished processing.
+	 */
+	assert(! con->is_destroy_sent);
+	con->is_destroy_sent = true;
+	cpipe_push(&tx_pipe, &con->destroy_msg);
+	return true;
+}
+
 /**
  * Initiate a connection shutdown. This method may
  * be invoked many times, and does the internal
@@ -597,23 +635,8 @@ iproto_connection_close(struct iproto_connection *con)
 		 */
 		con->p_ibuf->wpos -= con->parse_size;
 		cpipe_push(&tx_pipe, &con->disconnect_msg);
-	}
-	/*
-	 * If the connection has no outstanding requests in the
-	 * input buffer, then no one (e.g. tx thread) is referring
-	 * to it, so it must be destroyed at once. Queue a msg to
-	 * run on_disconnect() trigger and destroy the connection.
-	 *
-	 * Otherwise, it will be destroyed by the last request on
-	 * this connection that has finished processing.
-	 *
-	 * The check is mandatory to not destroy a connection
-	 * twice.
-	 */
-	if (iproto_connection_is_idle(con)) {
-		assert(! con->is_destroy_sent);
-		con->is_destroy_sent = true;
-		cpipe_push(&tx_pipe, &con->destroy_msg);
+	} else if (con->do_destroy_on_close) {
+		iproto_connection_try_to_start_destroy(con);
 	}
 	rlist_del(&con->in_stop_list);
 }
@@ -1049,6 +1072,7 @@ iproto_connection_new(int fd)
 	cmsg_init(&con->destroy_msg, destroy_route);
 	cmsg_init(&con->disconnect_msg, disconnect_route);
 	con->is_destroy_sent = false;
+	con->do_destroy_on_close = false;
 	con->tx.is_push_pending = false;
 	con->tx.is_push_sent = false;
 	rmean_collect(rmean_net, IPROTO_CONNECTIONS, 1);
@@ -1281,6 +1305,14 @@ tx_process_disconnect(struct cmsg *m)
 	}
 }
 
+static void
+net_finish_disconnect(struct cmsg *m)
+{
+	struct iproto_connection *con =
+		container_of(m, struct iproto_connection, disconnect_msg);
+	con->do_destroy_on_close = !iproto_connection_try_to_start_destroy(con);
+}
+
 /**
  * Destroy the session object, as well as output buffers of the
  * connection.
-- 
2.21.0 (Apple Git-122.2)

Re: [Tarantool-patches] [PATCH v2 1/1] iproto: don't destroy a session during disconnect

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/11/16 12:48]:
> Binary session disconnect trigger yield could lead to use after
> free of the session object. That happened because iproto thread
> sent two requests to TX thread at disconnect:

LGTM, except that I usually try to avoid duplicating state, so
do_destroy_on_close looks a bit superfluous, especially since you
have another variable - is_destroy_sent.

>  	/** True if destroy message is sent. Debug-only. */
>  	bool is_destroy_sent;


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH v2 1/1] iproto: don't destroy a session during disconnect

[Vladislav Shpilevoy]

Hi! Thanks for the review!

On 16/11/2019 14:07, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/11/16 12:48]:
>> Binary session disconnect trigger yield could lead to use after
>> free of the session object. That happened because iproto thread
>> sent two requests to TX thread at disconnect:
> 
> LGTM, except that I usually try to avoid duplicating state, so
> do_destroy_on_close looks a bit superfluous, especially since you
> have another variable - is_destroy_sent.

It is not superfluous. Because 'is_destroy_sent' is not enough.

If that flag is false, it does not say anything about whether
disconnect is done. And can't be used in iproto_connection_close()
to decide if it needs to be destroyed now.

If that flag is true, it means, that destroy is already sent, and
it is too late to check for anything. Close will never be called
again.

But I could replace both flags with a 'state':
IPROTO_CON_ALIVE -> IPROTO_CON_CLOSED -> IPROTO_CON_PENDING_DESTROY,
IPROTO_CON_DESTROYED.

Alive is an initial state. Closed is when the socket dies and
disconnect is sent to TX. Pending destroy is an optional state,
equivalent of do_destroy_on_close. Destroyed is an equivalent of
is_destroy_sent.

If you think this is better, I can do that.

In terms of memory both state and flag are free anyway. Because
curently we have 7 byte unused padding between is_destroy_sent
and in_stop_list. A flag or an int (enum) fill this padding.

Re: [Tarantool-patches] [PATCH v2 1/1] iproto: don't destroy a session during disconnect

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/11/17 20:44]:
> But I could replace both flags with a 'state':
> IPROTO_CON_ALIVE -> IPROTO_CON_CLOSED -> IPROTO_CON_PENDING_DESTROY,
> IPROTO_CON_DESTROYED.
> 
> Alive is an initial state. Closed is when the socket dies and
> disconnect is sent to TX. Pending destroy is an optional state,
> equivalent of do_destroy_on_close. Destroyed is an equivalent of
> is_destroy_sent.
> 
> If you think this is better, I can do that.
> 

I think it's better.


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH v2 1/1] iproto: don't destroy a session during disconnect

[Vladislav Shpilevoy]

Ok, will do tomorrow. Kirill, please, don't push anything.

On 17/11/2019 18:49, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/11/17 20:44]:
>> But I could replace both flags with a 'state':
>> IPROTO_CON_ALIVE -> IPROTO_CON_CLOSED -> IPROTO_CON_PENDING_DESTROY,
>> IPROTO_CON_DESTROYED.
>>
>> Alive is an initial state. Closed is when the socket dies and
>> disconnect is sent to TX. Pending destroy is an optional state,
>> equivalent of do_destroy_on_close. Destroyed is an equivalent of
>> is_destroy_sent.
>>
>> If you think this is better, I can do that.
>>
> 
> I think it's better.
> 
>