From 72a6abeecdc884c129c0d7e3bdf94ec6a198ed0f Mon Sep 17 00:00:00 2001 From: Ilya Verbin Date: Wed, 9 Aug 2023 18:53:27 +0300 Subject: [PATCH] core: fix ASAN_START_SWITCH_FIBER() usage The `__sanitizer_start_switch_fiber()` function takes a pointer as the first argument to store the current fake stack if there is one (it is necessary when stack-use-after-return detection is enabled). When leaving a fiber definitely, NULL must be passed so that the fake stack is destroyed. Before this patch, NULL was passed for dead fibers, however this is wrong for dead fibers that are recycled and resumed. In such cases ASAN destroys the fake stack, and the fiber crashes trying to use it in `fiber_yield()` upon return from `coro_transfer()`. Closes tarantool/tarantool-qa#321 NO_DOC=bugfix NO_TEST=tested by test-release-asan workflow --- .../ghqa-321-segfault-with-clang-16-asan.md | 5 ++ src/lib/core/fiber.c | 87 +++++++++++++------ src/lib/core/fiber.h | 3 + 3 files changed, 68 insertions(+), 27 deletions(-) create mode 100644 changelogs/unreleased/ghqa-321-segfault-with-clang-16-asan.md diff --git a/changelogs/unreleased/ghqa-321-segfault-with-clang-16-asan.md b/changelogs/unreleased/ghqa-321-segfault-with-clang-16-asan.md new file mode 100644 index 000000000000..1d03d1588f3d --- /dev/null +++ b/changelogs/unreleased/ghqa-321-segfault-with-clang-16-asan.md @@ -0,0 +1,5 @@ +## bugfix/core + +* Fixed a crash that could happen when Tarantool is compiled by `clang` + version 15 and above with enabled AddressSanitizer + (tarantool/tarantool-qa#321). diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c index 958e4a02fba6..2a4d6482cf25 100644 --- a/src/lib/core/fiber.c +++ b/src/lib/core/fiber.c @@ -143,21 +143,27 @@ static int (*fiber_invoke)(fiber_func f, va_list ap); #if ENABLE_ASAN #include -#define ASAN_START_SWITCH_FIBER(var_name, will_switch_back, bottom, size) \ - void *var_name = NULL; \ - __sanitizer_start_switch_fiber((will_switch_back) ? &var_name : NULL, \ +#define ASAN_START_SWITCH_FIBER(fake_stack_save, will_switch_back, bottom, \ + size) \ + /* \ + * When leaving a fiber definitely, NULL must be passed as the first \ + * argument so that the fake stack is destroyed. \ + */ \ + void *fake_stack_save = NULL; \ + __sanitizer_start_switch_fiber((will_switch_back) ? &fake_stack_save \ + : NULL, \ (bottom), (size)) #if ASAN_INTERFACE_OLD -#define ASAN_FINISH_SWITCH_FIBER(var_name) \ - __sanitizer_finish_switch_fiber(var_name); +#define ASAN_FINISH_SWITCH_FIBER(fake_stack_save) \ + __sanitizer_finish_switch_fiber(fake_stack_save) #else -#define ASAN_FINISH_SWITCH_FIBER(var_name) \ - __sanitizer_finish_switch_fiber(var_name, 0, 0); +#define ASAN_FINISH_SWITCH_FIBER(fake_stack_save) \ + __sanitizer_finish_switch_fiber(fake_stack_save, NULL, NULL) #endif #else -#define ASAN_START_SWITCH_FIBER(var_name, will_switch_back, bottom, size) -#define ASAN_FINISH_SWITCH_FIBER(var_name) +#define ASAN_START_SWITCH_FIBER(fake_stack_save, will_switch_back, bottom, size) +#define ASAN_FINISH_SWITCH_FIBER(fake_stack_save) #endif static inline int @@ -396,6 +402,17 @@ cord_reset_slice(struct fiber *f) cord()->slice = new_slice; } +/** + * True if a fiber with `fiber_flags` can be reused. + * A fiber can not be reused if it is somehow non-standard. + */ +static bool +fiber_is_reusable(uint32_t fiber_flags) +{ + /* For now we can not reuse fibers with custom stack size. */ + return (fiber_flags & FIBER_CUSTOM_STACK) == 0; +} + /** * Transfer control to callee fiber. */ @@ -678,12 +695,11 @@ fiber_join_timeout(struct fiber *fiber, double timeout) } /** - * @note: this is not a cancellation point (@sa fiber_testcancel()) - * but it is considered good practice to call testcancel() - * after each yield. + * Implementation of `fiber_yield()` and `fiber_yield_final()`. + * `will_switch_back` argument is used only by ASAN. */ -void -fiber_yield(void) +static void +fiber_yield_impl(MAYBE_UNUSED bool will_switch_back) { struct cord *cord = cord(); struct fiber *caller = cord->fiber; @@ -710,14 +726,28 @@ fiber_yield(void) cord->fiber = callee; callee->flags = (callee->flags & ~FIBER_IS_READY) | FIBER_IS_RUNNING; - ASAN_START_SWITCH_FIBER(asan_state, - (caller->flags & FIBER_IS_DEAD) == 0, - callee->stack, + ASAN_START_SWITCH_FIBER(asan_state, will_switch_back, callee->stack, callee->stack_size); coro_transfer(&caller->ctx, &callee->ctx); ASAN_FINISH_SWITCH_FIBER(asan_state); } +void +fiber_yield(void) +{ + fiber_yield_impl(true); +} + +/** + * Like `fiber_yield()`, but should be used when this is the last switch from + * a dead fiber to the scheduler. + */ +static void +fiber_yield_final(void) +{ + fiber_yield_impl(false); +} + struct fiber_watcher_data { struct fiber *f; bool timed_out; @@ -882,7 +912,7 @@ fiber_reset(struct fiber *fiber) clock_stat_reset(&fiber->clock_stat); } -/** Destroy an active fiber and prepare it for reuse. */ +/** Destroy an active fiber and prepare it for reuse or delete it. */ static void fiber_recycle(struct fiber *fiber) { @@ -891,7 +921,6 @@ fiber_recycle(struct fiber *fiber) assert(diag_is_empty(&fiber->diag)); /* no pending wakeup */ assert(rlist_empty(&fiber->state)); - bool has_custom_stack = fiber->flags & FIBER_CUSTOM_STACK; fiber_stack_recycle(fiber); fiber_reset(fiber); fiber->name[0] = '\0'; @@ -910,7 +939,7 @@ fiber_recycle(struct fiber *fiber) fiber->first_alloc_bt = NULL; region_set_callbacks(&fiber->gc, NULL, NULL, NULL); #endif - if (!has_custom_stack) { + if (fiber_is_reusable(fiber->flags)) { rlist_move_entry(&cord()->dead, fiber, link); } else { cord_add_garbage(cord(), fiber); @@ -1037,7 +1066,14 @@ fiber_loop(MAYBE_UNUSED void *data) * function again, ap is garbage by now. */ fiber->f = NULL; - fiber_yield(); /* give control back to scheduler */ + /* + * Give control back to the scheduler. + * If the fiber is not reusable, this is its final yield. + */ + if (fiber_is_reusable(fiber->flags)) + fiber_yield(); + else + fiber_yield_final(); } } @@ -1329,13 +1365,10 @@ fiber_new_ex(const char *name, const struct fiber_attr *fiber_attr, assert(fiber_attr != NULL); cord_collect_garbage(cord); - /* Now we can not reuse fiber if custom attribute was set */ - if (!(fiber_attr->flags & FIBER_CUSTOM_STACK) && - !rlist_empty(&cord->dead)) { - fiber = rlist_first_entry(&cord->dead, - struct fiber, link); + if (fiber_is_reusable(fiber_attr->flags) && !rlist_empty(&cord->dead)) { + fiber = rlist_first_entry(&cord->dead, struct fiber, link); rlist_move_entry(&cord->alive, fiber, link); - assert((fiber->flags | FIBER_IS_DEAD) != 0); + assert(fiber_is_dead(fiber)); } else { fiber = (struct fiber *) mempool_alloc(&cord->fiber_mempool); diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h index ab7e0a2160a3..21ada812d92b 100644 --- a/src/lib/core/fiber.h +++ b/src/lib/core/fiber.h @@ -270,6 +270,9 @@ fiber_new_ex(const char *name, const struct fiber_attr *fiber_attr, fiber_func f /** * Return control to another fiber and wait until it'll be woken. * + * \note this is not a cancellation point (\sa fiber_testcancel()), but it is + * considered a good practice to call fiber_testcancel() after each yield. + * * \sa fiber_wakeup */ API_EXPORT void