[Tarantool-patches] [PATCH luajit v2] Fix command-line argv handling.
Sergey Kaplun
skaplun at tarantool.org
Thu Apr 11 16:26:18 MSK 2024
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
More information about the Tarantool-patches
mailing list