[Tarantool-patches] [PATCH v2 0/2] fiber: prevent fiber_join from misuse

[Tarantool-patches] [PATCH v2 0/2] fiber: prevent fiber_join from misuse

[Cyrill Gorcunov via Tarantool-patches]

v2 (by Vlad):
 - Use explicit != 0 form for flag testing
 - in Lua's fiber:join() drop the test for FIBER_IS_JOINABLE
   flag since now we do the same on lower level
 - extend tests
 - provide changelog

issue https://github.com/tarantool/tarantool/issues/6046
branch gorcunov/gh-6046-fiber-join-2

Cyrill Gorcunov (2):
  fiber: fiber_join -- drop redundat variable
  fiber: fiber_join -- don't crash on misuse

 .../unreleased/gh-6046-fiber-join-misuse.md   |  6 ++++
 src/lib/core/fiber.c                          | 12 +++++--
 src/lua/fiber.c                               | 10 ++++--
 test/app/fiber.result                         | 34 +++++++++++++++++++
 test/app/fiber.test.lua                       | 12 +++++++
 test/unit/fiber.cc                            |  7 ++++
 6 files changed, 75 insertions(+), 6 deletions(-)
 create mode 100644 changelogs/unreleased/gh-6046-fiber-join-misuse.md


base-commit: f44663ed75769a20095ff09f728e9b29f9319dc5
-- 
2.30.2

[Tarantool-patches] [PATCH v2 1/2] fiber: fiber_join -- drop redundat variable

[Cyrill Gorcunov via Tarantool-patches]

No need for additional variable here.

In-scope-of #6046

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lib/core/fiber.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index cb6295171..a4b60e864 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -614,8 +614,7 @@ fiber_reschedule(void)
 int
 fiber_join(struct fiber *fiber)
 {
-	int rc = fiber_join_timeout(fiber, TIMEOUT_INFINITY);
-	return rc;
+	return fiber_join_timeout(fiber, TIMEOUT_INFINITY);
 }
 
 int
-- 
2.30.2

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

[Cyrill Gorcunov via Tarantool-patches]

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@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

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

[Serge Petrenko via Tarantool-patches]

Hi! Thanks for the patch!
Please find 2 comments below.


28.04.2021 13:22, Cyrill Gorcunov пишет:
> In case if we call jiber_join() over the non joinable fiber

1. typo: fiber_join.
> 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@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);

2. ch1:get() should be enough instead of the while loop.

> +---
> +...
> +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);

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH v2 1/2] fiber: fiber_join -- drop redundat variable

[Serge Petrenko via Tarantool-patches]

28.04.2021 13:22, Cyrill Gorcunov пишет:
> No need for additional variable here.
>
> In-scope-of #6046
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/lib/core/fiber.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
> index cb6295171..a4b60e864 100644
> --- a/src/lib/core/fiber.c
> +++ b/src/lib/core/fiber.c
> @@ -614,8 +614,7 @@ fiber_reschedule(void)
>   int
>   fiber_join(struct fiber *fiber)
>   {
> -	int rc = fiber_join_timeout(fiber, TIMEOUT_INFINITY);
> -	return rc;
> +	return fiber_join_timeout(fiber, TIMEOUT_INFINITY);
>   }
>   
>   int
Thanks! LGTM

-- 
Serge Petrenko

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

[Cyrill Gorcunov via Tarantool-patches]

On Wed, Apr 28, 2021 at 06:13:30PM +0300, Serge Petrenko wrote:
> Hi! Thanks for the patch!
> Please find 2 comments below.

Thanks! I've force pushed an update.
---
 test/app/fiber.result   | 2 +-
 test/app/fiber.test.lua | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/app/fiber.result b/test/app/fiber.result
index d3a245428..b145302aa 100644
--- a/test/app/fiber.result
+++ b/test/app/fiber.result
@@ -1154,7 +1154,7 @@ end, 10);
 ch1 = fiber.channel(1);
 ---
 ...
-f = fiber.new(function() while ch1:is_empty() do fiber.sleep(0) end end);
+f = fiber.new(function() ch1:get() end);
 ---
 ...
 f:set_joinable(false);
diff --git a/test/app/fiber.test.lua b/test/app/fiber.test.lua
index 430e4ae96..9f25f7f9c 100644
--- a/test/app/fiber.test.lua
+++ b/test/app/fiber.test.lua
@@ -488,7 +488,7 @@ 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 = fiber.new(function() ch1:get() end);
 f:set_joinable(false);
 _, errmsg = pcall(f.join, f);
 errmsg;
-- 
2.30.2

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

[Serge Petrenko via Tarantool-patches]

28.04.2021 18:21, Cyrill Gorcunov пишет:
> On Wed, Apr 28, 2021 at 06:13:30PM +0300, Serge Petrenko wrote:
>> Hi! Thanks for the patch!
>> Please find 2 comments below.
> Thanks! I've force pushed an update.
> ---
>   test/app/fiber.result   | 2 +-
>   test/app/fiber.test.lua | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/test/app/fiber.result b/test/app/fiber.result
> index d3a245428..b145302aa 100644
> --- a/test/app/fiber.result
> +++ b/test/app/fiber.result
> @@ -1154,7 +1154,7 @@ end, 10);
>   ch1 = fiber.channel(1);
>   ---
>   ...
> -f = fiber.new(function() while ch1:is_empty() do fiber.sleep(0) end end);
> +f = fiber.new(function() ch1:get() end);
>   ---
>   ...
>   f:set_joinable(false);
> diff --git a/test/app/fiber.test.lua b/test/app/fiber.test.lua
> index 430e4ae96..9f25f7f9c 100644
> --- a/test/app/fiber.test.lua
> +++ b/test/app/fiber.test.lua
> @@ -488,7 +488,7 @@ 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 = fiber.new(function() ch1:get() end);
>   f:set_joinable(false);
>   _, errmsg = pcall(f.join, f);
>   errmsg;
Thanks! LGTM.

-- 
Serge Petrenko

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

[Vladislav Shpilevoy via Tarantool-patches]

> 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
> @@ -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);

After looking at this hunk I realized that it might be wrong to
allow to call join on a non-joinable fiber. Firstly, you have no
way to check why did join return -1: because it wasn't joinable
or because this is what the fiber's function has returned. It is
simply impossible in the public API (module.h).

Secondly, fiber_join() is documented to always return the
fiber's function result. I see it in module.h and on the site.
Here the behaviour has kind of changed - it might return something
even if the fiber didn't really end. This is especially bad if the
fiber was using some resources which are freed right after the join.
And doubly-bad if the user's function never fails, so fiber_join()
result wasn't even checked in his code.

Thirdly, this leads to inconsistent behaviour. In this example
fiber.join does not raise an error - it returns false + error:

	fiber = require('fiber')
	do
	    f = fiber.new(function() box.error('other error') end)
	    f:set_joinable(true)
	end

	tarantool> f:join()
	---
	- false
	- '[string "do..."]:2: box.error(): bad arguments'
	...

But when I change the error type, it raises the error:

	fiber = require('fiber')
	do
	    f = fiber.new(function() box.lib.load() end)
	    f:set_joinable(true)
	end

	tarantool> f:join()
	---
	- error: Expects box.lib.load('name') but no name passed
	...

It didn't happen before your patch. The same problem exists for
fiber_join_timeout(), but at least it is documented to be able to
return before the fiber has joined.

With that said, I think we must call panic() on an attempt to join
a non-joinable fiber.

For easier usage we might need to introduce fiber_join_ex(), which
wouldn't mix its own fail and the fiber's function fail. But maybe
not now since nobody really asked for that.

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

[Cyrill Gorcunov via Tarantool-patches]

On Wed, Apr 28, 2021 at 11:16:20PM +0200, Vladislav Shpilevoy wrote:
> > 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
> > @@ -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);
> 
> After looking at this hunk I realized that it might be wrong to
> allow to call join on a non-joinable fiber. Firstly, you have no
> way to check why did join return -1: because it wasn't joinable
> or because this is what the fiber's function has returned. It is
> simply impossible in the public API (module.h).

Not really. The error message returned actually a part of api. Before
the patch if we call join() on non joinable fiber we have an error
thrown with "the fiber is not joinable", right? So we keep it here
intact. So in Lua interface we keep everything as it was (if only
I'm not missing something obvious, sorry brain is almost off
already).

Now to the C level interface. When we call it with non joinable fiber
without the patch it leads to some unpredictable results (for now,
since in future we might reuse fiber_id = 0 or even something more
wider) and a crash in *best* case. In worst case it is completely
undeterminable behaviour and exiting with error (or even crash
some user application if it is not ready for such change) is a
way better than silent potential data corruption in memory.

As to reason for exit -- we have two error codes here and diag
last error will show it. So that user's module can fetch and
figure out the reason.

Or you mean something else? I'll reread your email tomorrow,
as I said I might be brain off already :(

side note: i figured out what you mean at the end of email...

> Secondly, fiber_join() is documented to always return the
> fiber's function result. I see it in module.h and on the site.
> Here the behaviour has kind of changed - it might return something
> even if the fiber didn't really end. This is especially bad if the
> fiber was using some resources which are freed right after the join.
> And doubly-bad if the user's function never fails, so fiber_join()
> result wasn't even checked in his code.

I see what you mean and this is api change indeed, but because of
this nasty error we've to break the api I think (simply because not
breaking api is a way worse). Obviously the C level of fibers API
either not much used or all users are sane and do the test for
joinability otherwise we would had a number of reports I think.

> Thirdly, this leads to inconsistent behaviour. In this example
> fiber.join does not raise an error - it returns false + error:
> 
> 	fiber = require('fiber')
> 	do
> 	    f = fiber.new(function() box.error('other error') end)
> 	    f:set_joinable(true)
> 	end
> 
> 	tarantool> f:join()
> 	---
> 	- false
> 	- '[string "do..."]:2: box.error(): bad arguments'
> 	...
> 
> But when I change the error type, it raises the error:
> 
> 	fiber = require('fiber')
> 	do
> 	    f = fiber.new(function() box.lib.load() end)
> 	    f:set_joinable(true)
> 	end
> 
> 	tarantool> f:join()
> 	---
> 	- error: Expects box.lib.load('name') but no name passed
> 	...
> 
> It didn't happen before your patch. The same problem exists for
> fiber_join_timeout(), but at least it is documented to be able to
> return before the fiber has joined.

So this comes from error type collision where before the patch we
didn't have. And that is really a show stopper, agreed.

> With that said, I think we must call panic() on an attempt to join
> a non-joinable fiber.

Another option is to enter endless loop with error message printed out?
Say

	if ((fiber->flags & FIBER_IS_JOINABLE) == 0) {
		say_crit("Attempt to join nonjoinable fiber");
		say_crit("Enters endless loop");
		while (!(fiber->flags & FIBER_IS_JOINABLE))
			fiber_yield();
	}

or something similar. At least a user will be able to do something? Don't
get me wrong but panic is definitely the last big hammer kick when
tarantool simply cant continue working.

> For easier usage we might need to introduce fiber_join_ex(), which
> wouldn't mix its own fail and the fiber's function fail. But maybe
> not now since nobody really asked for that.

	Cyrill

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

[Cyrill Gorcunov via Tarantool-patches]

In case if we call fiber_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.

Since we're not allowed to break API on C level lets just
panic early in case of such misuse, it is a way better than
continue operating with potentially screwed data in memory.

Fixes #6046

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
issue https://github.com/tarantool/tarantool/issues/6046
branch gorcunov/gh-6046-fiber-join-3

 changelogs/unreleased/gh-6046-fiber-join-misuse.md | 6 ++++++
 src/lib/core/fiber.c                               | 3 ++-
 2 files changed, 8 insertions(+), 1 deletion(-)
 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..196dffe26 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -620,7 +620,8 @@ 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)
+		panic("the fiber is not joinable");
 
 	if (! fiber_is_dead(fiber)) {
 		bool exceeded = false;
-- 
2.30.2

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

[Vladislav Shpilevoy via Tarantool-patches]

Hi! Thanks for the patch!

> 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

fiber::join() -> fiber:join()

And `` -> ` in the end.

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

[Cyrill Gorcunov via Tarantool-patches]

On Thu, Apr 29, 2021 at 09:37:56PM +0200, Vladislav Shpilevoy wrote:
> > +* 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
> 
> fiber::join() -> fiber:join()
> 
> And `` -> ` in the end.

Force pushed an update. Thanks!
branch gorcunov/gh-6046-fiber-join-3
---
 changelogs/unreleased/gh-6046-fiber-join-misuse.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/changelogs/unreleased/gh-6046-fiber-join-misuse.md b/changelogs/unreleased/gh-6046-fiber-join-misuse.md
index 32c15566d..4515d96a8 100644
--- a/changelogs/unreleased/gh-6046-fiber-join-misuse.md
+++ b/changelogs/unreleased/gh-6046-fiber-join-misuse.md
@@ -2,5 +2,5 @@
 
 * 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
+  level only, in Lua interface `fiber:join()` the protection is
   turned on already.
-- 
2.30.2

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

[Vladislav Shpilevoy via Tarantool-patches]

Thanks for the patch!

LGTM.

> issue https://github.com/tarantool/tarantool/issues/6046
> branch gorcunov/gh-6046-fiber-join-3

Kirill, use join-3, not join-2.

Re: [Tarantool-patches] [PATCH v2 0/2] fiber: prevent fiber_join from misuse

[Kirill Yukhin via Tarantool-patches]

Hello,

On 28 апр 13:22, Cyrill Gorcunov wrote:
> v2 (by Vlad):
>  - Use explicit != 0 form for flag testing
>  - in Lua's fiber:join() drop the test for FIBER_IS_JOINABLE
>    flag since now we do the same on lower level
>  - extend tests
>  - provide changelog
> 
> issue https://github.com/tarantool/tarantool/issues/6046
> branch gorcunov/gh-6046-fiber-join-2
> 
> Cyrill Gorcunov (2):
>   fiber: fiber_join -- drop redundat variable
>   fiber: fiber_join -- don't crash on misuse
> 
>  .../unreleased/gh-6046-fiber-join-misuse.md   |  6 ++++
>  src/lib/core/fiber.c                          | 12 +++++--
>  src/lua/fiber.c                               | 10 ++++--
>  test/app/fiber.result                         | 34 +++++++++++++++++++
>  test/app/fiber.test.lua                       | 12 +++++++
>  test/unit/fiber.cc                            |  7 ++++
>  6 files changed, 75 insertions(+), 6 deletions(-)
>  create mode 100644 changelogs/unreleased/gh-6046-fiber-join-misuse.md

I've checked your patchset into 2.8 and master.
ALso, cherry-picked top patch to 2.7.

--
Regards, Kirill Yukhin