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

changelogs/unreleased/ghqa-321-segfault-with-clang-16-asan.md

@@ -0,0 +1,4 @@
+## bugfix/core
+
+* Fixed a crash that could happen when Tarantool is compiled by `clang`
+  version 16 with enabled AddressSanitizer (tarantool/tarantool-qa#321).

src/lib/core/fiber.c

@@ -143,21 +143,27 @@ static int (*fiber_invoke)(fiber_func f, va_list ap);
 #if ENABLE_ASAN
 #include <sanitizer/asan_interface.h>
 
-#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, 0, 0)
 #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
@@ -365,8 +371,12 @@ fiber_on_stop(struct fiber *f)
 	assert(rlist_empty(&f->on_stop));
 }
 
+/**
+ * Destroy an active fiber and prepare it for reuse (if is_reusable is set),
+ * or for deletion.
+ */
 static void
-fiber_recycle(struct fiber *fiber);
+fiber_recycle(struct fiber *fiber, bool is_reusable);
 
 static void
 fiber_stack_recycle(struct fiber *fiber);
@@ -673,17 +683,17 @@ fiber_join_timeout(struct fiber *fiber, double timeout)
 		diag_move(&fiber->diag, &fiber()->diag);
 	}
 	/* The fiber is already dead. */
-	fiber_recycle(fiber);
+	bool is_reusable = (fiber->flags & FIBER_CUSTOM_STACK) == 0;
+	fiber_recycle(fiber, is_reusable);
 	return ret;
 }
 
 /**
- * @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 +720,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,16 +906,14 @@ fiber_reset(struct fiber *fiber)
 	clock_stat_reset(&fiber->clock_stat);
 }
 
-/** Destroy an active fiber and prepare it for reuse. */
 static void
-fiber_recycle(struct fiber *fiber)
+fiber_recycle(struct fiber *fiber, bool is_reusable)
 {
 	assert((fiber->flags & FIBER_IS_DEAD) != 0);
 	/* no exceptions are leaking */
 	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 +932,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 (is_reusable) {
 		rlist_move_entry(&cord()->dead, fiber, link);
 	} else {
 		cord_add_garbage(cord(), fiber);
@@ -1030,14 +1052,22 @@ fiber_loop(MAYBE_UNUSED void *data)
 		fiber_on_stop(fiber);
 		/* reset pending wakeups */
 		rlist_del(&fiber->state);
+		bool is_reusable = (fiber->flags & FIBER_CUSTOM_STACK) == 0;
 		if (! (fiber->flags & FIBER_IS_JOINABLE))
-			fiber_recycle(fiber);
+			fiber_recycle(fiber, is_reusable);
 		/*
 		 * Crash if spurious wakeup happens, don't call the old
 		 * 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 (is_reusable)
+			fiber_yield();
+		else
+			fiber_yield_final();
 	}
 }
 

src/lib/core/fiber.h

@@ -269,6 +269,8 @@ 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 (see `fiber_testcancel()`), but it is
+ * considered a good practice to call `fiber_testcancel()` after each yield.
  *
  * \sa fiber_wakeup
  */