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

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

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

Hi, Sergey,

thanks for the patch! LGTM

On 6/5/25 08:44, Sergey Kaplun wrote:
> 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)

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