From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id DCBDA55BD6E; Mon, 31 Jul 2023 15:20:16 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org DCBDA55BD6E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1690806017; bh=bNoe7dLtbrdvWUW1znxiL+1HHqOj/0mqafqvS6JhuhI=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=qbK6Oc+TQeJc1n+zav0cfs1HTH7678+35KN+Se7849kO1IzCMkTvJs59J7wTIHoG3 2SJli7PXH+VdAVWvetxIOTN6ovv3rdlU6TCIElakkyvIeD8KwR6mjNRCLSVYEuleud NbUEzIzH6JWoFG20Ke8ijV0y7XyF0+PBdW9KkFrM= Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [95.163.41.91]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 8E47155BD6E for ; Mon, 31 Jul 2023 15:20:15 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 8E47155BD6E Received: by smtp49.i.mail.ru with esmtpa (envelope-from ) id 1qQRsl-006HkW-0Q; Mon, 31 Jul 2023 15:20:15 +0300 Date: Mon, 31 Jul 2023 15:20:14 +0300 To: Sergey Bronnikov Message-ID: References: <738d30841b4fb39c094e3fe53317c8f7915e268a.1690300762.git.sergeyb@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <738d30841b4fb39c094e3fe53317c8f7915e268a.1690300762.git.sergeyb@tarantool.org> X-Mailru-Src: smtp X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD920919EE866B1AD244612AE7A9AA84D5E3192772E8A364F6300894C459B0CD1B9267ABF86B7BCBE7767D4C11C413F4A74180CCFF49CFF00F8599FBF3E6EA3F319 X-C1DE0DAB: 0D63561A33F958A555F25C8C691E287ECB7633554CBA732C800696E7FB627DA2F87CCE6106E1FC07E67D4AC08A07B9B0CE135D2742255B35CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0AD5177F0B940C8B66ECE892A7B2722663E91682638B966EB3F662256BEEFA9527F5D5B2EFB5A2559157C12F7BCC1F737541B239D7DAFF4FC0886C839D06CFC3AE163D82A80DC522A2637FD76D11AF80A0DE69BD00DCB0200867D02EFBF82E424ECEA455F16B58544A21C197AAF4D2E4732965026E5D17F6739C77C69D99B9914278E50E1F0597A6FD5CD72808BE417F3B9E0E7457915DAA85F X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojxgaGEV50waKIKyKuz5qlww== X-Mailru-Sender: 11C2EC085EDE56FA38FD4C59F7EFE407BD5B955C1BFFDAA97E812831CEB2AED3F2618984F9E88A09D51284F0FE6F529ABC7555A253F5B200DF104D74F62EE79D27EC13EC74F6107F4198E0F3ECE9B5443453F38A29522196 X-Mras: OK Subject: Re: [Tarantool-patches] [PATCH 2/2] Followup fix for embedded bytecode loader. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Maxim Kokryashkin via Tarantool-patches Reply-To: Maxim Kokryashkin Cc: max.kokryashkin@gmail.com, tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > + > +#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 >