From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp61.i.mail.ru (smtp61.i.mail.ru [217.69.128.41]) (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 3393246970E for ; Tue, 4 Feb 2020 00:56:15 +0300 (MSK) Date: Tue, 4 Feb 2020 00:56:27 +0300 From: Alexander Turenko Message-ID: <20200203215627.l55b6rikwubeaqvn@tkn_work_nb> References: <20200115170524.20471-1-gorcunov@gmail.com> <20200115170524.20471-2-gorcunov@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200115170524.20471-2-gorcunov@gmail.com> Subject: Re: [Tarantool-patches] [PATCH v2 1/2] fiber: use diag_ logger in fiber_madvise/mprotect failures List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov Cc: tml Everything looks correct. However, I don't believe my eyes, so I wrote a test case (see below). Don't sure whether it is good to have error injections for such functions. Maybe LD_PRELOAD way is better (see [1]). Anyway, I checked that the behaviour was incorrect using the test and that it becomes correct. Please, consider the shared test and decide whether it worth to add it to the repository (I don't sure). [1]: https://tbrindus.ca/correct-ld-preload-hooking-libc/ WBR, Alexander Turenko. On Wed, Jan 15, 2020 at 08:05:23PM +0300, Cyrill Gorcunov wrote: > Both madvise and mprotect calls can fail due to various > reasons, mostly because of lack of free memory in the > system. > > We log such cases via say_x helpers but this is not enough. > In particular tarantool/memcached relies on diag error to be > set to detect an error condition: > > | expire_fiber = fiber_new(name, memcached_expire_loop); > | const box_error_t *err = box_error_last(); > | if (err) { > | say_error("Can't start the expire fiber"); > | say_error("%s", box_error_message(err)); > | return -1; > | } > > Thus lets use diag_set() helper here and instead of macros > use inline functions for better readability. > > Fixes #4722 > > Reported-by: Alexander Turenko > Signed-off-by: Cyrill Gorcunov > --- > src/lib/core/fiber.c | 70 +++++++++++++++++++++++++++++++++----------- > 1 file changed, 53 insertions(+), 17 deletions(-) ---- diff --git a/src/lib/core/errinj.h b/src/lib/core/errinj.h index 672da2119..10d761e69 100644 --- a/src/lib/core/errinj.h +++ b/src/lib/core/errinj.h @@ -135,6 +135,9 @@ struct errinj { _(ERRINJ_COIO_SENDFILE_CHUNK, ERRINJ_INT, {.iparam = -1}) \ _(ERRINJ_SWIM_FD_ONLY, ERRINJ_BOOL, {.bparam = false}) \ _(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT, {.iparam = 0}) \ + _(ERRINJ_FIBER_MADVISE, ERRINJ_BOOL, {.bparam = false}) \ + _(ERRINJ_FIBER_MPROTECT_PROT_NONE, ERRINJ_BOOL, {.bparam = false}) \ + ENUM0(errinj_id, ERRINJ_LIST); extern struct errinj errinjs[]; diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c index fdad7607c..b698419cf 100644 --- a/src/lib/core/fiber.c +++ b/src/lib/core/fiber.c @@ -41,6 +41,7 @@ #include "assoc.h" #include "memory.h" #include "trigger.h" +#include "errinj.h" #if ENABLE_FIBER_TOP #include /* __rdtscp() */ @@ -176,7 +177,12 @@ static int (*fiber_invoke)(fiber_func f, va_list ap); static inline int fiber_madvise(void *addr, size_t len, int advice) { - if (madvise(addr, len, advice) != 0) { + int rc = 0; + ERROR_INJECT(ERRINJ_FIBER_MADVISE, { + errno = ENOMEM; + rc = -1; + }); + if (rc != 0 || madvise(addr, len, advice) != 0) { diag_set(SystemError, "fiber madvise %p:%zu:%d failed", addr, len, advice); return -1; @@ -187,7 +193,14 @@ fiber_madvise(void *addr, size_t len, int advice) static inline int fiber_mprotect(void *addr, size_t len, int prot) { - if (mprotect(addr, len, prot) != 0) { + int rc = 0; + ERROR_INJECT(ERRINJ_FIBER_MPROTECT_PROT_NONE, { + if (prot == PROT_NONE) { + errno = ENOMEM; + rc = -1; + } + }); + if (rc != 0 || mprotect(addr, len, prot) != 0) { diag_set(SystemError, "fiber mprotect %p:%zu:%d failed", addr, len, prot); return -1; diff --git a/test/unit/fiber.cc b/test/unit/fiber.cc index 91f7d43f9..d64c4f89d 100644 --- a/test/unit/fiber.cc +++ b/test/unit/fiber.cc @@ -2,6 +2,7 @@ #include "fiber.h" #include "unit.h" #include "trivia/util.h" +#include "errinj.h" static struct fiber_attr default_attr; @@ -181,12 +182,43 @@ fiber_name_test() footer(); } +static void +fiber_mprotect_fail_test(void) +{ + header(); + + /* + * Set non-default stack size to prevent reusing of an + * existing fiber. + */ + struct fiber_attr *fiber_attr = fiber_attr_new(); + fiber_attr_setstacksize(fiber_attr, default_attr.stack_size * 2); + + /* + * Clear the fiber's diagnostics area to check that failed + * fiber_new() sets an error. + */ + diag_clear(diag_get()); + + struct errinj *errinj = &errinjs[ERRINJ_FIBER_MPROTECT_PROT_NONE]; + errinj->bparam = true; + struct fiber *fiber = fiber_new_ex("test_mprotect", fiber_attr, noop_f); + errinj->bparam = false; + + ok(fiber == NULL, "failed mprotect() causes fiber_new() fail"); + struct error *error = diag_last_error(diag_get()); + ok(error != NULL, "failed fiber_new() sets an error"); + + footer(); +} + static int main_f(va_list ap) { fiber_name_test(); fiber_join_test(); fiber_stack_test(); + fiber_mprotect_fail_test(); ev_break(loop(), EVBREAK_ALL); return 0; }