Re: [Tarantool-patches] [PATCH luajit v2] Fix command-line argv handling.

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 15901 bytes --]

Hi, Maxim

thanks for the patch. See my comments below.


Sergey

On 11.04.2024 16:00, 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

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 <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
>

[-- Attachment #2: Type: text/html, Size: 18173 bytes --]