Skip to content

Commit

Permalink
Fix command-line argv handling.
Browse files Browse the repository at this point in the history
(cherry-picked from commit 9ebebc9)

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 the program name
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 only for x86_64
Debug build to avoid CI jobs outgrowing.

The <gh-8594-sysprof-ffunc-crash.test.c> inspects internal symbols from
the LuaJIT static library, so it can't be linked for shared build. The
test 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. You may inspect the `dmesg` log for the corresponding warning
entrance.

Maxim Kokryashkin:
* added the description and the test for the problem

[1]: https://lore.kernel.org/all/[email protected]/

Part of tarantool/tarantool#10709

Reviewed-by: Sergey Kaplun <[email protected]>
Reviewed-by: Sergey Bronnikov <[email protected]>
Signed-off-by: Sergey Kaplun <[email protected]>
  • Loading branch information
Mike Pall authored and Buristan committed Jan 28, 2025
1 parent c698ff5 commit 026a0e4
Show file tree
Hide file tree
Showing 8 changed files with 144 additions and 13 deletions.
10 changes: 9 additions & 1 deletion .github/workflows/exotic-builds-testing.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,16 @@ jobs:
BUILDTYPE: [Debug, Release]
ARCH: [ARM64, x86_64]
GC64: [ON, OFF]
FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind, tablebump]
FLAVOR: [checkhook, dualnum, dynamic, gdbjit, nojit, nounwind, tablebump]
include:
- BUILDTYPE: Debug
CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
- BUILDTYPE: Release
CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo
- FLAVOR: dualnum
FLAVORFLAGS: -DLUAJIT_NUMMODE=2
- FLAVOR: dynamic
FLAVORFLAGS: -DBUILDMODE=dynamic
- FLAVOR: checkhook
FLAVORFLAGS: -DLUAJIT_ENABLE_CHECKHOOK=ON
- FLAVOR: nojit
Expand All @@ -58,6 +60,12 @@ jobs:
# DUALNUM is default for ARM64, no need for additional testing.
- FLAVOR: dualnum
ARCH: ARM64
# There is no need to test any scenarios except the most
# common one for the shared library build.
- FLAVOR: dynamic
ARCH: ARM64
- FLAVOR: dynamic
BUILDTYPE: Release
# With table bump optimization enabled (and due to our modification
# related to metrics), some offsets in GG_State stop fitting in 12bit
# immediate. Hence, the build failed due to the DASM error
Expand Down
24 changes: 12 additions & 12 deletions src/luajit.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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);
}
Expand All @@ -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;
Expand Down Expand Up @@ -268,9 +269,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 */
Expand Down Expand Up @@ -322,8 +322,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. */
Expand Down Expand Up @@ -383,7 +382,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;
}
Expand Down Expand Up @@ -567,7 +566,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. */

Expand Down Expand Up @@ -623,9 +621,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;
Expand Down
8 changes: 8 additions & 0 deletions test/tarantool-c-tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,14 @@ file(GLOB tests "${CMAKE_CURRENT_SOURCE_DIR}/*${CTEST_SRC_SUFFIX}")
foreach(test_source ${tests})
# Get test name without suffix. Needed to set OUTPUT_NAME.
get_filename_component(exe ${test_source} NAME_WE)

# Test requires static build, since it inspects internal
# symbols.
if(exe STREQUAL "gh-8594-sysprof-ffunc-crash" AND
BUILDMODE STREQUAL "dynamic")
continue()
endif()

add_executable(${exe} EXCLUDE_FROM_ALL ${test_source})
target_include_directories(${exe} PRIVATE
${CMAKE_CURRENT_SOURCE_DIR}
Expand Down
10 changes: 10 additions & 0 deletions test/tarantool-tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ add_subdirectory(profilers/gh-5813-resolving-of-c-symbols/gnuhash)
add_subdirectory(profilers/gh-5813-resolving-of-c-symbols/hash)
add_subdirectory(profilers/gh-5813-resolving-of-c-symbols/stripped)

if(BUILDMODE STREQUAL "dynamic")
add_subdirectory(fix-argv-handling)
endif()

# JIT, profiler, and bytecode toolchains are located in the <src/>
# directory, <jit/vmdef.lua> is the autogenerated file also
# located in the <src/> directory, but in the scope of the binary
Expand Down Expand Up @@ -99,6 +103,12 @@ if(LUAJIT_USE_VALGRIND)
list(APPEND LUA_TEST_ENV_MORE LUAJIT_TEST_USE_VALGRIND=1)
endif()

# Needed to skip the <fix-argv-handling.test.lua> for a
# non-dynamic build.
if(BUILDMODE STREQUAL "dynamic")
list(APPEND LUA_TEST_ENV_MORE LUAJIT_BUILDMODE=dynamic)
endif()

set(TEST_SUITE_NAME "tarantool-tests")

# XXX: The call produces both test and target
Expand Down
26 changes: 26 additions & 0 deletions test/tarantool-tests/fix-argv-handling.test.lua
Original file line number Diff line number Diff line change
@@ -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',
['Skipped for static build'] = os.getenv('LUAJIT_BUILDMODE') ~= 'dynamic',
})

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/[email protected]/

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)
3 changes: 3 additions & 0 deletions test/tarantool-tests/fix-argv-handling/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
get_filename_component(test_name ${CMAKE_CURRENT_SOURCE_DIR} NAME)
BuildTestCLib(mynewstate mynewstate.c ${test_name}.test.lua)
BuildTestCLib(execlib execlib.c ${test_name}.test.lua)
67 changes: 67 additions & 0 deletions test/tarantool-tests/fix-argv-handling/execlib.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
#define _GNU_SOURCE
#include "lua.h"
#include "lauxlib.h"

#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], STDOUT_FILENO));
CHECKED(dup2(pipefds[1], STDERR_FILENO));
/*
* 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;
}
9 changes: 9 additions & 0 deletions test/tarantool-tests/fix-argv-handling/mynewstate.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#include <stddef.h>

struct lua_State;

/* Error-injected mock. */
struct lua_State *luaL_newstate(void)
{
return NULL;
}

0 comments on commit 026a0e4

Please sign in to comment.