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 F1D69A8D840; Thu, 11 Apr 2024 16:30:22 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org F1D69A8D840 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1712842223; bh=nH8OE0OQL1BJgGSEEVqwRz1BvQSMPFG5bp97IjnXE8Y=; 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=Joz4u6dEC8H3gCIXi3GVcKEAUDPMiL0hj3rwyVsV9wHFPcFaSwS1faOxIi1TurlWd c3pSvl9ZFjXlKAZMY5Xt4oJ/W130aaXK78fFjb8c0wMKvMlcpEJpXNZuC6CvRGJKM8 OHCPzWVDrVn56FiIQr5N474Au7IA87LSJf9pTZO4= Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [95.163.41.87]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id EC8AFA8D840 for ; Thu, 11 Apr 2024 16:30:21 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org EC8AFA8D840 Received: by smtp51.i.mail.ru with esmtpa (envelope-from ) id 1ruuVP-00000004h3h-1Nql; Thu, 11 Apr 2024 16:30:20 +0300 Date: Thu, 11 Apr 2024 16:26:18 +0300 To: Maxim Kokryashkin Message-ID: References: <20240411130057.1144616-1-m.kokryashkin@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240411130057.1144616-1-m.kokryashkin@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9D327C87852EB66D3FD55D334CC1A2A9DF13713DBDDE85F9F182A05F538085040C916A79B831FDEE0D4FF92D56319F197FD619772F8E9049CB5F37AF0D8324C51ECAC19F6B383DD1F X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7BB17EE3498E810FEEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637790A9327A9AFEF4F8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8A9E098BB6DC7BC47EB82983089D2BBFA8859B0C839FB60B1CC7F00164DA146DAFE8445B8C89999728AA50765F7900637CAEE156C82D3D7D9389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8062BEEFFB5F8EA3EF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA7E827F84554CEF5019E625A9149C048EE33AC447995A7AD181150BA43C84913FC3A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E7356436AE5DD6441DC7C4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A57CFB078B65A0BFF65002B1117B3ED6962BA3CECC061AAB48D57BAD45EC4C5DE1823CB91A9FED034534781492E4B8EEAD8D8BB953E4894305BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF947275C9AC3249F4E633832870169B60BE21F9C749B9AB0D250AF628A2883EB15C1052AD1686EE7343A9A4803F78FBB9D9B305425F97EB55A63607FB69A77DDCD393A095924560CA5F4332CA8FE04980913E6812662D5F2A5EAB5682573093F7837F15F2B5E4A70B33F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioja81YqLNwFpnJ7ClGkIsjgg== X-DA7885C5: E80EFE2179D01CF0F255D290C0D534F9B4C6438C554E8F7C7A10C4B1AD5939AE8442934AAA972C325B1A4C17EAA7BC4BEF2421ABFA55128DAF83EF9164C44C7E X-Mailru-Sender: 689FA8AB762F7393590D8C940224AE339196070B625B7AA0DB371A47D5160AC04A04C1FD7B8C87ECE49D44BB4BD9522A059A1ED8796F048DB274557F927329BE89D5A3BC2B10C37545BD1C3CC395C826B4A721A3011E896F X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v2] Fix command-line argv handling. 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: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Maxim! Thanks for the fixes and clarifications. LGTM, except a few nits below. On 11.04.24, Maxim Kokryashkin wrote: > From: Mike Pall > > (cherry-picked from commit 9ebebc9b588dc1516c988b46d829445f505fdc1f) > > --- > Branch: https://github.com/tarantool/luajit/tree/fckxorg/fix-argv-handling > > Changes in v2: > - Fixed comments as per review by Sergey Kaplun > > .github/workflows/exotic-builds-testing.yml | 4 +- > src/luajit.c | 24 +++---- > test/tarantool-c-tests/CMakeLists.txt | 8 +++ > test/tarantool-tests/CMakeLists.txt | 9 +++ > .../fix-argv-handling.test.lua | 25 +++++++ > .../fix-argv-handling/CMakeLists.txt | 2 + > .../fix-argv-handling/execlib.c | 68 +++++++++++++++++++ > .../fix-argv-handling/mynewstate.c | 9 +++ > 8 files changed, 136 insertions(+), 13 deletions(-) > create mode 100644 test/tarantool-tests/fix-argv-handling.test.lua > create mode 100644 test/tarantool-tests/fix-argv-handling/CMakeLists.txt > create mode 100644 test/tarantool-tests/fix-argv-handling/execlib.c > create mode 100644 test/tarantool-tests/fix-argv-handling/mynewstate.c > > diff --git a/.github/workflows/exotic-builds-testing.yml b/.github/workflows/exotic-builds-testing.yml > index 859603bd..ae54de2e 100644 > --- a/.github/workflows/exotic-builds-testing.yml > +++ b/.github/workflows/exotic-builds-testing.yml > diff --git a/src/luajit.c b/src/luajit.c > index b63c92d1..dc142684 100644 > --- a/src/luajit.c > +++ b/src/luajit.c > diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt > index 30d174bb..7ae440e2 100644 > --- a/test/tarantool-c-tests/CMakeLists.txt > +++ b/test/tarantool-c-tests/CMakeLists.txt > @@ -36,6 +36,14 @@ add_test_suite_target(tarantool-c-tests > DEPENDS libluajit libtest tarantool-c-tests-build > ) > > +# XXX: The tarantool-c-tests target cannot be linked with the LuaJIT > +# library when it is built as shared. The test suite is disabled for > +# the dynamic build mode. Nit: comment width is more than 66 symbols. > +if(BUILDMODE STREQUAL "dynamic") > + message("Dynamic build mode, tarantool-c-tests suite is empty") > + return() > +endif() > + > set(CTEST_SRC_SUFFIX ".test.c") > file(GLOB tests "${CMAKE_CURRENT_SOURCE_DIR}/*${CTEST_SRC_SUFFIX}") > foreach(test_source ${tests}) > diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt > index 56660932..05deb534 100644 > --- a/test/tarantool-tests/CMakeLists.txt > +++ b/test/tarantool-tests/CMakeLists.txt > @@ -123,6 +127,11 @@ add_test_suite_target(tarantool-tests > file(GLOB_RECURSE tests ${CMAKE_CURRENT_SOURCE_DIR} "*${LUA_TEST_SUFFIX}") > foreach(test_path ${tests}) > get_filename_component(test_name ${test_path} NAME) > + > + if(test_name STREQUAL "fix-argv-handling.test.lua" AND NOT BUILDMODE STREQUAL "dynamic") Nit: Line width is more than 80 symbols. Side note: Such skip looks clumsy, but for now we can leave it as the most robust solution. > + continue() > + endif() > + > set(test_title "test/${TEST_SUITE_NAME}/${test_name}") > add_test(NAME ${test_title} > COMMAND ${LUAJIT_TEST_COMMAND} ${test_path} > diff --git a/test/tarantool-tests/fix-argv-handling.test.lua b/test/tarantool-tests/fix-argv-handling.test.lua > new file mode 100644 > index 00000000..57e5f169 > --- /dev/null > +++ b/test/tarantool-tests/fix-argv-handling.test.lua > @@ -0,0 +1,25 @@ > +local tap = require('tap') > +local test = tap.test('fix-argv-handling'):skipcond({ > + ['DYLD_INSERT_LIBRARIES does not work on macOS'] = jit.os == 'OSX', > +}) > + > +test:plan(1) > + > +-- XXX: Since the Linux kernel 5.18-rc1 release, `argv` is Nit: Comment line looks underfilled > +-- forced to a single empty string if it is empty [1], so > +-- the issue is not reproducible on new kernels. > +-- > +-- [1]: https://lore.kernel.org/all/20220201000947.2453721-1-keescook@chromium.org/ > + > +local utils = require('utils') > +local execlib = require('execlib') > +local cmd = utils.exec.luabin(arg) > + > +-- Start the LuaJIT with an empty argv array and mocked > +-- `luaL_newstate`. > +local output = execlib.empty_argv_exec(cmd) > + > +-- Without the patch, the test fails with a segmentation fault instead of Nit: comment line width is more than 66 symbols. > +-- returning an error. > +test:like(output, 'cannot create state', 'correct argv handling') > +test:done(true) > diff --git a/test/tarantool-tests/fix-argv-handling/execlib.c b/test/tarantool-tests/fix-argv-handling/execlib.c > new file mode 100644 > index 00000000..ef8217d4 > --- /dev/null > +++ b/test/tarantool-tests/fix-argv-handling/execlib.c > @@ -0,0 +1,68 @@ > +#include "lua.h" > +#include "lauxlib.h" Now I got the following warning during the build. | /home/burii/reviews/luajit/argv-handling/test/tarantool-tests/fix-argv-handling/execlib.c:28:17: | warning: implicit declaration of function 'pipe2'; did you mean 'pipe'? [-Wimplicit-function-declaration] | 28 | CHECKED(pipe2(pipefds, O_CLOEXEC)); | | ^~~~~ | /home/burii/reviews/luajit/argv-handling/test/tarantool-tests/fix-argv-handling/execlib.c:15:14: note: in definition of macro 'CHECKED' | 15 | if ((call) == -1) { \ | | ^~ I suppose _GNU_SOURCE should be defined before any onther includes. > + > +#define _GNU_SOURCE > +#include > +#include > +#include > +#include > +#include > +static int empty_argv_exec(struct lua_State *L) > +{ > + const char *path = luaL_checkstring(L, -1); > + int pipefds[2] = {}; > + char *const argv[] = {NULL}; Typo: s/ / / > + char buf[BUF_SIZE]; > + > + CHECKED(pipe2(pipefds, O_CLOEXEC)); > + > + pid_t pid = fork(); > + CHECKED(pid); > + > + if (pid == 0) { > + /* > + * Mock the `luaL_newstate` with Nit: Comment line looks underfilled. > + * an error-injected version. > + */ > + setenv("LD_PRELOAD", "mynewstate.so", 1); > + CHECKED(dup2(pipefds[1], 1)); > + CHECKED(dup2(pipefds[1], 2)); Why don't use `STDOUT_FILENO` and `STDERR_FILENO`? > + /* > + * Pipes are closed on the exec > + * call because of the O_CLOEXEC flag. Side note: Thanks for the clarification. PEBKAC. > + */ > + CHECKED(execvp(path, argv)); > + } > + > + close(pipefds[1]); > + CHECKED(waitpid(pid, NULL, 0)); > + Nit: Excess empty line. > + > + CHECKED(read(pipefds[0], buf, BUF_SIZE)); > diff --git a/test/tarantool-tests/fix-argv-handling/mynewstate.c b/test/tarantool-tests/fix-argv-handling/mynewstate.c > new file mode 100644 > index 00000000..cf4a67e7 > --- /dev/null > +++ b/test/tarantool-tests/fix-argv-handling/mynewstate.c > 2.44.0 > -- Best regards, Sergey Kaplun