Hello, Sergey,
the test is passed when CMake option -DLUAJIT_USE_VALGRIND=ON is used and
patch with fix is reverted.
Sergey
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}")