Re: [Tarantool-patches] [PATCH v7 2/2] fiber: leak slab if unable to bring prots back

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the fixes!

See 2 comments below.

On 13/02/2020 21:56, Cyrill Gorcunov wrote:
> In case if we unable to revert guard page back to
> read|write we should never use such slab again.
> 
> Initially I thought of just put panic here and
> exit but it is too destructive. I think better
> print an error and continue. If node admin ignore
> this message then one moment at future there won't
> be slab left for use and creating new fibers get
> prohibited.
> 
> In future (hopefully near one) we plan to drop
> guard pages to prevent VMA fracturing and use
> stack marks instead.
> 
> Reviewed-by: Alexander Turenko <alexander.turenko@tarantool.org>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  src/lib/core/fiber.c         | 11 +++++++++--
>  test/unit/fiber_stack.c      | 29 ++++++++++++++++++++++++++---
>  test/unit/fiber_stack.result |  4 +++-
>  3 files changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/test/unit/fiber_stack.c b/test/unit/fiber_stack.c
> index 103dfaf0d..5eb128ea1 100644
> --- a/test/unit/fiber_stack.c
> +++ b/test/unit/fiber_stack.c
> @@ -59,6 +58,30 @@ main_f(va_list ap)
>  	ok(fiber != NULL, "madvise: non critical error on madvise hint");
>  	ok(diag_get() != NULL, "madvise: diag is armed after error");
>  
> +	/*
> +	 * Check if we leak on fiber descrution.

1. You probably have meant 'destruction'.

> +	 * We will print an error and result get
> +	 * compared by testing engine.
> +	 */
> +	fiber_attr_delete(fiber_attr);
> +	fiber_attr = fiber_attr_new();
> +	fiber_attr->flags |= FIBER_CUSTOM_STACK;
> +	fiber_attr->stack_size = 64 << 10;
> +
> +	diag_clear(diag_get());
> +	fiber = fiber_new_ex("test_madvise", fiber_attr, noop_f);
> +	ok(fiber != NULL, "fiber with custom stack");
> +	fiber_set_joinable(fiber, true);
> +
> +	inj = errinj(ERRINJ_FIBER_MPROTECT, ERRINJ_INT);
> +	inj->iparam = PROT_READ | PROT_WRITE;
> +
> +	fiber_start(fiber);
> +	fiber_join(fiber);
> +
> +	inj->iparam = -1;
> +
> +	fiber_attr_delete(fiber_attr);

2. When I run the test without the fix, I got 'Bus error'.
Was it intended? In my email I proposed to compare memory
statistics, but crash also looks ok, maybe. If this is what
you meant. And then there should be a comment saying, that we
expect fiber_join() to crash when no leak is done, because it
would put the slab into the cash, where it would be read/written
against protection. This was not obvious until I reverted your
patch and run the test.

IMO, problem of that way of testing is that if internals of
core/fiber.c one day will stop touching stack of a just
destroyed fiber (for example, a pointer at it would be cached
somewhere for newer fibers), this test will happily pass, even
though it perhaps should not. It will pass regardless of what
happens with a fiber stack unless something tries to read/write
it.

>  	footer();
>  
>  	ev_break(loop(), EVBREAK_ALL);
> diff --git a/test/unit/fiber_stack.result b/test/unit/fiber_stack.result
> index 43ff74b2f..f0f5a59c2 100644
> --- a/test/unit/fiber_stack.result
> +++ b/test/unit/fiber_stack.result
> @@ -1,8 +1,10 @@
>  SystemError fiber mprotect failed: Cannot allocate memory
> +fiber: Can't put guard page to slab. Leak 57344 bytes: Cannot allocate memory
>  	*** main_f ***
> -1..4
> +1..5
>  ok 1 - mprotect: failed to setup fiber guard page
>  ok 2 - mprotect: diag is armed after error
>  ok 3 - madvise: non critical error on madvise hint
>  ok 4 - madvise: diag is armed after error
> +ok 5 - fiber with custom stack
>  	*** main_f: done ***
>