From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 396F3A72B41; Thu, 11 Apr 2024 12:51:25 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 396F3A72B41 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1712829085; bh=D6/iivW0tIrFd0QyPvMr6uZDgI7Naxf/qecHNi0MgOA=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=p+zFIA1vgweRH5SsRMj/FfUS+p7Yw4D4mpHgx+rGLZF0EnO36tvtjGaysy7YG8/VV jvL+hFQl44c3JcMSsKK3wZFDvNj4GbXYpj8ZaDRb6DazNfxDM3rx1LesYbTPiBAMpp HBTjlCrr6cE5e7nKltMAVdrfI1ejih39BHKDwwUc= Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [95.163.41.80]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 4B941A72B57 for ; Thu, 11 Apr 2024 12:51:23 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4B941A72B57 Received: by smtp39.i.mail.ru with esmtpa (envelope-from ) id 1rur5V-00000009Lrh-3jVl; Thu, 11 Apr 2024 12:51:22 +0300 Date: Thu, 11 Apr 2024 12:47:20 +0300 To: Maxim Kokryashkin Message-ID: References: <20240410232933.969244-1-m.kokryashkin@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240410232933.969244-1-m.kokryashkin@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9D327C87852EB66D31116567C8E3CFDFF0E1C6585A2586D0D182A05F5380850404A4B341E7C0902CC5D1BE6A8D71B10A52C89923F6B654437483DCBC56C8C17E6AF0056C46A67EA41 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE76C0A440987CA342DC2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE788A2BECDB72E1542EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38B73AB1701401CD8712AB0CE5FD875D337ACA1346F51FD1D3FEA5E9196CB037C67A471835C12D1D9774AD6D5ED66289B5278DA827A17800CE70F3DDF2BBF19B93A9FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C3E478A468B35FE767117882F4460429728AD0CFFFB425014E868A13BD56FB6657E2021AF6380DFAD1A18204E546F3947C1D471462564A2E192E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89FB26E97DCB74E62526D8C47C27EEC5E9FB5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A5E838001C807EAA095002B1117B3ED696C00D88C8F13BEF04D57BAD45EC4C5DE1823CB91A9FED034534781492E4B8EEAD3CCD70CEBBF18A22BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF7CD46D6358F39C340E3BA85EECEB61FD50F754D048E9E54E4B87200B32FC833417C141E43CE0FC0B43A9A4803F78FBB916E3041924F0F719EFA718C63B1640B8C687BCA0763655ED5F4332CA8FE04980913E6812662D5F2A5EAB5682573093F7837F15F2B5E4A70B33F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioja81YqLNwFpl6IZRxg6tsmQ== X-DA7885C5: 3DBD87C3DB424386F255D290C0D534F91434C8A0BFA8ECD67A10C4B1AD5939AEF95C0F9CC3EB97A55B1A4C17EAA7BC4BEF2421ABFA55128DAF83EF9164C44C7E X-Mailru-Sender: 689FA8AB762F7393590D8C940224AE330E51CEA53DD52780609024FC8E4C2C9BFBA74F2E4235A9AAE49D44BB4BD9522A059A1ED8796F048DB274557F927329BE89D5A3BC2B10C37545BD1C3CC395C826B4A721A3011E896F X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Fix command-line argv handling. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Maxim! Thanks for the patch! Please consider my comments below. On 11.04.24, Maxim Kokryashkin wrote: > From: Mike Pall > > (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 > 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 > 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 > +#include > +#include > +#include > +#include > + > +#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 > -- > 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