Re: [Tarantool-patches] [PATCH 2/2] Followup fix for embedded bytecode loader.

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the patch!
Please consider my comments below.

On Tue, Jul 25, 2023 at 07:37:01PM +0300, Sergey Bronnikov via Tarantool-patches wrote:
> From: sergeyb@tarantool.org
> 
> (cherry-picked from commit e49863eda13d095b1a78fd4ca0fd3a6a9a17d782)
The description is missing. As we've already discussed offline, it is generally
not a good practice to describe a commit in the previous commit. So it would be
great if you moved the relevant part of the description here instead.

> 
> Sergey Bronnikov:
>   * added the partial test for this and a previous patch
> ---
>  src/lj_lex.c                                  |   1 +
>  test/tarantool-c-tests/lj-549-lua_load.test.c | 146 ++++++++++++++++++
>  2 files changed, 147 insertions(+)
>  create mode 100644 test/tarantool-c-tests/lj-549-lua_load.test.c
> 
> diff --git a/src/lj_lex.c b/src/lj_lex.c
> index 82e4ba6f..161d862e 100644
> --- a/src/lj_lex.c
> +++ b/src/lj_lex.c
> @@ -51,6 +51,7 @@ static LJ_NOINLINE LexChar lex_more(LexState *ls)
>    if (sz >= LJ_MAX_BUF) {
>      if (sz != ~(size_t)0) lj_err_mem(ls->L);
>      sz = ~(uintptr_t)0 - (uintptr_t)p;
> +    if (sz >= LJ_MAX_BUF) sz = LJ_MAX_BUF-1;
>      ls->endmark = 1;
>    }
>    ls->pe = p + sz;
> diff --git a/test/tarantool-c-tests/lj-549-lua_load.test.c b/test/tarantool-c-tests/lj-549-lua_load.test.c
> new file mode 100644
> index 00000000..4fb144cf
> --- /dev/null
> +++ b/test/tarantool-c-tests/lj-549-lua_load.test.c

As I have already mentioned in the previous patch, it passes with no problems
whatsoever before the patch on x32 machine, so I am not sure we need to add it.

> @@ -0,0 +1,146 @@
> +#include <assert.h>
> +#include <stdint.h>
> +#include <stddef.h>
> +#include <string.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +#include <lua.h>
> +#include <lualib.h>
> +#include <lauxlib.h>
> +
> +#include "test.h"
> +#include "utils.h"
> +
> +/* Need for skipcond. */
> +#include "lj_arch.h"
> +
> +/* Defined in lj_def.h. */
> +#define LJ_MAX_MEM32    0x7fffff00      /* Max. 32 bit memory allocation. */
> +#define LJ_MAX_BUF      LJ_MAX_MEM32    /* Max. buffer length. */
> +
> +#define UNUSED(x) ((void)(x))
> +
> +/**
> + * Array with bytecode was generated using commands below:
> + *
> + * cat << EOF > a.lua
> + * local a = 1
> + * EOF
> + * luajit -b a.lua a.h
> + */
> +#define luaJIT_BC_sample_SIZE 22
> +static const unsigned char luaJIT_BC_sample[] = {
> +27,76,74,2,2,15,2,0,1,0,0,0,2,41,0,1,0,75,0,1,0,0
Missing spaces.
> +};
> +
> +/**
> + * Function generates a huge chunk with "bytecode" with a size bigger than
Typo: s/with "bytecode"/of "bytecode"/
> + * LJ_MAX_BUF. Generated chunk must enable endmark in a Lex state.
> + */
> +static const char *
> +bc_reader_with_endmark(lua_State *L, void *data, size_t *size)
> +{
> +	UNUSED(data);
> +	static char *bc_chunk = NULL;
> +	free(bc_chunk);
> +
> +	int bc_chunk_size = (size_t)0;
We ussually follow the C89 standard, which mentions that variables must be
declared in the beginning of the block. Here and below.
> +	bc_chunk = malloc(bc_chunk_size);
> +	assert(bc_chunk != NULL);
> +
> +	/**
> +	 * Put a chunk with a valid bytecode to the beginning of allocated region.
Typo: s/to the beginning/at the beginning/
Typo: s/of allocated/of the allocated/
> +	 * lua_load automatically detects whether the chunk is text or binary,
Typo: s/lua_load/`lua_load`/ Feel free to ignore, though.
> +	 * and loads it accordingly. We need a trace for bytecode input,
> +	 * so it is necessary to deceive a check in lj_lex_setup, that makes a
> +	 * sanity check and detects whether input is bytecode or text by first char.
Typo: s/by first/by the first/
> +	 * Strictly speaking it is enough to put LUA_SIGNATURE[0] as a first
Typo: s/Strictly speaking/Strictly speaking,/
Typo: s/a first/the first/
> +	 * symbol in produced chunk.
Typo: s/in produced/in the produced/
> +	 */
> +	memcpy(bc_chunk, luaJIT_BC_sample, luaJIT_BC_sample_SIZE);
> +
> +	*size = bc_chunk_size;
> +
> +	return bc_chunk;
> +}
> +
> +static int bc_loader_with_endmark(void *test_state)
> +{
> +	lua_State *L = test_state;
> +	void *ud = NULL;
> +	int res = lua_load(L, bc_reader_with_endmark, ud, "endmark");
> +
> +	/* Make sure we passed condition with lj_err_mem in a function lex_more. */
Typo: s/passed condition/passed the condition/
Typo: s/in a function/in the function/
> +	assert_true(res != LUA_ERRMEM);
> +
> +	return TEST_EXIT_SUCCESS;
> +}
> +
> +enum bc_emission_state {
> +	EMIT_BC,
> +	EMIT_EOF,
> +};
> +
> +typedef struct {
> +	enum bc_emission_state state;
> +} dt;
> +
> +/**
> + * Function returns bytecode chunk on the first call and NULL and size equals
Typo: s/returns bytecode/returns a bytecode/
Typo: s/equals/equal/
> + * to zero on the second call. Triggers END_OF_STREAM flag in a function
Typo: s/Triggers/Triggers the/
Typo: s/in a function/in the function/
> + * lex_more.
> + */
> +static const char *
> +bc_reader_with_eof(lua_State *L, void *data, size_t *size)
> +{
> +	UNUSED(data);
> +	UNUSED(L);
> +	dt *test_data = (dt *)data;
> +	if (test_data->state == EMIT_EOF) {
> +		*size = 0;
> +		return NULL;
> +	}
> +
> +	char *bc_chunk = malloc(luaJIT_BC_sample_SIZE);
> +	memcpy(bc_chunk, luaJIT_BC_sample, luaJIT_BC_sample_SIZE);
> +	*size = luaJIT_BC_sample_SIZE;
> +
> +	return bc_chunk;
> +}
> +
> +static int bc_loader_with_eof(void *test_state)
> +{
> +	lua_State *L = test_state;
> +	dt test_data = {0};
> +	test_data.state = EMIT_BC;
> +	int res = lua_load(L, bc_reader_with_eof, &test_data, "eof");
> +	assert_true(res = LUA_ERRSYNTAX);
> +	if (res == LUA_OK) {
> +		lua_pcall(L, 0, 0, 0);
> +	}
> +
> +	return TEST_EXIT_SUCCESS;
> +}
> +
> +int main(void)
> +{
> +	if (LJ_GC64 || !LUAJIT_ARCH_X64 || !LJ_TARGET_LINUX)
> +		/**
> +		 * lua_load source code is common on all platforms,
> +		 * when bytecode is not portable.
> +		 * So test runs on Linux/x86_64 only and skipped on other
Typo: s/So test/So the test/
Typo: s/and skipped/and is skipped/
> +		 * platforms.
> +		 */
> +		return skip_all("Enabled on Linux/x86_64 with disabled GC64");
We prefer to run tests like that on all paltforms, just to be sure. We usually
exclude tests from platforms only if those are completely irrelevant to the test,
or there is some kind of major issue.
> +
> +	lua_State *L = utils_lua_init();
> +	const struct test_unit tgroup[] = {
> +		test_unit_def(bc_loader_with_endmark),
> +		test_unit_def(bc_loader_with_eof)
> +	};
> +
> +	const int test_result = test_run_group(tgroup, L);
> +	utils_lua_close(L);
> +	return test_result;
> +}
> -- 
> 2.34.1
>