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

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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