issue https://github.com/tarantool/tarantool/issues/6046 branch gorcunov/gh-6046-fiber-join Cyrill Gorcunov (2): fiber: fiber_join -- drop redundat variable fiber: fiber_join -- don't crash on misuse src/lib/core/fiber.c | 8 +++++--- test/unit/fiber.cc | 3 +++ 2 files changed, 8 insertions(+), 3 deletions(-) base-commit: f998ea39e96d93113823d92727a1faf9860c8ea6 -- 2.30.2
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 f8b85d99d..baf78a130 100644 --- a/src/lib/core/fiber.c +++ b/src/lib/core/fiber.c @@ -609,8 +609,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
In case if we call jiber_join() over the nonjoinable 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. Since nobody complained about this bug yet I think such combination is not use commonly in external C modules and we can change the API behaviour. Fixes #6046 Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com> --- src/lib/core/fiber.c | 5 ++++- test/unit/fiber.cc | 3 +++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c index baf78a130..dd7498dd7 100644 --- a/src/lib/core/fiber.c +++ b/src/lib/core/fiber.c @@ -615,7 +615,10 @@ 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)) { + diag_set(IllegalParams, "the fiber is not joinable"); + return -1; + } if (! fiber_is_dead(fiber)) { bool exceeded = false; diff --git a/test/unit/fiber.cc b/test/unit/fiber.cc index 9c1a23bdd..fbdd82772 100644 --- a/test/unit/fiber.cc +++ b/test/unit/fiber.cc @@ -96,6 +96,9 @@ fiber_join_test() header(); struct fiber *fiber = fiber_new_xc("join", noop_f); + /* gh-6046: crash on attempt to join non joinable */ + fiber_set_joinable(fiber, false); + fiber_join(fiber); fiber_set_joinable(fiber, true); fiber_wakeup(fiber); fiber_join(fiber); -- 2.30.2
Hi! Thanks for the patch! > diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c > index baf78a130..dd7498dd7 100644 > --- a/src/lib/core/fiber.c > +++ b/src/lib/core/fiber.c > @@ -615,7 +615,10 @@ 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)) { 1. https://github.com/tarantool/tarantool/wiki/Code-review-procedure#code-style In C we don't apply ! operator to non-boolean values. It means, to check if an integer is not 0, you use != 0. To check if a pointer is not NULL, you use != NULL. The same for ==; > + diag_set(IllegalParams, "the fiber is not joinable"); > + return -1; > + } > > if (! fiber_is_dead(fiber)) { > bool exceeded = false; > diff --git a/test/unit/fiber.cc b/test/unit/fiber.cc > index 9c1a23bdd..fbdd82772 100644 > --- a/test/unit/fiber.cc > +++ b/test/unit/fiber.cc > @@ -96,6 +96,9 @@ fiber_join_test() > header(); > > struct fiber *fiber = fiber_new_xc("join", noop_f); > + /* gh-6046: crash on attempt to join non joinable */ > + fiber_set_joinable(fiber, false); > + fiber_join(fiber); 2. Would be good to test that it returns -1, and that the diag is not empty.
Also you can remove if (!(fiber->flags & FIBER_IS_JOINABLE)) from lua/fiber.c and check result of fiber_join() so as not to touch fiber internals there.
On Mon, Apr 26, 2021 at 10:58:16PM +0200, Vladislav Shpilevoy wrote: > Hi! Thanks for the patch! > > > diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c > > index baf78a130..dd7498dd7 100644 > > --- a/src/lib/core/fiber.c > > +++ b/src/lib/core/fiber.c > > @@ -615,7 +615,10 @@ 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)) { > > 1. https://github.com/tarantool/tarantool/wiki/Code-review-procedure#code-style > > In C we don't apply ! operator to non-boolean values. It means, to > check if an integer is not 0, you use != 0. To check if a pointer is > not NULL, you use != NULL. The same for ==; I know. I left it this way on a purpose -- the whole file follows more sane use of negation operator. I thought better to keep the old form, otherwise we will have a mixture of old a new style in one file. > > struct fiber *fiber = fiber_new_xc("join", noop_f); > > + /* gh-6046: crash on attempt to join non joinable */ > > + fiber_set_joinable(fiber, false); > > + fiber_join(fiber); > > 2. Would be good to test that it returns -1, and that the diag is not > empty. Will update, thanks!