[tarantool-patches] [PATCH 2/2] session: fix box.session.sync()

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Before the patch box.session.sync() is global for the session and
is updated on each new iproto request. When the connection is
multiplexed, box.session.sync() can be changed with no finishing
a current request, if a new one arrives.

The patch makes box.session.push() local for the request,
protecting it from multiplexing mess. Box.session.sync() after
the patch can be safely used inside a request.

Closes #3450

@TarantoolBot document
Title: box.session.sync() became request local
Box.session.sync() was global for a session, so it was unusable
when the connection behind the session is multiplexed. Now
box.session.sync() is request local and can be safely used inside
the request processor.
---
 src/box/iproto.cc             | 11 +++++++----
 src/box/session.h             | 17 ++++++-----------
 src/fiber.h                   | 27 ++++++++++++++++++---------
 test/box-tap/session.test.lua | 29 ++++++++++++++++++++++++++++-
 4 files changed, 59 insertions(+), 25 deletions(-)

diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 0102aec4b..759945278 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1171,7 +1171,8 @@ error:
 static void
 tx_fiber_init(struct session *session, uint64_t sync)
 {
-	session->meta.sync = sync;
+	struct fiber *f = fiber();
+	f->storage.net.sync = sync;
 	/*
 	 * We do not cleanup fiber keys at the end of each request.
 	 * This does not lead to privilege escalation as long as
@@ -1182,8 +1183,8 @@ tx_fiber_init(struct session *session, uint64_t sync)
 	 * background tasks clean up their session in on_stop
 	 * trigger as well.
 	 */
-	fiber_set_session(fiber(), session);
-	fiber_set_user(fiber(), &session->credentials);
+	fiber_set_session(f, session);
+	fiber_set_user(f, &session->credentials);
 }
 
 /**
@@ -1818,7 +1819,9 @@ iproto_session_fd(struct session *session)
 int64_t
 iproto_session_sync(struct session *session)
 {
-	return session->meta.sync;
+	(void) session;
+	assert(session == fiber()->storage.session);
+	return fiber()->storage.net.sync;
 }
 
 /** {{{ IPROTO_PUSH implementation. */
diff --git a/src/box/session.h b/src/box/session.h
index 17190ab47..6ed97dc46 100644
--- a/src/box/session.h
+++ b/src/box/session.h
@@ -66,16 +66,11 @@ extern const char *session_type_strs[];
  * types, and allows to do not store attributes in struct session,
  * that are used only by a session of particular type.
  */
-struct session_meta {
-	union {
-		/** IProto connection meta. */
-		struct {
-			uint64_t sync;
-			void *connection;
-		};
-		/** Only by console is used. */
-		int fd;
-	};
+union session_meta {
+	/** IProto connection. */
+	void *connection;
+	/** Console file/socket descriptor. */
+	int fd;
 };
 
 /**
@@ -92,7 +87,7 @@ struct session {
 	uint64_t id;
 	enum session_type type;
 	/** Session metadata. */
-	struct session_meta meta;
+	union session_meta meta;
 	/** Session user id and global grants */
 	struct credentials credentials;
 	/** Trigger for fiber on_stop to cleanup created on-demand session */
diff --git a/src/fiber.h b/src/fiber.h
index b7003aeb6..021929822 100644
--- a/src/fiber.h
+++ b/src/fiber.h
@@ -393,15 +393,24 @@ struct fiber {
 		struct session *session;
 		struct credentials *credentials;
 		struct txn *txn;
-		/**
-		 * Fields used by a fiber created in Lua: Lua
-		 * stack and the optional fiber.storage Lua
-		 * reference.
-		 */
-		struct {
-			struct lua_State *stack;
-			int ref;
-		} lua;
+		union {
+			/**
+			 * Fields used by a fiber created in Lua:
+			 * Lua stack and the optional
+			 * fiber.storage Lua reference.
+			 */
+			struct {
+				struct lua_State *stack;
+				int ref;
+			} lua;
+			/**
+			 * Fields used by a fiber created to
+			 * process an iproto request.
+			 */
+			struct {
+				uint64_t sync;
+			} net;
+		};
 	} storage;
 	/** An object to wait for incoming message or a reader. */
 	struct ipc_wait_pad *wait_pad;
diff --git a/test/box-tap/session.test.lua b/test/box-tap/session.test.lua
index 6fddced3c..c3c07a67c 100755
--- a/test/box-tap/session.test.lua
+++ b/test/box-tap/session.test.lua
@@ -15,7 +15,7 @@ session = box.session
 space = box.schema.space.create('tweedledum')
 index = space:create_index('primary', { type = 'hash' })
 
-test:plan(53)
+test:plan(55)
 
 ---
 --- Check that Tarantool creates ADMIN session for #! script
@@ -183,6 +183,33 @@ local sp = conn:eval("return box.space._space.index.name:get{\"sp1\"}[2]")
 test:is(sp, 1, "effective ddl owner")
 conn:close()
 
+--
+-- gh-3450: box.session.sync() becomes request local.
+--
+cond = fiber.cond()
+local sync1, sync2
+local started = 0
+function f1()
+	started = started + 1
+	cond:wait()
+	sync1 = box.session.sync()
+end
+function f2()
+	started = started + 1
+	sync2 = box.session.sync()
+	cond:signal()
+end
+box.schema.user.grant('guest', 'read,write,execute', 'universe')
+conn = net.box.connect(box.cfg.listen)
+test:ok(conn:ping(), 'connect to self')
+_ = fiber.create(function() conn:call('f1') end)
+while started ~= 1 do fiber.sleep(0.01) end
+_ = fiber.create(function() conn:call('f2') end)
+while started ~= 2 do fiber.sleep(0.01) end
+test:isnt(sync1, sync2, 'session.sync() is request local')
+conn:close()
+box.schema.user.revoke('guest', 'read,write,execute', 'universe')
+
 inspector:cmd('stop server session with cleanup=1')
 session = nil
 os.exit(test:check() == true and 0 or -1)
-- 
2.15.1 (Apple Git-101)