* Re: [Tarantool-patches] [PATCH luajit v2] Fix command-line argv handling.
2024-04-11 13:00 [Tarantool-patches] [PATCH luajit v2] Fix command-line argv handling Maxim Kokryashkin via Tarantool-patches
@ 2024-04-11 13:26 ` Sergey Kaplun via Tarantool-patches
2024-04-11 14:45 ` Maxim Kokryashkin via Tarantool-patches
2024-04-11 16:30 ` Sergey Bronnikov via Tarantool-patches
1 sibling, 1 reply; 5+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-04-11 13:26 UTC (permalink / raw)
To: Maxim Kokryashkin; +Cc: 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 <mike>
>
> (cherry-picked from commit 9ebebc9b588dc1516c988b46d829445f505fdc1f)
>
<snipped>
> ---
> 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
<snipped>
> diff --git a/src/luajit.c b/src/luajit.c
> index b63c92d1..dc142684 100644
> --- a/src/luajit.c
> +++ b/src/luajit.c
<snipped>
> 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
<snipped>
> @@ -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)
<snipped>
> 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 <fcntl.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/wait.h>
> +#include <unistd.h>
<snipped>
> +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));
<snipped>
> 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
<snipped>
> 2.44.0
>
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Tarantool-patches] [PATCH luajit v2] Fix command-line argv handling.
2024-04-11 13:00 [Tarantool-patches] [PATCH luajit v2] Fix command-line argv handling Maxim Kokryashkin via Tarantool-patches
2024-04-11 13:26 ` Sergey Kaplun via Tarantool-patches
@ 2024-04-11 16:30 ` Sergey Bronnikov via Tarantool-patches
1 sibling, 0 replies; 5+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-04-11 16:30 UTC (permalink / raw)
To: Maxim Kokryashkin, tarantool-patches, skaplun
[-- Attachment #1: Type: text/plain, Size: 15901 bytes --]
Hi, Maxim
thanks for the patch. See my comments below.
Sergey
On 11.04.2024 16:00, Maxim Kokryashkin wrote:
> From: Mike Pall <mike>
>
> (cherry-picked from commit 9ebebc9b588dc1516c988b46d829445f505fdc1f)
>
> Before the patch, there was a situation where `luaL_newstate`
> could fail in main and the `argv[0]` could be used as a progname
Nit: main and progname are the function and variable in code,
so I would wrap them by backticks.
> in `l_message`. However, `argv[0]` is not guaranteed to be
> non-NULL, so the segmentation fault could occur. This patch fixes
> the issue by using the predefined name in that case. Moreover, it
> refactors the `l_message`, so now there is no need to pass
> `pname` everywhere.
>
> The patch is tested with the help of the mocking of
> `luaL_newstate` by providing an error-injected implementation
> of it and preloading it. For preload to work, the LuaJIT must
> be built with dynamic build mode enabled. The corresponding
> flavor is added to the CI.
>
> 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.
This wording is confusing because target cannot be linked with anything.
I would rephrase to something like: tests in "tarantool-c-tests" cannot
be linked
with LuaJIT library built as shared library. Same for the message in
CMakeLists.txt.
> Since the Linux kernel 5.18-rc1 release, `argv` is 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/
>
> Part of tarantool/tarantool#9924
> ---
> 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
> @@ -34,7 +34,7 @@ jobs:
> BUILDTYPE: [Debug, Release]
> ARCH: [ARM64, x86_64]
> GC64: [ON, OFF]
> - FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind]
> + FLAVOR: [checkhook, dualnum, dynamic_build, gdbjit, nojit, nounwind]
Nit: "Dynamic build" can be replaced with "shared_lib" which is more
clear for my taste.
Feel free to ignore.
> include:
> - BUILDTYPE: Debug
> CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
> @@ -42,6 +42,8 @@ jobs:
> CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo
> - FLAVOR: dualnum
> FLAVORFLAGS: -DLUAJIT_NUMMODE=2
> + - FLAVOR: dynamic_build
> + FLAVORFLAGS: -DBUILDMODE=dynamic
> - FLAVOR: checkhook
> FLAVORFLAGS: -DLUAJIT_ENABLE_CHECKHOOK=ON
> - FLAVOR: nojit
> diff --git a/src/luajit.c b/src/luajit.c
> index b63c92d1..dc142684 100644
> --- a/src/luajit.c
> +++ b/src/luajit.c
> @@ -39,6 +39,7 @@
>
> static lua_State *globalL = NULL;
> static const char *progname = LUA_PROGNAME;
> +static char *empty_argv[2] = { NULL, NULL };
>
> #if !LJ_TARGET_CONSOLE
> static void lstop(lua_State *L, lua_Debug *ar)
> @@ -90,9 +91,9 @@ static void print_tools_usage(void)
> fflush(stderr);
> }
>
> -static void l_message(const char *pname, const char *msg)
> +static void l_message(const char *msg)
> {
> - if (pname) { fputs(pname, stderr); fputc(':', stderr); fputc(' ', stderr); }
> + if (progname) { fputs(progname, stderr); fputc(':', stderr); fputc(' ', stderr); }
> fputs(msg, stderr); fputc('\n', stderr);
> fflush(stderr);
> }
> @@ -102,7 +103,7 @@ static int report(lua_State *L, int status)
> if (status && !lua_isnil(L, -1)) {
> const char *msg = lua_tostring(L, -1);
> if (msg == NULL) msg = "(error object is not a string)";
> - l_message(progname, msg);
> + l_message(msg);
> lua_pop(L, 1);
> }
> return status;
> @@ -267,9 +268,8 @@ static void dotty(lua_State *L)
> lua_getglobal(L, "print");
> lua_insert(L, 1);
> if (lua_pcall(L, lua_gettop(L)-1, 0, 0) != 0)
> - l_message(progname,
> - lua_pushfstring(L, "error calling " LUA_QL("print") " (%s)",
> - lua_tostring(L, -1)));
> + l_message(lua_pushfstring(L, "error calling " LUA_QL("print") " (%s)",
> + lua_tostring(L, -1)));
> }
> }
> lua_settop(L, 0); /* clear stack */
> @@ -321,8 +321,7 @@ static int loadjitmodule(lua_State *L)
> lua_getfield(L, -1, "start");
> if (lua_isnil(L, -1)) {
> nomodule:
> - l_message(progname,
> - "unknown luaJIT command or jit.* modules not installed");
> + l_message("unknown luaJIT command or jit.* modules not installed");
> return 1;
> }
> lua_remove(L, -2); /* Drop module table. */
> @@ -382,7 +381,7 @@ static int runtoolcmd(lua_State *L, const char *tool_name)
> if (msg) {
> if (!strncmp(msg, "module ", 7))
> msg = "unknown luaJIT command or tools not installed";
> - l_message(progname, msg);
> + l_message(msg);
> }
> return 1;
> }
> @@ -566,7 +565,6 @@ static int pmain(lua_State *L)
> int argn;
> int flags = 0;
> globalL = L;
> - if (argv[0] && argv[0][0]) progname = argv[0];
>
> LUAJIT_VERSION_SYM(); /* Linker-enforced version check. */
>
> @@ -622,9 +620,11 @@ static int pmain(lua_State *L)
> int main(int argc, char **argv)
> {
> int status;
> - lua_State *L = lua_open();
> + lua_State *L;
> + if (!argv[0]) argv = empty_argv; else if (argv[0][0]) progname = argv[0];
> + L = lua_open(); /* create state */
> if (L == NULL) {
> - l_message(argv[0], "cannot create state: not enough memory");
> + l_message("cannot create state: not enough memory");
> return EXIT_FAILURE;
> }
> smain.argc = argc;
> 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.
> +if(BUILDMODE STREQUAL "dynamic")
> + message("Dynamic build mode, tarantool-c-tests suite is empty")
> + return()
> +endif()
This is confusing ("Add tarantool-c-tests..." and then "test suite is
empty"):
[1] ~/sources/MRG/tarantool/third_party/luajit$ cmake -S . -B build
-DBUILDMODE=dynamic
-- [SetVersion] Reading version from VCS: v2.1.0-beta3-528-g964e26a2
-- [SetBuildParallelLevel] CMAKE_BUILD_PARALLEL_LEVEL is 8
-- Add test suite LuaJIT-tests
-- Add test suite PUC-Rio-Lua-5.1-tests
-- Add test suite lua-Harness-tests
-- Add test suite tarantool-c-tests
Dynamic build mode, tarantool-c-tests suite is empty
-- Add test suite tarantool-tests
-- Configuring done (0.1s)
-- Generating done (0.1s)
I propose to add an empty target and put condition
before add_test_suite_target:
diff --git a/test/tarantool-c-tests/CMakeLists.txt
b/test/tarantool-c-tests/CMakeLists.txt
index 5f789ad8..63525682 100644
--- a/test/tarantool-c-tests/CMakeLists.txt
+++ b/test/tarantool-c-tests/CMakeLists.txt
@@ -21,6 +21,17 @@ AppendFlags(TESTS_C_FLAGS
"-D__LJ_TEST_DIR__='\"${CMAKE_CURRENT_SOURCE_DIR}\"'")
set(TEST_SUITE_NAME "tarantool-c-tests")
+# 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.
+if(BUILDMODE STREQUAL "dynamic")
+ set(MSG "Dynamic build mode, ${TEST_SUITE_NAME} suite is empty")
+ add_custom_target(${TEST_SUITE_NAME}
+ COMMAND ${CMAKE_COMMAND} -E cmake_echo_color --red ${MSG}
+ )
+ add_custom_target(${TEST_SUITE_NAME}-deps)
+ return()
+endif()
+
+
# The proxy CMake target with all targets that build C tests.
# This is needed because targets for each C test are generated
# at the same time as CMake tests, and all prerequisites must
@@ -36,14 +47,6 @@ 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.
-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})
> +
> 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
> @@ -40,6 +40,10 @@ add_subdirectory(lj-flush-on-trace)
> add_subdirectory(lj-1004-oom-error-frame)
> add_subdirectory(lj-1066-fix-cur_L-after-coroutine-resume)
>
> +if(BUILDMODE STREQUAL "dynamic")
> + add_subdirectory(fix-argv-handling)
> +endif()
> +
> # The part of the memory profiler toolchain is located in tools
> # directory, jit, profiler, and bytecode toolchains are located
> # in src/ directory, jit/vmdef.lua is autogenerated file also
> @@ -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")
> + continue()
> + endif()
> +
> set(test_title "test/${TEST_SUITE_NAME}/${test_name}")
> add_test(NAME ${test_title}
> COMMAND ${LUAJIT_TEST_COMMAND} ${test_path}
I would not touch this loop. After adding a number quirks for tests like
above
the loop body will become unreadable. I propose to add such quirks right
after the loop
and mark such tests as disabled using test properties. It is sad, but
CTest cannot mark test as skipped.
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -127,12 +127,6 @@ 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")
- continue()
- endif()
-
set(test_title "test/${TEST_SUITE_NAME}/${test_name}")
add_test(NAME ${test_title}
COMMAND ${LUAJIT_TEST_COMMAND} ${test_path}
@@ -147,3 +141,12 @@ foreach(test_path ${tests})
DEPENDS tarantool-tests-deps
)
endforeach()
+
+# Test's properties.
+
+if (NOT BUILDMODE STREQUAL "dynamic")
+ set(test_title "test/tarantool-tests/fix-argv-handling.test.lua")
+ set_tests_properties(${test_title} PROPERTIES
+ DISABLED True
+ )
+endif()
> 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
> +-- 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
> +-- returning an error.
> +test:like(output, 'cannot create state', 'correct argv handling')
> +test:done(true)
> diff --git a/test/tarantool-tests/fix-argv-handling/CMakeLists.txt b/test/tarantool-tests/fix-argv-handling/CMakeLists.txt
> new file mode 100644
> index 00000000..c37bded7
> --- /dev/null
> +++ b/test/tarantool-tests/fix-argv-handling/CMakeLists.txt
> @@ -0,0 +1,2 @@
> +BuildTestCLib(mynewstate mynewstate.c)
> +BuildTestCLib(execlib execlib.c)
> 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"
> +
> +#define _GNU_SOURCE
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/wait.h>
> +#include <unistd.h>
> +
> +/* 1Kb should be enough. */
> +#define BUF_SIZE 1024
> +#define CHECKED(call) \
> +do { \
> + if ((call) == -1) { \
> + perror(#call); \
> + exit(1); \
> + } \
> +} while(0)
> +
> +static int empty_argv_exec(struct lua_State *L)
> +{
> + const char *path = luaL_checkstring(L, -1);
> + int pipefds[2] = {};
> + char *const argv[] = {NULL};
> + char buf[BUF_SIZE];
> +
> + CHECKED(pipe2(pipefds, O_CLOEXEC));
> +
> + pid_t pid = fork();
> + CHECKED(pid);
> +
> + if (pid == 0) {
> + /*
> + * Mock the `luaL_newstate` with
> + * an error-injected version.
> + */
> + setenv("LD_PRELOAD", "mynewstate.so", 1);
> + CHECKED(dup2(pipefds[1], 1));
> + CHECKED(dup2(pipefds[1], 2));
> + /*
> + * Pipes are closed on the exec
> + * call because of the O_CLOEXEC flag.
> + */
> + CHECKED(execvp(path, argv));
> + }
> +
> + close(pipefds[1]);
> + CHECKED(waitpid(pid, NULL, 0));
> +
> +
> + CHECKED(read(pipefds[0], buf, BUF_SIZE));
> + close(pipefds[0]);
> +
> + lua_pushstring(L, buf);
> + return 1;
> +}
> +
> +static const struct luaL_Reg execlib[] = {
> + {"empty_argv_exec", empty_argv_exec},
> + {NULL, NULL}
> +};
> +
> +LUA_API int luaopen_execlib(lua_State *L)
> +{
> + luaL_register(L, "execlib", execlib);
> + return 1;
> +}
> 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
> @@ -0,0 +1,9 @@
> +#include <stddef.h>
> +
> +struct lua_State;
> +
> +/* Error-injected mock. */
> +struct lua_State *luaL_newstate(void)
> +{
> + return NULL;
> +}
> --
> 2.44.0
>
[-- Attachment #2: Type: text/html, Size: 18173 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread