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

[Tarantool-patches] [PATCH 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.

TX thread solution (which is chosen in the patch): 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 solution is simple, but adds new members to iproto_connection
struct, and requires lots of commenting.

Iproto thread solution (alternative): just don't send destroy
request until disconnect returns back to iproto thread.

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

There is a second version of a fix, which I like more. But anyway
decided to send this one too.

 src/box/iproto.cc                             | 32 ++++++++++
 .../box/gh-4627-session-use-after-free.result | 60 +++++++++++++++++++
 .../gh-4627-session-use-after-free.test.lua   | 35 +++++++++++
 3 files changed, 127 insertions(+)
 create mode 100644 test/box/gh-4627-session-use-after-free.result
 create mode 100644 test/box/gh-4627-session-use-after-free.test.lua

diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 34c8f469a..58ea120a0 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -479,6 +479,23 @@ struct iproto_connection
 		 * return.
 		 */
 		bool is_push_pending;
+		/**
+		 * True if the connection disconnect request was
+		 * fully handled by TX thread. This is used to
+		 * prevent destroy of the connection and its
+		 * session until disconnect is processed. The
+		 * problem is that disconnect triggers of a
+		 * session may yield, and during this a destroy
+		 * request may arrive from iproto.
+		 */
+		bool is_disconnected;
+		/**
+		 * The condition is signaled when disconnect
+		 * request is done. A destroy request may wait for
+		 * that in case it arrived before disconnect is
+		 * done.
+		 */
+		struct fiber_cond cond_disconnected;
 	} tx;
 	/** Authentication salt. */
 	char salt[IPROTO_SALT_SIZE];
@@ -1051,6 +1068,8 @@ iproto_connection_new(int fd)
 	con->is_destroy_sent = false;
 	con->tx.is_push_pending = false;
 	con->tx.is_push_sent = false;
+	con->tx.is_disconnected = false;
+	fiber_cond_create(&con->tx.cond_disconnected);
 	rmean_collect(rmean_net, IPROTO_CONNECTIONS, 1);
 	return con;
 }
@@ -1073,6 +1092,7 @@ iproto_connection_delete(struct iproto_connection *con)
 	       con->obuf[0].iov[0].iov_base == NULL);
 	assert(con->obuf[1].pos == 0 &&
 	       con->obuf[1].iov[0].iov_base == NULL);
+	fiber_cond_destroy(&con->tx.cond_disconnected);
 	mempool_free(&iproto_connection_pool, con);
 }
 
@@ -1278,6 +1298,8 @@ tx_process_disconnect(struct cmsg *m)
 			tx_fiber_init(con->session, 0);
 			session_run_on_disconnect_triggers(con->session);
 		}
+		con->tx.is_disconnected = true;
+		fiber_cond_signal(&con->tx.cond_disconnected);
 	}
 }
 
@@ -1291,6 +1313,16 @@ tx_process_destroy(struct cmsg *m)
 	struct iproto_connection *con =
 		container_of(m, struct iproto_connection, destroy_msg);
 	if (con->session) {
+		/*
+		 * Disconnect still may be not finished, if
+		 * on disconnect triggers yield. In that case the
+		 * destroy request may arrive too early.
+		 */
+		if (! con->tx.is_disconnected)
+		{
+			fiber_cond_wait(&con->tx.cond_disconnected);
+			assert(con->tx.is_disconnected);
+		}
 		session_destroy(con->session);
 		con->session = NULL; /* safety */
 	}
diff --git a/test/box/gh-4627-session-use-after-free.result b/test/box/gh-4627-session-use-after-free.result
new file mode 100644
index 000000000..5e5c154b9
--- /dev/null
+++ b/test/box/gh-4627-session-use-after-free.result
@@ -0,0 +1,60 @@
+-- test-run result file version 2
+--
+-- gh-4627: 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.
+--
+net_box = require('net.box')
+ | ---
+ | ...
+fiber = require('fiber')
+ | ---
+ | ...
+
+sid_before_yield = nil
+ | ---
+ | ...
+sid_after_yield = nil
+ | ---
+ | ...
+func = box.session.on_disconnect(function()     \
+    sid_before_yield = box.session.id()         \
+    fiber.yield()                               \
+    sid_after_yield = box.session.id()          \
+end)
+ | ---
+ | ...
+
+connection = net_box.connect(box.cfg.listen)
+ | ---
+ | ...
+connection:ping()
+ | ---
+ | - true
+ | ...
+connection:close()
+ | ---
+ | ...
+
+while not sid_after_yield do fiber.yield() end
+ | ---
+ | ...
+
+sid_after_yield == sid_before_yield and sid_after_yield ~= 0 or \
+    {sid_after_yield, sid_before_yield}
+ | ---
+ | - true
+ | ...
+
+box.session.on_disconnect(nil, func)
+ | ---
+ | ...
diff --git a/test/box/gh-4627-session-use-after-free.test.lua b/test/box/gh-4627-session-use-after-free.test.lua
new file mode 100644
index 000000000..70624a96a
--- /dev/null
+++ b/test/box/gh-4627-session-use-after-free.test.lua
@@ -0,0 +1,35 @@
+--
+-- gh-4627: 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.
+--
+net_box = require('net.box')
+fiber = require('fiber')
+
+sid_before_yield = nil
+sid_after_yield = nil
+func = box.session.on_disconnect(function()     \
+    sid_before_yield = box.session.id()         \
+    fiber.yield()                               \
+    sid_after_yield = box.session.id()          \
+end)
+
+connection = net_box.connect(box.cfg.listen)
+connection:ping()
+connection:close()
+
+while not sid_after_yield do fiber.yield() end
+
+sid_after_yield == sid_before_yield and sid_after_yield ~= 0 or \
+    {sid_after_yield, sid_before_yield}
+
+box.session.on_disconnect(nil, func)
-- 
2.21.0 (Apple Git-122.2)

Re: [Tarantool-patches] [PATCH 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:
> 
>     - 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.
> 
> TX thread solution (which is chosen in the patch): 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 solution is simple, but adds new members to iproto_connection
> struct, and requires lots of commenting.
> 
> Iproto thread solution (alternative): just don't send destroy
> request until disconnect returns back to iproto thread.

I like this one more to be honest. 

-- 
Konstantin Osipov, Moscow, Russia

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

[Vladislav Shpilevoy]

On 16/11/2019 12:54, 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:
>>
>>     - 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.
>>
>> TX thread solution (which is chosen in the patch): 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 solution is simple, but adds new members to iproto_connection
>> struct, and requires lots of commenting.
>>
>> Iproto thread solution (alternative): just don't send destroy
>> request until disconnect returns back to iproto thread.
> 
> I like this one more to be honest. 
> 

Me too. Then look at v2 thread.