From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 03E236F87A; Wed, 28 Apr 2021 18:13:34 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 03E236F87A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1619622814; bh=riRQ2caqEhy+7Eo2K3ClP4UtfRTcVLAPMpxb7gk2Wgk=; h=To:References:Date:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=xqS7ytd13FtV3399nQX2+HhSY/P88qIEE4GDKQHgzDS01ablE2r5nvL86eEM0EOlW W8u50Kr29f8ZO4eYxUHKleNBvNGhl54VKZjPq0ei+3OabB2LkgBXzUvWQPxAPpp0oH c2hctRQxV/eOHU1r1womKli3wLWgmjDP0MmOVbIc= Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 2966D6F87A for ; Wed, 28 Apr 2021 18:13:32 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2966D6F87A Received: by smtp63.i.mail.ru with esmtpa (envelope-from ) id 1lblsY-0008GD-SG; Wed, 28 Apr 2021 18:13:31 +0300 To: Cyrill Gorcunov , tml References: <20210428102251.552976-1-gorcunov@gmail.com> <20210428102251.552976-3-gorcunov@gmail.com> Message-ID: Date: Wed, 28 Apr 2021 18:13:30 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 MIME-Version: 1.0 In-Reply-To: <20210428102251.552976-3-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9ECFD8CE5F059401062EF72DCC8B8CDABD8D4F98D4AE0C03D182A05F5380850405974505F9112F16A693CA014C98B240C00D1C70663BE9B898BF2611F9122B6E7 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7196003627DEC9496EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006378D08D652E28591A78638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B2E15D20C02D3E97ED48E7D8631BD0DE0598738A72DB41484ED2E47CDBA5A96583C09775C1D3CA48CF17B107DEF921CE79117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE71AE4D56B06699BBC9FA2833FD35BB23DF004C90652538430302FCEF25BFAB3454AD6D5ED66289B5278DA827A17800CE7BB78EDD9C0D8E15ED32BA5DBAC0009BE395957E7521B51C20BC6067A898B09E4090A508E0FED6299176DF2183F8FC7C0A2E2240085632A06CD04E86FAF290E2D7E9C4E3C761E06A71DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6BF3059D42242344A089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A53893F028A77A919DFA597811D7238D910193810F74582815D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D344E332383F80D58BBAED139E2FB5B7598A11CDA97506AE23ECFF16DD10C27D35C21C428E7DED009871D7E09C32AA3244C307F4B649E86F7C4F7D86EEBB411C93230363D8B7DA7DD44927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojoCaqxM2e5soraX/ZRoP0Mw== X-Mailru-Sender: 583F1D7ACE8F49BDD2846D59FC20E9F84A4ABAEC5A1EFCDBA5D0FD619EE19C76731CE6A9DA9A53EF424AE0EB1F3D1D21E2978F233C3FAE6EE63DB1732555E4A8EE80603BA4A5B0BC112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v2 2/2] fiber: fiber_join -- don't crash on misuse X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Serge Petrenko via Tarantool-patches Reply-To: Serge Petrenko Cc: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "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 > --- > .../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 > #include > #include > @@ -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