[Tarantool-patches] [PATCH luajit] Fix command-line argv handling.
Sergey Kaplun
skaplun at tarantool.org
Thu Apr 11 12:47:20 MSK 2024
Hi, Maxim!
Thanks for the patch!
Please consider my comments below.
On 11.04.24, 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
> in `l_message`. However, `argv[0]` is not guaranteed to be
> non-NULL, so segmentation fault could occur. This patch fixes the
Typo: s/segmentation fault/the segmentation fault/
> 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.
Side note: Maybe it is worth to mention how it may be exploided via
CVE-2021-4034 [1][2].
Feel free to ignore, although.
>
> 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. Corresponding flavor
Typo: s/Corresponding/The corresponding/
> 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.
>
> Part of tarantool/tarantool#9924
> ---
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/fix-argv-handling
>
> .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 | 26 +++++++++++
> .../fix-argv-handling/CMakeLists.txt | 2 +
> .../fix-argv-handling/empty_argv_exec.c | 45 +++++++++++++++++++
> .../fix-argv-handling/mynewstate.c | 9 ++++
> 8 files changed, 114 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/empty_argv_exec.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..b3cc5ca1 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
> @@ -50,6 +50,8 @@ jobs:
> FLAVORFLAGS: -DLUAJIT_USE_GDBJIT=ON
> - FLAVOR: nounwind
> FLAVORFLAGS: -DLUAJIT_NO_UNWIND=ON
> + - FLAVOR: dynamic_build
> + FLAVORFLAGS: -DBUILDMODE=dynamic
Nit: Please sort entries alphabetically (as it is done for the list).
> exclude:
> - ARCH: ARM64
> GC64: OFF
> 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
<snipped>
> 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()
I suppose it is better to use skiplist in the else() branch above to be
passed to the CTest command (see -E). Hence, we can use it later to skip some
other tests as well based on their specific conditions without
mentioning them in this loop.
> +
> 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
When build like the following:
| cmake -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_APICHECK=ON -DLUA_USE_ASSERT=ON -DBUILDMODE=dynamic . && make -j
The test passes with reverted patch. What am I doing wrong?
The content of buf is the following:
": cannot create state: not enough memory\n"
| ctest -R fix-argv-h
| Test project /home/burii/reviews/luajit/argv-handling
| Start 62: test/tarantool-tests/fix-argv-handling.test.lua
| 1/1 Test #62: test/tarantool-tests/fix-argv-handling.test.lua ... Passed 0.01 sec
|
| 100% tests passed, 0 tests failed out of 1
> new file mode 100644
> index 00000000..ccb2f52e
> --- /dev/null
> +++ b/test/tarantool-tests/fix-argv-handling.test.lua
> @@ -0,0 +1,26 @@
> +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)
> +
> +local ffi = require('ffi')
> +local utils = require('utils')
> +
> +ffi.cdef[[
> +const char *empty_argv_exec(const char *path);
> +void free(void *ptr);
> +]]
> +local execlib = ffi.load('emptyargvexec')
> +local cmd = utils.exec.luabin(arg)
> +
> +-- Start the LuaJIT with an empty argv array and mocked `luaL_newstate`.
Nit: Comment width is more than 66 symbols.
> +local output = execlib.empty_argv_exec(cmd)
> +ffi.gc(output, ffi.C.free)
I get the following error:
| ERROR in finalizer: bad argument #1 to '?' (cannot convert 'const char *' to 'void *')
> +local output_str = ffi.string(output)
Why do we need ffi usage here instead of luaC call?
> +
> +-- Without the patch, the test fails with a segmentation fault instead of
> +-- returning an error.
> +test:like(output_str, '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..431da2ad
> --- /dev/null
> +++ b/test/tarantool-tests/fix-argv-handling/CMakeLists.txt
> @@ -0,0 +1,2 @@
> +BuildTestCLib(mynewstate mynewstate.c)
> +BuildTestCLib(libemptyargvexec empty_argv_exec.c)
> diff --git a/test/tarantool-tests/fix-argv-handling/empty_argv_exec.c b/test/tarantool-tests/fix-argv-handling/empty_argv_exec.c
> new file mode 100644
> index 00000000..d1f06014
> --- /dev/null
> +++ b/test/tarantool-tests/fix-argv-handling/empty_argv_exec.c
> @@ -0,0 +1,45 @@
> +#define _GNU_SOURCE
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/wait.h>
> +#include <unistd.h>
> +
> +#define BUF_SIZE 1024
> +#define CHECKED(call) \
> +do { \
> + int status = 0; \
> + status = (call); \
Minor: Why not `int status = (call);`?
> + if (status == -1) { \
> + perror(#call); \
> + exit(1); \
> +} \
Something strage with alignment here.
> +} while(0)
> +
> +const char *empty_argv_exec(const char *path)
> +{
> + int pipefds[2] = {};
> + char* const argv[] = {NULL};
Typo: s/char* /char */
> + CHECKED(pipe2(pipefds, O_CLOEXEC));
> +
> + pid_t pid = fork();
What should we do in the case of -1?
> + if (pid == 0) {
Should we now close pipefds[0] since we don't need it here?
> + /* Mock the `luaL_newstate` with an error-injected version. */
Comment width is more than 66 symbols.
> + setenv("LD_PRELOAD", "mynewstate.so", 1);
> + CHECKED(dup2(pipefds[1], 1));
> + CHECKED(dup2(pipefds[1], 2));
Minor: It is better to use `STDOUT_FILENO` and `STDERR_FILENO`.
Also, IINM, pipefds[1] may be closed too now, as excessive (we
duplicated stdout and stderr it it).
> + CHECKED(execvp(path, argv));
> + }
> +
> + close(pipefds[1]);
> + waitpid(pid, NULL, 0);
Should we check the return status of `waitpid()`?
> + /* 1Kb should be enough. */
> + char *buf = calloc(BUF_SIZE, sizeof(char));
Why not just malloc?
Also, since the size of the buffer isn't big, we can use stack memory
for our purposes (if we are using the LuaC API with `lua_pushstring()`).
> + if (buf == NULL) {
> + perror("calloc");
> + exit(1);
> + }
> + CHECKED(read(pipefds[0], buf, BUF_SIZE));
Should we now close pipefds[0]?
> + return buf;
> +}
> +
Nit: excess empty line.
> 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
>
[1]: https://github.com/advisories/GHSA-qgr2-xgqv-24x8
[2]: https://www.qualys.com/2022/01/25/cve-2021-4034/pwnkit.txt
--
Best regards,
Sergey Kaplun
More information about the Tarantool-patches
mailing list