[Tarantool-patches] [PATCH luajit 0/3] Fix descriptor leak in loadfile

[Tarantool-patches] [PATCH luajit 0/3] Fix descriptor leak in loadfile

[Sergey Kaplun via Tarantool-patches]

This patch set fixes the possible descriptor leak in the `loadfile()`
in case of the error.
The first patch adds the Valgrind configuration to the corresponding CI
workflow to test the descriptor leakage.
The last two fix two similar scenarios of the leak.

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1249-loadfile-fd-leak
Related issues:
* https://github.com/LuaJIT/LuaJIT/issues/1249
* https://github.com/tarantool/tarantool/issues/11278

Mike Pall (2):
  Fix potential file descriptor leak in luaL_loadfile*().
  Fix another potential file descriptor leak in luaL_loadfile*().

Sergey Kaplun (1):
  ci: add track-fds Valgrind scenario

 .github/workflows/valgrind-testing.yaml       |  9 ++-
 src/lj_load.c                                 | 19 +++---
 .../lj-1249-loadfile-fd-leak.test.lua         | 59 +++++++++++++++++++
 test/tarantool-tests/utils/CMakeLists.txt     |  1 +
 4 files changed, 78 insertions(+), 10 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1249-loadfile-fd-leak.test.lua

-- 
2.49.0

[Tarantool-patches] [PATCH luajit 1/3] ci: add track-fds Valgrind scenario

[Sergey Kaplun via Tarantool-patches]

This patch adds a new field, track-fds [1], in the Valgrind workflow
matrix to detect descriptor leakage in the tests.

[1]: https://valgrind.org/docs/manual/manual-core.html#opt.track-fds

Needed for tarantool/tarantool#11278
---
 .github/workflows/valgrind-testing.yaml | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/.github/workflows/valgrind-testing.yaml b/.github/workflows/valgrind-testing.yaml
index e6606478..b3c7bc80 100644
--- a/.github/workflows/valgrind-testing.yaml
+++ b/.github/workflows/valgrind-testing.yaml
@@ -38,7 +38,11 @@ jobs:
         # Therefore, testing on this platform is currently
         # disabled.
         BUILDTYPE: [Debug, Release]
-        VALGRIND_SCENARIO: [full, malloc-free-fill-0x00, malloc-free-fill-0xff]
+        VALGRIND_SCENARIO:
+          - full
+          - malloc-free-fill-0x00
+          - malloc-free-fill-0xff
+          - track-fds
         include:
           - BUILDTYPE: Debug
             CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
@@ -59,6 +63,9 @@ jobs:
           - VALGRIND_SCENARIO: malloc-free-fill-0xff
             VALGRIND_OPTS: --leak-check=no --malloc-fill=0xff --free-fill=0xff
             JOB_POSTFIX: "malloc/free-fill: 0xff"
+          - VALGRIND_SCENARIO: track-fds
+            VALGRIND_OPTS: --leak-check=no --track-fds=yes
+            JOB_POSTFIX: "track-fds"
     runs-on: [self-hosted, regular, Linux, x86_64]
     name: >
       LuaJIT with Valgrind (Linux/x86_64)
-- 
2.49.0

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

[Sergey Kaplun via Tarantool-patches]

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}")
-- 
2.49.0

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

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Reported by Peter Cawley.

(cherry picked from commit ab39082fddfca0de268a106a3b6d736eef032328)

`loadfile()` doesn't close the fd in case when `fopen()` results
successfully, but then the file can't be read (since it is a directory,
for example) or parser failure occurs for some reason.

This patch fixes that behaviour by moving the error formatting after the
cleanup of the descriptor.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#11278
---
 src/lj_load.c                                 | 16 ++++-----
 .../lj-1249-loadfile-fd-leak.test.lua         | 34 ++++++++++++++++++-
 2 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/src/lj_load.c b/src/lj_load.c
index a6d0b464..fdbc54cb 100644
--- a/src/lj_load.c
+++ b/src/lj_load.c
@@ -87,6 +87,7 @@ LUALIB_API int luaL_loadfilex(lua_State *L, const char *filename,
   FileReaderCtx ctx;
   int status;
   const char *chunkname;
+  int err = 0;
   if (filename) {
     chunkname = lua_pushfstring(L, "@%s", filename);
     ctx.fp = fopen(filename, "rb");
@@ -100,17 +101,16 @@ LUALIB_API int luaL_loadfilex(lua_State *L, const char *filename,
     chunkname = "=stdin";
   }
   status = lua_loadx(L, reader_file, &ctx, chunkname, mode);
-  if (ferror(ctx.fp)) {
-    L->top -= filename ? 2 : 1;
-    lua_pushfstring(L, "cannot read %s: %s", chunkname+1, strerror(errno));
-    if (filename)
-      fclose(ctx.fp);
-    return LUA_ERRFILE;
-  }
+  if (ferror(ctx.fp)) err = errno;
   if (filename) {
+    fclose(ctx.fp);
     L->top--;
     copyTV(L, L->top-1, L->top);
-    fclose(ctx.fp);
+  }
+  if (err) {
+    L->top--;
+    lua_pushfstring(L, "cannot read %s: %s", chunkname+1, strerror(err));
+    return LUA_ERRFILE;
   }
   return status;
 }
diff --git a/test/tarantool-tests/lj-1249-loadfile-fd-leak.test.lua b/test/tarantool-tests/lj-1249-loadfile-fd-leak.test.lua
index c1a45724..fe406fd1 100644
--- a/test/tarantool-tests/lj-1249-loadfile-fd-leak.test.lua
+++ b/test/tarantool-tests/lj-1249-loadfile-fd-leak.test.lua
@@ -6,7 +6,7 @@ local tap = require('tap')
 -- See also, https://github.com/LuaJIT/LuaJIT/issues/1249.
 local test = tap.test('lj-1249-loadfile-fd-leak')
 
-test:plan(2)
+test:plan(4)
 
 local allocinject = require('allocinject')
 
@@ -24,4 +24,36 @@ test:ok(not res, 'correct status, OOM on filename creation')
 test:like(errmsg, 'not enough memory',
           'correct error message, OOM on filename creation')
 
+-- Now try to read the directory. It can be opened but not read as
+-- a file on Linux-like systems.
+
+-- On macOS and BSD-like systems, the content of the directory may
+-- be read and contain some internal data, which we are not
+-- interested in.
+test:skipcond({
+  ['Disabled on non-Linux systems'] = jit.os ~= 'Linux',
+})
+
+local DIRNAME = '/dev'
+local GCSTR_OBJSIZE = 24
+
+-- Now the OOM error should be obtained on the creation of the
+-- error message that the given file (or directory) is not
+-- readable. But the string for the file name should be allocated
+-- without OOM, so set the corresponding limit for the string
+-- object.
+-- Don't forget to count the leading '@' and trailing '\0'.
+allocinject.enable_null_limited_alloc(#DIRNAME + GCSTR_OBJSIZE + 1 + 1)
+
+-- Error since can't read the directory (but actually the OOM on
+-- parsing preparation is raised before, due to allocation limit).
+res, errmsg = pcall(loadfile, DIRNAME)
+
+allocinject.disable()
+
+-- Sanity checks.
+test:ok(not res, 'correct status, OOM on error message creation')
+test:like(errmsg, 'not enough memory',
+          'correct error message, OOM on error message creation')
+
 test:done(true)
-- 
2.49.0

Re: [Tarantool-patches] [PATCH luajit 1/3] ci: add track-fds Valgrind scenario

[Sergey Bronnikov via Tarantool-patches]

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

Hello, Sergey,

Thanks for the patch! See my comments below.

On 6/5/25 08:44, Sergey Kaplun wrote:
> This patch adds a new field, track-fds [1], in the Valgrind workflow
> matrix to detect descriptor leakage in the tests.
>
> [1]:https://valgrind.org/docs/manual/manual-core.html#opt.track-fds
>
> Needed for tarantool/tarantool#11278
> ---
>   .github/workflows/valgrind-testing.yaml | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/.github/workflows/valgrind-testing.yaml b/.github/workflows/valgrind-testing.yaml
> index e6606478..b3c7bc80 100644
> --- a/.github/workflows/valgrind-testing.yaml
> +++ b/.github/workflows/valgrind-testing.yaml
> @@ -38,7 +38,11 @@ jobs:
>           # Therefore, testing on this platform is currently
>           # disabled.
>           BUILDTYPE: [Debug, Release]
> -        VALGRIND_SCENARIO: [full, malloc-free-fill-0x00, malloc-free-fill-0xff]
> +        VALGRIND_SCENARIO:
> +          - full
> +          - malloc-free-fill-0x00
> +          - malloc-free-fill-0xff
> +          - track-fds
Why we cannot add "track-fds" to the existed scenario?
>           include:
>             - BUILDTYPE: Debug
>               CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
> @@ -59,6 +63,9 @@ jobs:
>             - VALGRIND_SCENARIO: malloc-free-fill-0xff
>               VALGRIND_OPTS: --leak-check=no --malloc-fill=0xff --free-fill=0xff
>               JOB_POSTFIX: "malloc/free-fill: 0xff"
> +          - VALGRIND_SCENARIO: track-fds

According to documentation, the option "print out a list of open file 
descriptors on exit or on request".

So the fd leak detection is semi-automated. How it should work on CI?

> +            VALGRIND_OPTS: --leak-check=no --track-fds=yes
> +            JOB_POSTFIX: "track-fds"
>       runs-on: [self-hosted, regular, Linux, x86_64]
>       name: >
>         LuaJIT with Valgrind (Linux/x86_64)

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

Re: [Tarantool-patches] [PATCH luajit 1/3] ci: add track-fds Valgrind scenario

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the review!
Please consider my answers below.

On 06.06.25, Sergey Bronnikov wrote:
> Hello, Sergey,
> 
> Thanks for the patch! See my comments below.
> 
> On 6/5/25 08:44, Sergey Kaplun wrote:
> > This patch adds a new field, track-fds [1], in the Valgrind workflow
> > matrix to detect descriptor leakage in the tests.
> >
> > [1]:https://valgrind.org/docs/manual/manual-core.html#opt.track-fds
> >
> > Needed for tarantool/tarantool#11278
> > ---
> >   .github/workflows/valgrind-testing.yaml | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/.github/workflows/valgrind-testing.yaml b/.github/workflows/valgrind-testing.yaml
> > index e6606478..b3c7bc80 100644
> > --- a/.github/workflows/valgrind-testing.yaml
> > +++ b/.github/workflows/valgrind-testing.yaml
> > @@ -38,7 +38,11 @@ jobs:
> >           # Therefore, testing on this platform is currently
> >           # disabled.
> >           BUILDTYPE: [Debug, Release]
> > -        VALGRIND_SCENARIO: [full, malloc-free-fill-0x00, malloc-free-fill-0xff]
> > +        VALGRIND_SCENARIO:
> > +          - full
> > +          - malloc-free-fill-0x00
> > +          - malloc-free-fill-0xff
> > +          - track-fds
> Why we cannot add "track-fds" to the existed scenario?

I've thought to add this flag to the "full" scenario. But then I
realized that we already have "full" workflow running too long, and the
parallel shorter task will not slow the CI, I suppose.

> >           include:
> >             - BUILDTYPE: Debug
> >               CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
> > @@ -59,6 +63,9 @@ jobs:
> >             - VALGRIND_SCENARIO: malloc-free-fill-0xff
> >               VALGRIND_OPTS: --leak-check=no --malloc-fill=0xff --free-fill=0xff
> >               JOB_POSTFIX: "malloc/free-fill: 0xff"
> > +          - VALGRIND_SCENARIO: track-fds
> 
> According to documentation, the option "print out a list of open file 
> descriptors on exit or on request".
> 
> So the fd leak detection is semi-automated. How it should work on CI?

The fd not checked on exit without this flag. I suppose the wording in
the Valgrind's documentation isn't perfect.

You may test it locally with and without the corresponding flag in
the`VALGRIND_OPTS` env variable.

> 
> > +            VALGRIND_OPTS: --leak-check=no --track-fds=yes
> > +            JOB_POSTFIX: "track-fds"
> >       runs-on: [self-hosted, regular, Linux, x86_64]
> >       name: >
> >         LuaJIT with Valgrind (Linux/x86_64)

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

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

Re: [Tarantool-patches] [PATCH luajit 1/3] ci: add track-fds Valgrind scenario

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey,


On 6/6/25 17:03, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the review!
> Please consider my answers below.
>
> On 06.06.25, Sergey Bronnikov wrote:
>> Hello, Sergey,
>>
>> Thanks for the patch! See my comments below.
>>
>> On 6/5/25 08:44, Sergey Kaplun wrote:
>>> This patch adds a new field, track-fds [1], in the Valgrind workflow
>>> matrix to detect descriptor leakage in the tests.
>>>
>>> [1]:https://valgrind.org/docs/manual/manual-core.html#opt.track-fds
>>>
>>> Needed for tarantool/tarantool#11278
>>> ---
>>>    .github/workflows/valgrind-testing.yaml | 9 ++++++++-
>>>    1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/.github/workflows/valgrind-testing.yaml b/.github/workflows/valgrind-testing.yaml
>>> index e6606478..b3c7bc80 100644
>>> --- a/.github/workflows/valgrind-testing.yaml
>>> +++ b/.github/workflows/valgrind-testing.yaml
>>> @@ -38,7 +38,11 @@ jobs:
>>>            # Therefore, testing on this platform is currently
>>>            # disabled.
>>>            BUILDTYPE: [Debug, Release]
>>> -        VALGRIND_SCENARIO: [full, malloc-free-fill-0x00, malloc-free-fill-0xff]
>>> +        VALGRIND_SCENARIO:
>>> +          - full
>>> +          - malloc-free-fill-0x00
>>> +          - malloc-free-fill-0xff
>>> +          - track-fds
>> Why we cannot add "track-fds" to the existed scenario?
> I've thought to add this flag to the "full" scenario. But then I
> realized that we already have "full" workflow running too long, and the
> parallel shorter task will not slow the CI, I suppose.
Ok.
>>>            include:
>>>              - BUILDTYPE: Debug
>>>                CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
>>> @@ -59,6 +63,9 @@ jobs:
>>>              - VALGRIND_SCENARIO: malloc-free-fill-0xff
>>>                VALGRIND_OPTS: --leak-check=no --malloc-fill=0xff --free-fill=0xff
>>>                JOB_POSTFIX: "malloc/free-fill: 0xff"
>>> +          - VALGRIND_SCENARIO: track-fds
>> According to documentation, the option "print out a list of open file
>> descriptors on exit or on request".
>>
>> So the fd leak detection is semi-automated. How it should work on CI?
> The fd not checked on exit without this flag. I suppose the wording in
> the Valgrind's documentation isn't perfect.
>
> You may test it locally with and without the corresponding flag in
> the`VALGRIND_OPTS` env variable.

FD leak is detected but test is passed:


UpdateCTestConfiguration  from 
:/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/DartConfiguration.tcl
UpdateCTestConfiguration  from 
:/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/DartConfiguration.tcl
Test project 
/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64
Constructing a list of tests
Done constructing a list of tests
Updating test list for fixtures
Added 0 tests to meet fixture requirements
Checking test dependency graph...
Checking test dependency graph end
test 154
     Start 154: test/tarantool-tests/lj-1249-loadfile-fd-leak.test.lua

154: Test command: /bin/valgrind "--leak-check=no" "--track-fds=yes" 
"--error-exitcode=1" 
"--suppressions=/home/sergeyb/sources/MRG/tarantool/third_party/luajit/src/lj.supp" 
"--suppressions=/home/sergeyb/sources/MRG/tarantool/third_party/luajit/src/lj_extra.supp" 
"/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/src/luajit" 
"-e" 
"dofile[[/home/sergeyb/sources/MRG/tarantool/third_party/luajit/test/luajit-test-init.lua]]" 
"/home/sergeyb/sources/MRG/tarantool/third_party/luajit/test/tarantool-tests/lj-1249-loadfile-fd-leak.test.lua"
154: Working Directory: 
/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/test/tarantool-tests
154: Environment variables:
154: 
LUA_PATH=/home/sergeyb/sources/MRG/tarantool/third_party/luajit/test/tarantool-tests/?.lua;/home/sergeyb/sources/MRG/tarantool/third_party/luajit/test/tarantool-tests/?/init.lua;/home/sergeyb/sources/MRG/tarantool/third_party/luajit/src/?.lua;/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/src/?.lua;;
154:  LUAJIT_TEST_USE_VALGRIND=1
154: 
LUA_CPATH=/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/test/tarantool-tests/utils/?.so;
154: 
LD_LIBRARY_PATH=/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/test/tarantool-tests/utils:
154: Test timeout computed to be: 10000000
154: ==2176604== Memcheck, a memory error detector
154: ==2176604== Copyright (C) 2002-2022, and GNU GPL'd, by Julian 
Seward et al.
154: ==2176604== Using Valgrind-3.22.0 and LibVEX; rerun with -h for 
copyright info
154: ==2176604== Command: 
/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/src/luajit 
-e 
dofile[[/home/sergeyb/sources/MRG/tarantool/third_party/luajit/test/luajit-test-init.lua]] 
/home/sergeyb/sources/MRG/tarantool/third_party/luajit/test/tarantool-tests/lj-1249-loadfile-fd-leak.test.lua
154: ==2176604==
154: TAP version 13
154: 1..4
154: ok - correct status, OOM on filename creation
154: ok - correct error message, OOM on filename creation
154: ok - correct status, OOM on error message creation
154: ok - correct error message, OOM on error message creation
154: ==2176604==
154: ==2176604== FILE DESCRIPTORS: 5 open (3 std) at exit.
154: ==2176604== Open file descriptor 4: /dev
154: ==2176604==    at 0x4ABB175: open (open64.c:41)
154: ==2176604==    by 0x4A31B0E: _IO_file_open (fileops.c:188)
154: ==2176604==    by 0x4A31E51: _IO_file_fopen@@GLIBC_2.2.5 
(fileops.c:281)
154: ==2176604==    by 0x4A25EE1: __fopen_internal (iofopen.c:75)
154: ==2176604==    by 0x4A25EE1: fopen@@GLIBC_2.2.5 (iofopen.c:86)
154: ==2176604==    by 0x1FAB1E: luaL_loadfilex (src/lj_load.c:92)
154: ==2176604==    by 0x217F18: lj_cf_loadfile (src/lib_base.c:384)
154: ==2176604==    by 0x2A19B9: lj_BC_FUNCC 
(/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/src/lj_vm.S:899)
154: ==2176604==    by 0x1E7C81: lua_pcall (src/lj_api.c:1173)
154: ==2176604==    by 0x1D738F: docall (src/luajit.c:134)
154: ==2176604==    by 0x1D6E56: handle_script (src/luajit.c:304)
154: ==2176604==    by 0x1D615B: pmain (src/luajit.c:602)
154: ==2176604==    by 0x2A19B9: lj_BC_FUNCC 
(/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/src/lj_vm.S:899)
154: ==2176604==
154: ==2176604== Open file descriptor 3: 
/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/Testing/Temporary/LastTest.log.tmp
154: ==2176604==    <inherited from parent>
154: ==2176604==
154: ==2176604==
154: ==2176604== HEAP SUMMARY:
154: ==2176604==     in use at exit: 472 bytes in 1 blocks
154: ==2176604==   total heap usage: 1,253 allocs, 1,252 frees, 179,771 
bytes allocated
154: ==2176604==
154: ==2176604== For a detailed leak analysis, rerun with: --leak-check=full
154: ==2176604==
154: ==2176604== For lists of detected and suppressed errors, rerun with: -s
154: ==2176604== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 67 
from 18)
1/1 Test #154: test/tarantool-tests/lj-1249-loadfile-fd-leak.test.lua 
... Passed    0.77 sec

The following tests passed:
     test/tarantool-tests/lj-1249-loadfile-fd-leak.test.lua

100% tests passed, 0 tests failed out of 1

Label Time Summary:
tarantool-tests    =   0.77 sec*proc (1 test)

Total Test time (real) =   0.79 sec

>>> +            VALGRIND_OPTS: --leak-check=no --track-fds=yes
>>> +            JOB_POSTFIX: "track-fds"
>>>        runs-on: [self-hosted, regular, Linux, x86_64]
>>>        name: >
>>>          LuaJIT with Valgrind (Linux/x86_64)

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

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the review!

On 06.06.25, Sergey Bronnikov wrote:
> Hello, Sergey,
> 
> the test is passed when CMake option -DLUAJIT_USE_VALGRIND=ON is used and
> 
> patch with fix is reverted.

You should run it with the corresponding env variable (like it is done
in the CI), see the comment in the test header:

| VALGRIND_OPTS="--track-fds=yes" ctest -V -R lj-1249

> 
> Sergey

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/3] ci: add track-fds Valgrind scenario

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the reply!
Please see my answer below.

On 06.06.25, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> 
> On 6/6/25 17:03, Sergey Kaplun wrote:
> > Hi, Sergey!
> > Thanks for the review!
> > Please consider my answers below.
> >
> > On 06.06.25, Sergey Bronnikov wrote:
> >> Hello, Sergey,
> >>
> >> Thanks for the patch! See my comments below.
> >>
> >> On 6/5/25 08:44, Sergey Kaplun wrote:
> >>> This patch adds a new field, track-fds [1], in the Valgrind workflow
> >>> matrix to detect descriptor leakage in the tests.
> >>>
> >>> [1]:https://valgrind.org/docs/manual/manual-core.html#opt.track-fds
> >>>
> >>> Needed for tarantool/tarantool#11278
> >>> ---
> >>>    .github/workflows/valgrind-testing.yaml | 9 ++++++++-
> >>>    1 file changed, 8 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/.github/workflows/valgrind-testing.yaml b/.github/workflows/valgrind-testing.yaml
> >>> index e6606478..b3c7bc80 100644
> >>> --- a/.github/workflows/valgrind-testing.yaml
> >>> +++ b/.github/workflows/valgrind-testing.yaml

<snipped>

> >>> @@ -59,6 +63,9 @@ jobs:
> >>>              - VALGRIND_SCENARIO: malloc-free-fill-0xff
> >>>                VALGRIND_OPTS: --leak-check=no --malloc-fill=0xff --free-fill=0xff
> >>>                JOB_POSTFIX: "malloc/free-fill: 0xff"
> >>> +          - VALGRIND_SCENARIO: track-fds
> >> According to documentation, the option "print out a list of open file
> >> descriptors on exit or on request".
> >>
> >> So the fd leak detection is semi-automated. How it should work on CI?
> > The fd not checked on exit without this flag. I suppose the wording in
> > the Valgrind's documentation isn't perfect.
> >
> > You may test it locally with and without the corresponding flag in
> > the`VALGRIND_OPTS` env variable.
> 
> FD leak is detected but test is passed:
> 
> 
> UpdateCTestConfiguration  from 
> :/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/DartConfiguration.tcl
> UpdateCTestConfiguration  from 
> :/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/DartConfiguration.tcl
> Test project 
> /home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64
> Constructing a list of tests
> Done constructing a list of tests
> Updating test list for fixtures
> Added 0 tests to meet fixture requirements
> Checking test dependency graph...
> Checking test dependency graph end
> test 154
>      Start 154: test/tarantool-tests/lj-1249-loadfile-fd-leak.test.lua
> 
> 154: Test command: /bin/valgrind "--leak-check=no" "--track-fds=yes" 
> "--error-exitcode=1" 
> "--suppressions=/home/sergeyb/sources/MRG/tarantool/third_party/luajit/src/lj.supp" 
> "--suppressions=/home/sergeyb/sources/MRG/tarantool/third_party/luajit/src/lj_extra.supp" 
> "/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/src/luajit" 
> "-e" 
> "dofile[[/home/sergeyb/sources/MRG/tarantool/third_party/luajit/test/luajit-test-init.lua]]" 
> "/home/sergeyb/sources/MRG/tarantool/third_party/luajit/test/tarantool-tests/lj-1249-loadfile-fd-leak.test.lua"
> 154: Working Directory: 
> /home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/test/tarantool-tests
> 154: Environment variables:
> 154: 
> LUA_PATH=/home/sergeyb/sources/MRG/tarantool/third_party/luajit/test/tarantool-tests/?.lua;/home/sergeyb/sources/MRG/tarantool/third_party/luajit/test/tarantool-tests/?/init.lua;/home/sergeyb/sources/MRG/tarantool/third_party/luajit/src/?.lua;/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/src/?.lua;;
> 154:  LUAJIT_TEST_USE_VALGRIND=1
> 154: 
> LUA_CPATH=/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/test/tarantool-tests/utils/?.so;
> 154: 
> LD_LIBRARY_PATH=/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/test/tarantool-tests/utils:
> 154: Test timeout computed to be: 10000000
> 154: ==2176604== Memcheck, a memory error detector
> 154: ==2176604== Copyright (C) 2002-2022, and GNU GPL'd, by Julian 
> Seward et al.
> 154: ==2176604== Using Valgrind-3.22.0 and LibVEX; rerun with -h for 
> copyright info
> 154: ==2176604== Command: 
> /home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/src/luajit 
> -e 
> dofile[[/home/sergeyb/sources/MRG/tarantool/third_party/luajit/test/luajit-test-init.lua]] 
> /home/sergeyb/sources/MRG/tarantool/third_party/luajit/test/tarantool-tests/lj-1249-loadfile-fd-leak.test.lua
> 154: ==2176604==
> 154: TAP version 13
> 154: 1..4
> 154: ok - correct status, OOM on filename creation
> 154: ok - correct error message, OOM on filename creation
> 154: ok - correct status, OOM on error message creation
> 154: ok - correct error message, OOM on error message creation
> 154: ==2176604==
> 154: ==2176604== FILE DESCRIPTORS: 5 open (3 std) at exit.
> 154: ==2176604== Open file descriptor 4: /dev
> 154: ==2176604==    at 0x4ABB175: open (open64.c:41)
> 154: ==2176604==    by 0x4A31B0E: _IO_file_open (fileops.c:188)
> 154: ==2176604==    by 0x4A31E51: _IO_file_fopen@@GLIBC_2.2.5 
> (fileops.c:281)
> 154: ==2176604==    by 0x4A25EE1: __fopen_internal (iofopen.c:75)
> 154: ==2176604==    by 0x4A25EE1: fopen@@GLIBC_2.2.5 (iofopen.c:86)
> 154: ==2176604==    by 0x1FAB1E: luaL_loadfilex (src/lj_load.c:92)
> 154: ==2176604==    by 0x217F18: lj_cf_loadfile (src/lib_base.c:384)
> 154: ==2176604==    by 0x2A19B9: lj_BC_FUNCC 
> (/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/src/lj_vm.S:899)
> 154: ==2176604==    by 0x1E7C81: lua_pcall (src/lj_api.c:1173)
> 154: ==2176604==    by 0x1D738F: docall (src/luajit.c:134)
> 154: ==2176604==    by 0x1D6E56: handle_script (src/luajit.c:304)
> 154: ==2176604==    by 0x1D615B: pmain (src/luajit.c:602)
> 154: ==2176604==    by 0x2A19B9: lj_BC_FUNCC 
> (/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/src/lj_vm.S:899)
> 154: ==2176604==
> 154: ==2176604== Open file descriptor 3: 
> /home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/Testing/Temporary/LastTest.log.tmp
> 154: ==2176604==    <inherited from parent>
> 154: ==2176604==
> 154: ==2176604==
> 154: ==2176604== HEAP SUMMARY:
> 154: ==2176604==     in use at exit: 472 bytes in 1 blocks
> 154: ==2176604==   total heap usage: 1,253 allocs, 1,252 frees, 179,771 
> bytes allocated
> 154: ==2176604==
> 154: ==2176604== For a detailed leak analysis, rerun with: --leak-check=full
> 154: ==2176604==
> 154: ==2176604== For lists of detected and suppressed errors, rerun with: -s
> 154: ==2176604== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 67 
> from 18)
> 1/1 Test #154: test/tarantool-tests/lj-1249-loadfile-fd-leak.test.lua 
> ... Passed    0.77 sec
> 
> The following tests passed:
>      test/tarantool-tests/lj-1249-loadfile-fd-leak.test.lua
> 
> 100% tests passed, 0 tests failed out of 1
> 
> Label Time Summary:
> tarantool-tests    =   0.77 sec*proc (1 test)
> 
> Total Test time (real) =   0.79 sec

Hm, this is weird:

| UpdateCTestConfiguration  from :/home/burii/builds_workspace/luajit/lj-1249-loadfile-fd-leak/DartConfiguration.tcl
| UpdateCTestConfiguration  from :/home/burii/builds_workspace/luajit/lj-1249-loadfile-fd-leak/DartConfiguration.tcl
| Test project /home/burii/builds_workspace/luajit/lj-1249-loadfile-fd-leak
| Constructing a list of tests
| Done constructing a list of tests
| Updating test list for fixtures
| Added 0 tests to meet fixture requirements
| Checking test dependency graph...
| Checking test dependency graph end
| test 154
|     Start 154: test/tarantool-tests/lj-1249-loadfile-fd-leak.test.lua
| 
| 154: Test command: /usr/bin/valgrind "--error-exitcode=1" "--suppressions=/home/burii/builds_workspace/luajit/lj-1249-loadfile-fd-leak/src/lj.supp" "--suppressions=/home/burii/builds_workspace/luajit/lj-1249-loadfile-fd-leak/src/lj_extra.supp" "/home/burii/builds_workspace/luajit/lj-1249-loadfile-fd-leak/src/luajit" "-e" "dofile[[/home/burii/builds_workspace/luajit/lj-1249-loadfile-fd-leak/test/luajit-test-init.lua]]" "/home/burii/builds_workspace/luajit/lj-1249-loadfile-fd-leak/test/tarantool-tests/lj-1249-loadfile-fd-leak.test.lua"
| 154: Working Directory: /home/burii/builds_workspace/luajit/lj-1249-loadfile-fd-leak/test/tarantool-tests
| 154: Environment variables: 
| 154:  LUA_PATH=/home/burii/builds_workspace/luajit/lj-1249-loadfile-fd-leak/test/tarantool-tests/?.lua;/home/burii/builds_workspace/luajit/lj-1249-loadfile-fd-leak/test/tarantool-tests/?/init.lua;/home/burii/builds_workspace/luajit/lj-1249-loadfile-fd-leak/src/?.lua;/home/burii/builds_workspace/luajit/lj-1249-loadfile-fd-leak/src/?.lua;;
| 154:  LUAJIT_TEST_USE_VALGRIND=1
| 154:  LUA_CPATH=/home/burii/builds_workspace/luajit/lj-1249-loadfile-fd-leak/test/tarantool-tests/utils/?.so;
| 154:  LD_LIBRARY_PATH=/home/burii/builds_workspace/luajit/lj-1249-loadfile-fd-leak/test/tarantool-tests/utils:
| 154: Test timeout computed to be: 10000000
| 154: ==22564== Memcheck, a memory error detector
| 154: ==22564== Copyright (C) 2002-2024, and GNU GPL'd, by Julian Seward et al.
| 154: ==22564== Using Valgrind-3.25.1 and LibVEX; rerun with -h for copyright info
| 154: ==22564== Command: /home/burii/builds_workspace/luajit/lj-1249-loadfile-fd-leak/src/luajit -e dofile[[/home/burii/builds_workspace/luajit/lj-1249-loadfile-fd-leak/test/luajit-test-init.lua]] /home/burii/builds_workspace/luajit/lj-1249-loadfile-fd-leak/test/tarantool-tests/lj-1249-loadfile-fd-leak.test.lua
| 154: ==22564== 
| 154: TAP version 13
| 154: 1..4
| 154: ok - correct status, OOM on filename creation
| 154: ok - correct error message, OOM on filename creation
| 154: ok - correct status, OOM on error message creation
| 154: ok - correct error message, OOM on error message creation
| 154: ==22564== 
| 154: ==22564== FILE DESCRIPTORS: 6 open (5 inherited) at exit.
| 154: ==22564== Open file descriptor 4: /dev
| 154: ==22564==    at 0x4BB6640: open (in /lib64/libc.so.6)
| 154: ==22564==    by 0x4B39825: _IO_file_open (in /lib64/libc.so.6)
| 154: ==22564==    by 0x4B399C4: _IO_file_fopen@@GLIBC_2.2.5 (in /lib64/libc.so.6)
| 154: ==22564==    by 0x4B2DECC: __fopen_internal (in /lib64/libc.so.6)
| 154: ==22564==    by 0x40268AE: luaL_loadfilex (lj_load.c:92)
| 154: ==22564==    by 0x403C433: lj_cf_loadfile (lib_base.c:384)
| 154: ==22564==    by 0x40AF0E8: lj_BC_FUNCC (buildvm_x86.dasc:852)
| 154: ==22564==    by 0x40168B8: lua_pcall (lj_api.c:1173)
| 154: ==22564==    by 0x4008C21: docall (luajit.c:134)
| 154: ==22564==    by 0x400963A: handle_script (luajit.c:304)
| 154: ==22564==    by 0x400A559: pmain (luajit.c:602)
| 154: ==22564==    by 0x40AF0E8: lj_BC_FUNCC (buildvm_x86.dasc:852)
| 154: ==22564== 
| 154: ==22564== 
| 154: ==22564== HEAP SUMMARY:
| 154: ==22564==     in use at exit: 472 bytes in 1 blocks
| 154: ==22564==   total heap usage: 1,248 allocs, 1,247 frees, 179,415 bytes allocated
| 154: ==22564== 
| 154: ==22564== For a detailed leak analysis, rerun with: --leak-check=full
| 154: ==22564== 
| 154: ==22564== For lists of detected and suppressed errors, rerun with: -s
| 154: ==22564== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 66 from 18)
| 1/1 Test #154: test/tarantool-tests/lj-1249-loadfile-fd-leak.test.lua ...***Failed    1.05 sec
| 
| 0% tests passed, 1 tests failed out of 1
| 
| Label Time Summary:
| tarantool-tests    =   1.05 sec*proc (1 test)
| 
| Total Test time (real) =   1.07 sec
| 
| The following tests FAILED:
| 	154 - test/tarantool-tests/lj-1249-loadfile-fd-leak.test.lua (Failed) tarantool-tests
| Errors while running CTest
| Output from these tests are in: /home/burii/builds_workspace/luajit/lj-1249-loadfile-fd-leak/Testing/Temporary/LastTest.log
| Use "--rerun-failed --output-on-failure" to re-run the failed cases verbosely.

As you can see, `ERROR SUMMARY` is different from yours -- 1 error from
1 context. Also, I've tried multiple configurations, but all works the
same.

My valgrind version.
| $ valgrind --version
| valgrind-3.25.1

> 
> >>> +            VALGRIND_OPTS: --leak-check=no --track-fds=yes
> >>> +            JOB_POSTFIX: "track-fds"
> >>>        runs-on: [self-hosted, regular, Linux, x86_64]
> >>>        name: >
> >>>          LuaJIT with Valgrind (Linux/x86_64)

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

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

LGTM

On 6/6/25 18:14, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the review!
>
> On 06.06.25, Sergey Bronnikov wrote:
>> Hello, Sergey,
>>
>> the test is passed when CMake option -DLUAJIT_USE_VALGRIND=ON is used and
>>
>> patch with fix is reverted.
> You should run it with the corresponding env variable (like it is done
> in the CI), see the comment in the test header:
>
> | VALGRIND_OPTS="--track-fds=yes" ctest -V -R lj-1249

It works, thanks, but the test is still passed.

In  private conversation, we have found a reason - I have a version of 
Valgrind,

that doesn't fail on detected fd leak:

Release 3.24.0 (31 Oct 2024)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~

...

* ==================== CORE CHANGES ===================

* Bad file descriptor usage now generates a real error with
   --track-fds=yes that is suppressible and shows up in the xml output
   with full execution backtrace. The warnings shown without using the
   option are deprecated and will be removed in a future valgrind
   version.

The same behavior is in our CI, because Valgrind 3.24.0 is in 25.04+, 
but in GHA the latest version is 24.04.

1. https://repology.org/project/valgrind/versions

2. 
https://docs.github.com/en/actions/using-github-hosted-runners/using-github-hosted-runners/about-github-hosted-runners

>> Sergey

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