Re: [Tarantool-patches] [PATCH luajit 2/3] sysprof: fix crash on a failed stream flush

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Sergey,
Thanks for the fixing this issue!
Generally LGTM, with a minor nits regarding the tests below.

On 02.11.26, Sergey Bronnikov wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> A writer may release the stream buffer and set its pointer to NULL
> on an unrecoverable error, e.g., when the file system with the
> profile is full. The write buffer stops the stream in this case,
> but a caller of `wbuf_reserve()` still writes data using the
> released buffer. This leads to a null pointer dereference inside
> `lj_utils_write_uleb128()` when the flush is triggered while
> streaming a sample.
> 
> Now `wbuf_reserve()` reports whether the stream was stopped during
> the flush. All the write functions skip writing in this case,
> including the loop pumping a large payload through the buffer.
> 
> Fixes tarantool/tarantool#11620

Typo: s/Fixes/Resolves/
It will be fixed after submodule bump in the Tarantool.

> ---
>  src/lj_wbuf.c                                 |  20 +-
>  .../gh-11620-sysprof-enospc-crash.test.c      | 180 ++++++++++++++++++
>  test/tarantool-c-tests/unit-wbuf.test.c       | 102 ++++++++++
>  3 files changed, 298 insertions(+), 4 deletions(-)
>  create mode 100644 test/tarantool-c-tests/gh-11620-sysprof-enospc-crash.test.c
> 
> diff --git a/src/lj_wbuf.c b/src/lj_wbuf.c
> index 0001a02e..0794b186 100644
> --- a/src/lj_wbuf.c
> +++ b/src/lj_wbuf.c

<snipped>

> diff --git a/test/tarantool-c-tests/gh-11620-sysprof-enospc-crash.test.c b/test/tarantool-c-tests/gh-11620-sysprof-enospc-crash.test.c
> new file mode 100644
> index 00000000..0f5ffe53
> --- /dev/null
> +++ b/test/tarantool-c-tests/gh-11620-sysprof-enospc-crash.test.c
> @@ -0,0 +1,180 @@
> +/*
> +** Testing of the sysprof behaviour when a stream write fails.
> +**
> +** A full file system makes the sysprof writer return an error and
> +** release the stream buffer. The profiler must stop the stream
> +** gracefully, return an error from luaM_sysprof_stop() and
> +** release the resources.
> +*/

Minor: It's better to use
| /*
|  *
|  */
Style of comments in the tests to be consistent with other codebase.
The comment style in the <unit-leb128.test.c> is inconsistent by my
mistake.


> +
> +#include "lauxlib.h"
> +#include "lua.h"
> +
> +/* Need for skipcond for OS and ARCH. */
> +#include "lj_arch.h"
> +
> +#include "test.h"
> +
> +#if LJ_HASSYSPROF && !defined(LUAJIT_USE_VALGRIND)
> +
> +#include "lmisclib.h"
> +#include "utils.h"
> +
> +#include <errno.h>
> +#include <signal.h>
> +#include <stdint.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +/*
> + * The buffer size is enough for the maximal ULEB128 value.
> + * A small buffer makes the profiler flush it right during the
> + * prologue or the first sample.
> + */
> +#define STREAM_BUFFER_SIZE 10
> +
> +/* Do not let the timer interfere with the manually emitted signals. */

Nit: Comment line length is more than 66 symbols.

<snipped>

> +static int sysprof_enospc_during_sampling(void *test_state)
> +{
> +	lua_State *L = test_state;
> +	static uint8_t buf[STREAM_BUFFER_SIZE];
> +	struct luam_Sysprof_Options opt;
> +	struct luam_Sysprof_Counters counters = {};
> +
> +	sysprof_test_configure(&opt, buf);
> +	assert_true(luaM_sysprof_start(L, &opt) == PROFILE_SUCCESS);
> +	assert_int_equal(ctx.on_stop_calls, 0);
> +
> +	/* Let the next stream flush fail as if the disk is full. */

Nit: Comment line length is more than 66 symbols.

> +	ctx.fail = 1;
> +	assert_int_equal(kill(getpid(), SIGPROF), 0);
> +
> +	/* The stream is stopped gracefully, without a crash. */
> +	errno = 0;
> +	assert_int_equal(luaM_sysprof_stop(L), PROFILE_ERRIO);
> +	assert_int_equal(errno, ENOSPC);
> +
> +	/* Counters are still available. */
> +	assert_true(luaM_sysprof_report(&counters) == PROFILE_SUCCESS);
> +	assert_true(counters.samples >= 1);
> +
> +	/* The profiler is stopped. */
> +	assert_int_equal(luaM_sysprof_stop(L), PROFILE_ERRRUN);
> +
> +	return TEST_EXIT_SUCCESS;
> +}
> +
> +static int sysprof_enospc_on_start(void *test_state)
> +{
> +	lua_State *L = test_state;
> +	static uint8_t buf[STREAM_BUFFER_SIZE];
> +	struct luam_Sysprof_Options opt;
> +
> +	sysprof_test_configure(&opt, buf);
> +	/* The very first flush fails, so the prologue cannot be streamed. */

Nit: Comment line length is more than 66 symbols.

> +	ctx.fail = 1;
> +	errno = 0;
> +	assert_int_equal(luaM_sysprof_start(L, &opt), PROFILE_ERRIO);
> +	assert_int_equal(errno, ENOSPC);
> +	/* The start error path releases the profiler resources. */

Nit: Comment line length is more than 66 symbols.

> +	assert_int_equal(ctx.on_stop_calls, 1);
> +
> +	/* The profiler is not running. */
> +	assert_int_equal(luaM_sysprof_stop(L), PROFILE_ERRRUN);
> +
> +	return TEST_EXIT_SUCCESS;
> +}
> +
> +#endif /* LJ_HASSYSPROF && !defined(LUAJIT_USE_VALGRIND) */
> +
> +int main(void)
> +{
> +#if LUAJIT_USE_VALGRIND
> +	return skip_all("Disabled due to #10803");
> +#elif !LJ_HASSYSPROF
> +	return skip_all("Sysprof is disabled");
> +#else
> +	if (LUAJIT_OS != LUAJIT_OS_LINUX)
> +		return skip_all("Sysprof is implemented for Linux only");
> +	if (LUAJIT_TARGET != LUAJIT_ARCH_X86
> +	    && LUAJIT_TARGET != LUAJIT_ARCH_X64)
> +		return skip_all("Sysprof is implemented for x86_64 only");
> +
> +	lua_State *L = utils_lua_init();
> +
> +	const struct test_unit tgroup[] = {
> +		test_unit_def(sysprof_enospc_during_sampling),
> +		test_unit_def(sysprof_enospc_on_start)
> +	};
> +	const int test_result = test_run_group(tgroup, L);
> +	utils_lua_close(L);
> +	return test_result;
> +#endif /* LUAJIT_USE_VALGRIND */
> +}
> diff --git a/test/tarantool-c-tests/unit-wbuf.test.c b/test/tarantool-c-tests/unit-wbuf.test.c
> index 4bee6243..c1b71ff3 100644
> --- a/test/tarantool-c-tests/unit-wbuf.test.c
> +++ b/test/tarantool-c-tests/unit-wbuf.test.c
> @@ -367,6 +367,104 @@ static int test_terminate(void *state)
>  ** this case, so the write functions must not touch the released
>  ** buffer.
>  */
> +static int test_addbyte_failed_flush(void *state)
> +{
> +	uint8_t mem[BUFFER_SIZE];
> +	struct lj_wbuf buf;
> +	struct writer_ctx ctx;
> +	size_t i;
> +
> +	UNUSED(state);
> +
> +	writer_ctx_init(&ctx, WMODE_FAIL);
> +	lj_wbuf_init(&buf, test_writer, &ctx, mem, sizeof(mem));
> +
> +	/* Fill the buffer without flushes. */
> +	for (i = 0; i < sizeof(mem); ++i)
> +		lj_wbuf_addbyte(&buf, (uint8_t)i);
> +	assert_sizet_equal(ctx.flushes, 0);
> +	assert_sizet_equal(buf_len(&buf), sizeof(mem));
> +
> +	/* The buffer is full, so the byte below triggers a failed flush. */

Nit: Comment line length is more than 66 symbols.

> +	lj_wbuf_addbyte(&buf, 0xaa);
> +	assert_stream_stopped_with_error(&buf);
> +	assert_sizet_equal(ctx.flushes, 1);
> +
> +	return TEST_EXIT_SUCCESS;
> +}
> +
> +static int test_addu64_failed_flush(void *state)
> +{
> +	uint8_t mem[BUFFER_SIZE];
> +	struct lj_wbuf buf;
> +	struct writer_ctx ctx;
> +
> +	UNUSED(state);
> +
> +	writer_ctx_init(&ctx, WMODE_FAIL);
> +	lj_wbuf_init(&buf, test_writer, &ctx, mem, sizeof(mem));
> +
> +	/* The maximal value takes exactly BUFFER_SIZE bytes. */
> +	lj_wbuf_addu64(&buf, UINT64_MAX);
> +	assert_sizet_equal(buf_len(&buf), sizeof(mem));
> +
> +	/* There is no space left, so the next write triggers a failed flush. */

Nit: Comment line length is more than 66 symbols.

> +	lj_wbuf_addu64(&buf, 0);
> +	assert_stream_stopped_with_error(&buf);
> +	assert_sizet_equal(ctx.flushes, 1);
> +
> +	return TEST_EXIT_SUCCESS;
> +}
> +
> +static int test_addn_failed_flush(void *state)
> +{
> +	uint8_t mem[BUFFER_SIZE];
> +	uint8_t payload[BUFFER_SIZE];
> +	struct lj_wbuf buf;
> +	struct writer_ctx ctx;
> +
> +	UNUSED(state);
> +
> +	memset(payload, 'a', sizeof(payload));
> +	writer_ctx_init(&ctx, WMODE_FAIL);
> +	lj_wbuf_init(&buf, test_writer, &ctx, mem, sizeof(mem));
> +
> +	/* Fill the buffer without flushes. */
> +	lj_wbuf_addn(&buf, payload, sizeof(payload));
> +	assert_sizet_equal(ctx.flushes, 0);
> +
> +	/* There is no space left, so the next write triggers a failed flush. */

Nit: Comment line length is more than 66 symbols.

> +	lj_wbuf_addn(&buf, "b", 1);
> +	assert_stream_stopped_with_error(&buf);
> +	assert_sizet_equal(ctx.flushes, 1);
> +
> +	return TEST_EXIT_SUCCESS;
> +}
> +
> +static int test_addn_failed_flush_in_loop(void *state)
> +{
> +	uint8_t mem[BUFFER_SIZE];
> +	uint8_t payload[PAYLOAD_SIZE];
> +	struct lj_wbuf buf;
> +	struct writer_ctx ctx;
> +
> +	UNUSED(state);
> +
> +	memset(payload, 'a', sizeof(payload));
> +	writer_ctx_init(&ctx, WMODE_FAIL);
> +	lj_wbuf_init(&buf, test_writer, &ctx, mem, sizeof(mem));
> +
> +	/*
> +	 * The payload does not fit into the buffer, so it is pumped through
> +	 * it. The flush fails in the middle of the pump loop.
> +	 */

Nit: Comment line length is more than 66 symbols.

> +	lj_wbuf_addn(&buf, payload, sizeof(payload));
> +	assert_stream_stopped_with_error(&buf);
> +	assert_sizet_equal(ctx.flushes, 1);
> +
> +	return TEST_EXIT_SUCCESS;
> +}
> +
>  int main(void)
>  {
>  	const struct test_unit tgroup[] = {
> @@ -381,6 +479,10 @@ int main(void)
>  		test_unit_def(test_flush_stopped_stream),
>  		test_unit_def(test_write_to_stopped_stream),
>  		test_unit_def(test_terminate),
> +		test_unit_def(test_addbyte_failed_flush),
> +		test_unit_def(test_addu64_failed_flush),
> +		test_unit_def(test_addn_failed_flush),

I suppose this 3 tests may be placed in one test_group, since they are:
1) Init buffer
2) Fill it until maximum capacity.
3) Write a value with 1 flush and ENOSPC error.
Hence, I suppose initialization of the test may be unified.

Feel free to ignore, though.

> +		test_unit_def(test_addn_failed_flush_in_loop),
>  	};
>  	const int test_result = test_run_group(tgroup, NULL);
>  
> -- 
> 2.56.0
> 

-- 
Best regards,
Sergey Kaplun