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

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

Hi, Max!

thanks for your comments. See my answers.

On 7/31/23 15:20, Maxim Kokryashkin via Tarantool-patches wrote:
> 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.
Description added to the second commit message as well.
>
>> 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.

Test for a public function is better than nothing. I would left it, but 
we can also wait

for opinion of the second reviewer and then decide.

>
>> @@ -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.
Fixed.
>> +};
>> +
>> +/**
>> + * Function generates a huge chunk with "bytecode" with a size bigger than
> Typo: s/with "bytecode"/of "bytecode"/
Fixed.
>> + * 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.
Fixed.
>> +	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/
Fixed.
>> +	 * lua_load automatically detects whether the chunk is text or binary,
> Typo: s/lua_load/`lua_load`/ Feel free to ignore, though.
Fixed.
>> +	 * 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/
Fixed.
>> +	 * 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/
Fixed.
>> +	 * symbol in produced chunk.
> Typo: s/in produced/in the produced/
Fixed.
>> +	 */
>> +	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/
Fixed.
>> +	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/
Fixed.
>> + * 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/
Fixed.
>> + * 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/
Fixed.
>> +		 * 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.

Yes, for this test the rule was ignored because bytecode is not portable

and following the rule will make maintenance difficult. Therefore 
supported platforms limited by x86_64 only.

>> +
>> +	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
>>