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

[Konstantin Osipov <kostja.osipov@gmail.com>]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/11/19 00:26]:

lgtm

> 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
> ---
> 
> Changes in v3:
> - Flags are replaced with a state enum.
> 
> Branch: https://github.com/tarantool/tarantool/tree/gerold103/gh-4627-session-use-after-free
> Issue: https://github.com/tarantool/tarantool/issues/4627
> 
>  src/box/iproto.cc | 104 ++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 83 insertions(+), 21 deletions(-)
> 
> diff --git a/src/box/iproto.cc b/src/box/iproto.cc
> index 34c8f469a..c39b8e7bf 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 }
>  };
>  
>  /**
> @@ -327,6 +332,32 @@ static const struct cmsg_hop push_route[] = {
>  
>  /* {{{ iproto_connection - declaration and definition */
>  
> +/** Connection life cycle stages. */
> +enum iproto_connection_state {
> +	/**
> +	 * A connection is always alive in the beginning because
> +	 * takes an already active socket in a constructor.
> +	 */
> +	IPROTO_CONNECTION_ALIVE,
> +	/**
> +	 * Socket was closed, a notification is sent to the TX
> +	 * thread to close the session.
> +	 */
> +	IPROTO_CONNECTION_CLOSED,
> +	/**
> +	 * TX thread was notified about close, but some requests
> +	 * are still not finished. That state may be skipped in
> +	 * case the connection was already idle (not having
> +	 * unfinished requests) at the moment of closing.
> +	 */
> +	IPROTO_CONNECTION_PENDING_DESTROY,
> +	/**
> +	 * All requests are finished, a destroy request is sent to
> +	 * the TX thread.
> +	 */
> +	IPROTO_CONNECTION_DESTROYED,
> +};
> +
>  /**
>   * Context of a single client connection.
>   * Interaction scheme:
> @@ -430,8 +461,12 @@ struct iproto_connection
>  	 * connection.
>  	 */
>  	struct cmsg destroy_msg;
> -	/** True if destroy message is sent. Debug-only. */
> -	bool is_destroy_sent;
> +	/**
> +	 * Connection state. Mainly it is used to determine when
> +	 * the connection can be destroyed, and for debug purposes
> +	 * to assert on a double destroy, for example.
> +	 */
> +	enum iproto_connection_state state;
>  	struct rlist in_stop_list;
>  	/**
>  	 * Kharon is used to implement box.session.push().
> @@ -572,6 +607,35 @@ 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 void
> +iproto_connection_try_to_start_destroy(struct iproto_connection *con)
> +{
> +	assert(con->state == IPROTO_CONNECTION_CLOSED ||
> +	       con->state == IPROTO_CONNECTION_PENDING_DESTROY);
> +	if (!iproto_connection_is_idle(con)) {
> +		/*
> +		 * Not all requests are finished. Let the last
> +		 * finished request destroy the connection.
> +		 */
> +		con->state = IPROTO_CONNECTION_PENDING_DESTROY;
> +		return;
> +	}
> +	/*
> +	 * 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.
> +	 */
> +	con->state = IPROTO_CONNECTION_DESTROYED;
> +	cpipe_push(&tx_pipe, &con->destroy_msg);
> +}
> +
>  /**
>   * Initiate a connection shutdown. This method may
>   * be invoked many times, and does the internal
> @@ -597,23 +661,12 @@ 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);
> +		assert(con->state == IPROTO_CONNECTION_ALIVE);
> +		con->state = IPROTO_CONNECTION_CLOSED;
> +	} else if (con->state == IPROTO_CONNECTION_PENDING_DESTROY) {
> +		iproto_connection_try_to_start_destroy(con);
> +	} else {
> +		assert(con->state == IPROTO_CONNECTION_CLOSED);
>  	}
>  	rlist_del(&con->in_stop_list);
>  }
> @@ -1048,7 +1101,7 @@ iproto_connection_new(int fd)
>  	/* It may be very awkward to allocate at close. */
>  	cmsg_init(&con->destroy_msg, destroy_route);
>  	cmsg_init(&con->disconnect_msg, disconnect_route);
> -	con->is_destroy_sent = false;
> +	con->state = IPROTO_CONNECTION_ALIVE;
>  	con->tx.is_push_pending = false;
>  	con->tx.is_push_sent = false;
>  	rmean_collect(rmean_net, IPROTO_CONNECTIONS, 1);
> @@ -1063,6 +1116,7 @@ iproto_connection_delete(struct iproto_connection *con)
>  	assert(!evio_has_fd(&con->output));
>  	assert(!evio_has_fd(&con->input));
>  	assert(con->session == NULL);
> +	assert(con->state == IPROTO_CONNECTION_DESTROYED);
>  	/*
>  	 * The output buffers must have been deleted
>  	 * in tx thread.
> @@ -1281,6 +1335,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);
> +	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)
> 

-- 
Konstantin Osipov, Moscow, Russia