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 89A3613CDF29; Mon, 9 Jun 2025 12:42:18 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 89A3613CDF29 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1749462138; bh=4baar0A0UnV+/3lH/OJPUpS7z79PL7mKgfVPI+HPWRE=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=tT+62Kb2bIWDBMSakNv3kjZCT/sHY9F8ZVRdpuO81MZqWF7ZWzylaBICKI10qXWv9 xPews8GEuvXhldon4aulWswnEXCQq7rUhYP4dQboeACNGrgFUnYjJnHlT5kZt+apk2 6NoDlYhTg2l0fAghgkD+wtGmHlNkuXtNxDPrCTrM= Received: from send240.i.mail.ru (send240.i.mail.ru [95.163.59.79]) (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 B060B13CDF1F for ; Mon, 9 Jun 2025 12:42:16 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B060B13CDF1F Received: by exim-smtp-567cc788d4-vtjb9 with esmtpa (envelope-from ) id 1uOZ1A-00000000E7p-2wFz; Mon, 09 Jun 2025 12:42:13 +0300 Content-Type: multipart/alternative; boundary="------------iMI0WFHdf0Ga0n60EySU3TFa" Message-ID: Date: Mon, 9 Jun 2025 12:42:12 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org References: <1bbdb610c35dae11562205060db742cd5c2fe263.1749101434.git.skaplun@tarantool.org> Content-Language: en-US In-Reply-To: <1bbdb610c35dae11562205060db742cd5c2fe263.1749101434.git.skaplun@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD95E97EA1894635BC30BDA652621C30F1215A06768B9B6C4E5182A05F538085040BA609CA9D78BBA583DE06ABAFEAF6705B04C797349A2A7307EDB27ADD8F3B4FDFEA245EA6D08AB3D X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE78CB87876C5D626D4EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637AC83A81C8FD4AD23D82A6BABE6F325AC2E85FA5F3EDFCBAA7353EFBB5533756677AE571B3D4B003C382ACD13F5699C0036B9AB59FB9E10E701F8BDB376D4DA9C389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C078FCF50C7EAF9C588941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B6F459A8243F1D1D44CC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB8D32BA5DBAC0009BE9E8FC8737B5C224958C1606C78F2434E76E601842F6C81A12EF20D2F80756B5FB606B96278B59C4276E601842F6C81A127C277FBC8AE2E8B6A4E49BB0F3BA1413AA81AA40904B5D99C9F4D5AE37F343AD1F44FA8B9022EA23BBE47FD9DD3FB595F5C1EE8F4F765FC72CEEB2601E22B093A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E735A3CCBC2573AEBDE1C4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A50073604A45D52E735002B1117B3ED6967339B34B8C15C062F5FEB6EB1EB183FD823CB91A9FED034534781492E4B8EEAD2B25D9E4C92BC8ACBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34951738D4D62E58A4E6D118D1B996088B7D1EED2E4069648E74116D403FFB1CC411D6070385B789671D7E09C32AA3244C8F6C69840A24224377DD89D51EBB77422B739EB7B6F61900EA455F16B58544A2E30DDF7C44BCB90DA5AE236DF995FB59978A700BF655EAEEED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXMZebaIdHP2ghjoIc/363UZI6Kf1ptIMVSykAyseJQ6/oTHWQpzqn6Y= X-Mailru-Sender: 520A125C2F17F0B1E52FEF5D219D6140402C216DCF31325703ED270C30F246C514B0B5FF45720E760152A3D17938EB451EB5A0BCEC6A560B3DDE9B364B0DF289BE2DA36745F2EEB5CEBA01FB949A1F1EEAB4BC95F72C04283CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 3/3] Fix another potential file descriptor leak in luaL_loadfile*(). 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 Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------iMI0WFHdf0Ga0n60EySU3TFa Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hi, Sergey, thanks for the patch! LGTM On 6/5/25 08:44, Sergey Kaplun wrote: > From: Mike Pall > > 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) --------------iMI0WFHdf0Ga0n60EySU3TFa Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit

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)
--------------iMI0WFHdf0Ga0n60EySU3TFa--