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

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

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