Hi, Maxim thanks for the patch. See my comments below. Sergey On 11.04.2024 16:00, Maxim Kokryashkin wrote: > From: Mike Pall > > (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 > +#include > +#include > +#include > +#include > + > +/* 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 > + > +struct lua_State; > + > +/* Error-injected mock. */ > +struct lua_State *luaL_newstate(void) > +{ > + return NULL; > +} > -- > 2.44.0 >