From: Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: tarantool-patches@dev.tarantool.org, skaplun@tarantool.org, sergeyb@tarantool.org Subject: [Tarantool-patches] [PATCH luajit v2] Fix command-line argv handling. Date: Thu, 11 Apr 2024 16:00:54 +0300 [thread overview] Message-ID: <20240411130057.1144616-1-m.kokryashkin@tarantool.org> (raw) 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 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. 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] 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() + 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} 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
next reply other threads:[~2024-04-11 13:01 UTC|newest] Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top 2024-04-11 13:00 Maxim Kokryashkin via Tarantool-patches [this message] 2024-04-11 13:26 ` Sergey Kaplun via Tarantool-patches 2024-04-11 14:45 ` Maxim Kokryashkin via Tarantool-patches 2024-04-11 14:57 ` Sergey Kaplun via Tarantool-patches 2024-04-11 16:30 ` Sergey Bronnikov via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20240411130057.1144616-1-m.kokryashkin@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=max.kokryashkin@gmail.com \ --cc=sergeyb@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit v2] Fix command-line argv handling.' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox