Re: [Tarantool-patches] [PATCH luajit 2/3] Fix potential file descriptor leak in luaL_loadfile*().

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

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

Hello, Sergey,

the test is passed when CMake option -DLUAJIT_USE_VALGRIND=ON is used and

patch with fix is reverted.

Sergey

On 6/5/25 08:44, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Reported by Assumeru.
>
> (cherry picked from commit 19db4e9b7c5e19398286adb4d953a4874cc39ae0)
>
> `loadfile()` doesn't close the fd in case when `fopen()` results
> successfully, but the call `lua_pushfstring()` raises the "not enough
> memory" error on creation of the filename string (started with @).
>
> This patch fixes that behaviour by moving the string creation before the
> `fopen()` call to avoid descriptor leak.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#11278
> ---
>   src/lj_load.c                                 |  3 ++-
>   .../lj-1249-loadfile-fd-leak.test.lua         | 27 +++++++++++++++++++
>   test/tarantool-tests/utils/CMakeLists.txt     |  1 +
>   3 files changed, 30 insertions(+), 1 deletion(-)
>   create mode 100644 test/tarantool-tests/lj-1249-loadfile-fd-leak.test.lua
>
> diff --git a/src/lj_load.c b/src/lj_load.c
> index 19ac6ba2..a6d0b464 100644
> --- a/src/lj_load.c
> +++ b/src/lj_load.c
> @@ -88,12 +88,13 @@ LUALIB_API int luaL_loadfilex(lua_State *L, const char *filename,
>     int status;
>     const char *chunkname;
>     if (filename) {
> +    chunkname = lua_pushfstring(L, "@%s", filename);
>       ctx.fp = fopen(filename, "rb");
>       if (ctx.fp == NULL) {
> +      L->top--;
>         lua_pushfstring(L, "cannot open %s: %s", filename, strerror(errno));
>         return LUA_ERRFILE;
>       }
> -    chunkname = lua_pushfstring(L, "@%s", filename);
>     } else {
>       ctx.fp = stdin;
>       chunkname = "=stdin";
> diff --git a/test/tarantool-tests/lj-1249-loadfile-fd-leak.test.lua b/test/tarantool-tests/lj-1249-loadfile-fd-leak.test.lua
> new file mode 100644
> index 00000000..c1a45724
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1249-loadfile-fd-leak.test.lua
> @@ -0,0 +1,27 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate fd leakage in case of OOM during the
> +-- `loadfile()` call. The test fails before the patch when run
> +-- under Valgrind with the `--track-fds=yes` option.
> +-- See also,https://github.com/LuaJIT/LuaJIT/issues/1249.
> +local test = tap.test('lj-1249-loadfile-fd-leak')
> +
> +test:plan(2)
> +
> +local allocinject = require('allocinject')
> +
> +allocinject.enable_null_alloc()
> +
> +-- Just use the /dev/null as the surely available file.
> +-- OOM is due to the creation of the string "@/dev/null" as the
> +-- filename to be stored.
> +local res, errmsg = pcall(loadfile, '/dev/null')
> +
> +allocinject.disable()
> +
> +-- Sanity checks.
> +test:ok(not res, 'correct status, OOM on filename creation')
> +test:like(errmsg, 'not enough memory',
> +          'correct error message, OOM on filename creation')
> +
> +test:done(true)
> diff --git a/test/tarantool-tests/utils/CMakeLists.txt b/test/tarantool-tests/utils/CMakeLists.txt
> index 15871934..d44a8802 100644
> --- a/test/tarantool-tests/utils/CMakeLists.txt
> +++ b/test/tarantool-tests/utils/CMakeLists.txt
> @@ -2,6 +2,7 @@ list(APPEND tests
>     lj-1166-error-stitch-oom-ir-buff.test.lua
>     lj-1166-error-stitch-oom-snap-buff.test.lua
>     lj-1247-fin-tab-rehashing-on-trace.test.lua
> +  lj-1249-loadfile-fd-leak.test.lua
>     lj-1298-oom-on-concat-recording.test.lua
>   )
>   BuildTestCLib(allocinject allocinject.c "${tests}")

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