Re: [Tarantool-patches] [PATCH v2 1/2] fiber: use diag_ logger in fiber_madvise/mprotect failures

[Alexander Turenko <alexander.turenko@tarantool.org>]

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 <alexander.turenko@tarantool.org>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  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 <x86intrin.h> /* __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;
 }