Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit] Fix command-line argv handling.
@ 2024-04-10 23:29 Maxim Kokryashkin via Tarantool-patches
  2024-04-11  9:47 ` Sergey Kaplun via Tarantool-patches
  0 siblings, 1 reply; 2+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2024-04-10 23:29 UTC (permalink / raw)
  To: tarantool-patches, skaplun, sergeyb

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

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
         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
@@ -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()
+
 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}
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..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`.
+local output = execlib.empty_argv_exec(cmd)
+ffi.gc(output, ffi.C.free)
+local output_str = ffi.string(output)
+
+-- 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); \
+	if (status == -1) { \
+		perror(#call); \
+		exit(1); \
+} \
+} while(0)
+
+const char *empty_argv_exec(const char *path)
+{
+	int pipefds[2] = {};
+	char* const  argv[] = {NULL};
+	CHECKED(pipe2(pipefds, O_CLOEXEC));
+
+	pid_t pid = fork();
+	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));
+		CHECKED(execvp(path, argv));
+	}
+
+	close(pipefds[1]);
+	waitpid(pid, NULL, 0);
+	/* 1Kb should be enough. */
+	char *buf = calloc(BUF_SIZE, sizeof(char));
+	if (buf == NULL) {
+		perror("calloc");
+		exit(1);
+	}
+	CHECKED(read(pipefds[0], buf, BUF_SIZE));
+	return buf;
+}
+
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


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Fix command-line argv handling.
  2024-04-10 23:29 [Tarantool-patches] [PATCH luajit] Fix command-line argv handling Maxim Kokryashkin via Tarantool-patches
@ 2024-04-11  9:47 ` Sergey Kaplun via Tarantool-patches
  0 siblings, 0 replies; 2+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-04-11  9:47 UTC (permalink / raw)
  To: Maxim Kokryashkin; +Cc: tarantool-patches

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

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2024-04-11  9:51 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-10 23:29 [Tarantool-patches] [PATCH luajit] Fix command-line argv handling Maxim Kokryashkin via Tarantool-patches
2024-04-11  9:47 ` Sergey Kaplun via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox