From: Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: tml <tarantool-patches@dev.tarantool.org> Cc: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Subject: [Tarantool-patches] [PATCH v2 2/2] fiber: fiber_join -- don't crash on misuse Date: Wed, 28 Apr 2021 13:22:51 +0300 [thread overview] Message-ID: <20210428102251.552976-3-gorcunov@gmail.com> (raw) In-Reply-To: <20210428102251.552976-1-gorcunov@gmail.com> 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
next prev parent reply other threads:[~2021-04-28 10:23 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-28 10:22 [Tarantool-patches] [PATCH v2 0/2] fiber: prevent fiber_join from misuse Cyrill Gorcunov via Tarantool-patches 2021-04-28 10:22 ` [Tarantool-patches] [PATCH v2 1/2] fiber: fiber_join -- drop redundat variable Cyrill Gorcunov via Tarantool-patches 2021-04-28 15:13 ` Serge Petrenko via Tarantool-patches 2021-04-28 10:22 ` Cyrill Gorcunov via Tarantool-patches [this message] 2021-04-28 15:13 ` [Tarantool-patches] [PATCH v2 2/2] fiber: fiber_join -- don't crash on misuse Serge Petrenko via Tarantool-patches 2021-04-28 15:21 ` Cyrill Gorcunov via Tarantool-patches 2021-04-28 15:34 ` Serge Petrenko via Tarantool-patches 2021-04-28 21:16 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-28 22:12 ` Cyrill Gorcunov via Tarantool-patches 2021-04-29 11:10 ` [Tarantool-patches] [PATCH v3 " Cyrill Gorcunov via Tarantool-patches 2021-04-29 19:37 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-29 20:39 ` Cyrill Gorcunov via Tarantool-patches 2021-04-29 21:10 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-30 8:13 ` [Tarantool-patches] [PATCH v2 0/2] fiber: prevent fiber_join from misuse Kirill Yukhin via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210428102251.552976-3-gorcunov@gmail.com \ --to=tarantool-patches@dev.tarantool.org \ --cc=gorcunov@gmail.com \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v2 2/2] fiber: fiber_join -- don'\''t crash on misuse' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox