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 C84115B2D6; Wed, 28 Apr 2021 13:23:58 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C84115B2D6 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1619605438; bh=BxYGGhIMdLHZeCr7mI0A8k1cAPlhpUMtFCi0xxUwupQ=; h=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=EhAljwWVen36A/qNOlvNk70pG9if8jDBWjBNaKnD7XF1vn2gAlL25rZegRHmn8B2w LViVbaxLWyxgPM+nG7aQpPZHV1giVKs2k4kfXvhy7FvLMYuw1/gz9zftsr//N7S2Fs CHs+AwQzGIJUQr7I680dspQlRHO7r6i1HqkQ+FEE= Received: from mail-lf1-f49.google.com (mail-lf1-f49.google.com [209.85.167.49]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 661477034A for ; Wed, 28 Apr 2021 13:23:25 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 661477034A Received: by mail-lf1-f49.google.com with SMTP id h36so44166162lfv.7 for ; Wed, 28 Apr 2021 03:23:25 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=tNpTpDkUwWe2WBwFXVR8n3t5omfUBys7mZXsjzDZm18=; b=DL7gR3cGPtbn+V3uGBLm274D+wGTQgGOcnGt3KCZQlbpPPtJZWFH++j7JtOJ5s3evX MWzHFslQW8FchIG0O3Vn1J111G33WkeI7WRme2wC7qz78yk0tBug1dNaPSxwMANwJLq9 IXXsNXrv8MclHun97Pmz8i68qL1XkCrbx83gYRkim2zdrQnVtB6f8LsaeiMCAzdd00ao t8qKLm7/njeue7J1rQ2LxzHN6hXpqnhs13AfRYyt0Jz/ekL+FOEkuDExuBW+cfRD/IX0 PJgDlHdmA+AOR1QYTsMys3OdJWF/dZm2dhOCeFpJ3uI+43JtI8m5iRmRt568OpboITNf WH4g== X-Gm-Message-State: AOAM532DaTxxDw90hAxFUYDAlwvt3/+spfYsvdxN4BwuPGfT5ByQzrTX oeLTnwIWFNG5mhoKQHqSKV8X91kDb8M= X-Google-Smtp-Source: ABdhPJx8pLa7rYEXtRaFd97CyEFkCa0oTCOGk/MtRDpFCF1LuKodZwldrnhg/XQPdzZN1EJQg/5W4g== X-Received: by 2002:a05:6512:1106:: with SMTP id l6mr19948865lfg.653.1619605404281; Wed, 28 Apr 2021 03:23:24 -0700 (PDT) Received: from grain.localdomain ([5.18.199.94]) by smtp.gmail.com with ESMTPSA id j18sm604615lfe.86.2021.04.28.03.23.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 28 Apr 2021 03:23:23 -0700 (PDT) Received: by grain.localdomain (Postfix, from userid 1000) id 10C03560127; Wed, 28 Apr 2021 13:22:55 +0300 (MSK) To: tml Date: Wed, 28 Apr 2021 13:22:51 +0300 Message-Id: <20210428102251.552976-3-gorcunov@gmail.com> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20210428102251.552976-1-gorcunov@gmail.com> References: <20210428102251.552976-1-gorcunov@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [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: Cyrill Gorcunov via Tarantool-patches Reply-To: Cyrill Gorcunov Cc: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "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 --- .../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); +--- +... +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