Re: [Tarantool-patches] [PATCH v2 2/4] fiber: add option and PoC for Lua parent backtrace

[Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Fri, Jul 09, 2021 at 02:03:51PM +0300, Egor Elchinov via Tarantool-patches wrote:
> 
> diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
> index 924ff3c82..dd26e2f13 100644
> --- a/src/lib/core/fiber.c
> +++ b/src/lib/core/fiber.c
> @@ -220,6 +220,10 @@ fiber_mprotect(void *addr, size_t len, int prot)
>  static __thread bool fiber_top_enabled = false;
>  #endif /* ENABLE_FIBER_TOP */
>  
> +#if ENABLE_BACKTRACE
> +static __thread bool fiber_parent_bt_enabled = false;
> +#endif /* ENABLE_BACKTRACE */
> +

So this variable is declared as TLS in a manner of fiber_top
functionality. You know I somehow find this fishy because
it is unclear why it is done this way. I think we should
revisit this moment for @fiber_top_enabled enabled as well
but not in this series, so lets leave it as is.

Please revisit snprintf calls in the patch and strncpy
as well because they _do_not_ append terminating \0 symbol
if destination size is not enough. I think we should add
\0 explicitly all the time.

I mean

+static int
+fiber_parent_backtrace_cb(int frameno, void *frameret, const char *func,
+			  size_t offset, void *cb_ctx)
+{
	...
+	char buf[512];
+	int l = snprintf(buf, sizeof(buf), "#%-2d %p in ", frameno, frameret);
+	if (func)
+		snprintf(buf + l, sizeof(buf) - l, "%s+%zu", func, offset);
+	else
+		snprintf(buf + l, sizeof(buf) - l, "?");

we could add

	buf[sizeof(buf)-1] = '\0';

Actually 512 bytes for function name should be more than enough but
better be on a safe side.

Other than that the patch looks ok to me, great job, thanks!