[Tarantool-patches] [PATCH v2 2/2] fiber: fiber_join -- don't crash on misuse

[Cyrill Gorcunov]

In case if we call jiber_join() over the non joinable fiber
we trigger an assert and crash execution (on debug build).

On release build the asserts will be zapped and won't cause
problems but there is an another one -- the target fiber will
cause double fiber_reset() calls which in result cause to
unregister_fid() with id = 0 (not causing crash but definitely
out of intention) and we will drop stack protection which
might be not ours anymore.

Thus lets return error just like Lua interface does. Same time
this allows us to relax Lua function a bit and leave flags testing
on lower level.

Fixes #6046

Signed-off-by: Cyrill Gorcunov <gorcunov at gmail.com>
---
 .../unreleased/gh-6046-fiber-join-misuse.md   |  6 ++++
 src/lib/core/fiber.c                          |  9 ++++-
 src/lua/fiber.c                               | 10 ++++--
 test/app/fiber.result                         | 34 +++++++++++++++++++
 test/app/fiber.test.lua                       | 12 +++++++
 test/unit/fiber.cc                            |  7 ++++
 6 files changed, 74 insertions(+), 4 deletions(-)
 create mode 100644 changelogs/unreleased/gh-6046-fiber-join-misuse.md

diff --git a/changelogs/unreleased/gh-6046-fiber-join-misuse.md b/changelogs/unreleased/gh-6046-fiber-join-misuse.md
new file mode 100644
index 000000000..32c15566d
--- /dev/null
+++ b/changelogs/unreleased/gh-6046-fiber-join-misuse.md
@@ -0,0 +1,6 @@
+## bugfix/core
+
+* Fixed lack of testing for non noinable fibers in `fiber_join()` call.
+  This could lead to unpredictable results. Note the issue affects C
+  level only, in Lua interface `fiber::join()`` the protection is
+  turned on already.
diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index a4b60e864..c0580b101 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -620,7 +620,14 @@ fiber_join(struct fiber *fiber)
 int
 fiber_join_timeout(struct fiber *fiber, double timeout)
 {
-	assert(fiber->flags & FIBER_IS_JOINABLE);
+	if ((fiber->flags & FIBER_IS_JOINABLE) == 0) {
+		/*
+		 * Don't change the error message, it is
+		 * part of API.
+		 */
+		diag_set(IllegalParams, "the fiber is not joinable");
+		return -1;
+	}
 
 	if (! fiber_is_dead(fiber)) {
 		bool exceeded = false;
diff --git a/src/lua/fiber.c b/src/lua/fiber.c
index 02ec3d158..0c8238cab 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -35,6 +35,8 @@
 #include "backtrace.h"
 #include "tt_static.h"
 
+#include "core/exception.h"
+
 #include <lua.h>
 #include <lauxlib.h>
 #include <lualib.h>
@@ -791,9 +793,11 @@ lbox_fiber_join(struct lua_State *L)
 	int num_ret = 0;
 	int coro_ref = 0;
 
-	if (!(fiber->flags & FIBER_IS_JOINABLE))
-		luaL_error(L, "the fiber is not joinable");
-	fiber_join(fiber);
+	if (fiber_join(fiber) != 0) {
+		e = diag_last_error(&fiber()->diag);
+		if (e->type == &type_IllegalParams)
+			luaL_error(L, e->errmsg);
+	}
 
 	if (child_L != NULL) {
 		coro_ref = lua_tointeger(child_L, -1);
diff --git a/test/app/fiber.result b/test/app/fiber.result
index 14c12b7cc..d3a245428 100644
--- a/test/app/fiber.result
+++ b/test/app/fiber.result
@@ -1150,6 +1150,40 @@ end, 10);
 - true
 - dead
 ...
+-- gh-6046: make sure non joinable fiber fails
+ch1 = fiber.channel(1);
+---
+...
+f = fiber.new(function() while ch1:is_empty() do fiber.sleep(0) end end);
+---
+...
+f:set_joinable(false);
+---
+...
+_, errmsg = pcall(f.join, f);
+---
+...
+errmsg;
+---
+- the fiber is not joinable
+...
+f:set_joinable(true);
+---
+...
+ch1:put(1);
+---
+- true
+...
+ch1:close();
+---
+...
+ch1 = nil;
+---
+...
+f:join();
+---
+- true
+...
 -- test side fiber in transaction
 s = box.schema.space.create("test");
 ---
diff --git a/test/app/fiber.test.lua b/test/app/fiber.test.lua
index a471c83e9..430e4ae96 100644
--- a/test/app/fiber.test.lua
+++ b/test/app/fiber.test.lua
@@ -486,6 +486,18 @@ test_run:wait_cond(function()
     return status == 'dead', status
 end, 10);
 
+-- gh-6046: make sure non joinable fiber fails
+ch1 = fiber.channel(1);
+f = fiber.new(function() while ch1:is_empty() do fiber.sleep(0) end end);
+f:set_joinable(false);
+_, errmsg = pcall(f.join, f);
+errmsg;
+f:set_joinable(true);
+ch1:put(1);
+ch1:close();
+ch1 = nil;
+f:join();
+
 -- test side fiber in transaction
 s = box.schema.space.create("test");
 _ = s:create_index("prim", {parts={1, 'number'}});
diff --git a/test/unit/fiber.cc b/test/unit/fiber.cc
index a1e3ded87..13672dd31 100644
--- a/test/unit/fiber.cc
+++ b/test/unit/fiber.cc
@@ -96,6 +96,13 @@ fiber_join_test()
 	header();
 
 	struct fiber *fiber = fiber_new_xc("join", noop_f);
+	/* gh-6046: crash on attempt to join non joinable */
+	diag_clear(&fiber()->diag);
+	fiber_set_joinable(fiber, false);
+	if (fiber_join(fiber) != -1)
+		fail("fiber_join passed", "");
+	if (diag_last_error(&fiber()->diag) == NULL)
+		fail("fiber_join has last error cleared", "");
 	fiber_set_joinable(fiber, true);
 	fiber_wakeup(fiber);
 	fiber_join(fiber);
-- 
2.30.2