* [Tarantool-patches] [PATCH luajit 0/2] Fix io.close().
@ 2022-05-18 8:58 Sergey Kaplun via Tarantool-patches
2022-05-18 8:58 ` [Tarantool-patches] [PATCH luajit 1/2] " Sergey Kaplun via Tarantool-patches
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2022-05-18 8:58 UTC (permalink / raw)
To: Sergey Ostanevich, Igor Munkin; +Cc: tarantool-patches
Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-735-io-close-on-closed-file-full-ci
Issue: https://github.com/LuaJIT/LuaJIT/issues/735
PR: https://github.com/tarantool/tarantool/pull/7148
CI is read due to known issue with ubuntu_22_04.
Mike Pall (2):
Fix io.close().
Fix io.close() error message.
src/lib_io.c | 28 ++++++++++++-------
.../lj-735-io-close-on-closed-file.test.lua | 23 +++++++++++++++
2 files changed, 41 insertions(+), 10 deletions(-)
create mode 100644 test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua
--
2.34.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Tarantool-patches] [PATCH luajit 1/2] Fix io.close().
2022-05-18 8:58 [Tarantool-patches] [PATCH luajit 0/2] Fix io.close() Sergey Kaplun via Tarantool-patches
@ 2022-05-18 8:58 ` Sergey Kaplun via Tarantool-patches
2022-07-13 11:58 ` sergos via Tarantool-patches
2022-05-18 8:58 ` [Tarantool-patches] [PATCH luajit 2/2] Fix io.close() error message Sergey Kaplun via Tarantool-patches
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2022-05-18 8:58 UTC (permalink / raw)
To: Sergey Ostanevich, Igor Munkin; +Cc: tarantool-patches
From: Mike Pall <mike>
Reported by farmboy0.
When `io.close()` is called without arguments on already closed default
output `iof->fp` is already NULL. So, the forward call to `fclose()`
leads to SEGFAULT.
This patch adds the corresponding check by using `io_stdfile()` instead
`IOSTDF_IOF()`.
Also, this patch refactors several internal functions by changing
the argument type from `FILE *` to `IOFileUD *`.
Sergey Kaplun:
* added the description and the test for the problem
---
src/lib_io.c | 20 +++++++++--------
.../lj-735-io-close-on-closed-file.test.lua | 22 +++++++++++++++++++
2 files changed, 33 insertions(+), 9 deletions(-)
create mode 100644 test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua
diff --git a/src/lib_io.c b/src/lib_io.c
index 9763ed46..d9028938 100644
--- a/src/lib_io.c
+++ b/src/lib_io.c
@@ -60,12 +60,12 @@ static IOFileUD *io_tofile(lua_State *L)
return iof;
}
-static FILE *io_stdfile(lua_State *L, ptrdiff_t id)
+static IOFileUD *io_stdfile(lua_State *L, ptrdiff_t id)
{
IOFileUD *iof = IOSTDF_IOF(L, id);
if (iof->fp == NULL)
lj_err_caller(L, LJ_ERR_IOSTDCL);
- return iof->fp;
+ return iof;
}
static IOFileUD *io_file_new(lua_State *L)
@@ -189,8 +189,9 @@ static int io_file_readlen(lua_State *L, FILE *fp, MSize m)
}
}
-static int io_file_read(lua_State *L, FILE *fp, int start)
+static int io_file_read(lua_State *L, IOFileUD *iof, int start)
{
+ FILE *fp = iof->fp;
int ok, n, nargs = (int)(L->top - L->base) - start;
clearerr(fp);
if (nargs == 0) {
@@ -226,8 +227,9 @@ static int io_file_read(lua_State *L, FILE *fp, int start)
return n - start;
}
-static int io_file_write(lua_State *L, FILE *fp, int start)
+static int io_file_write(lua_State *L, IOFileUD *iof, int start)
{
+ FILE *fp = iof->fp;
cTValue *tv;
int status = 1;
for (tv = L->base+start; tv < L->top; tv++) {
@@ -261,7 +263,7 @@ static int io_file_iter(lua_State *L)
memcpy(L->top, &fn->c.upvalue[1], n*sizeof(TValue));
L->top += n;
}
- n = io_file_read(L, iof->fp, 0);
+ n = io_file_read(L, iof, 0);
if (ferror(iof->fp))
lj_err_callermsg(L, strVdata(L->top-2));
if (tvisnil(L->base) && (iof->type & IOFILE_FLAG_CLOSE)) {
@@ -287,18 +289,18 @@ static int io_file_lines(lua_State *L)
LJLIB_CF(io_method_close)
{
IOFileUD *iof = L->base < L->top ? io_tofile(L) :
- IOSTDF_IOF(L, GCROOT_IO_OUTPUT);
+ io_stdfile(L, GCROOT_IO_OUTPUT);
return io_file_close(L, iof);
}
LJLIB_CF(io_method_read)
{
- return io_file_read(L, io_tofile(L)->fp, 1);
+ return io_file_read(L, io_tofile(L), 1);
}
LJLIB_CF(io_method_write) LJLIB_REC(io_write 0)
{
- return io_file_write(L, io_tofile(L)->fp, 1);
+ return io_file_write(L, io_tofile(L), 1);
}
LJLIB_CF(io_method_flush) LJLIB_REC(io_flush 0)
@@ -452,7 +454,7 @@ LJLIB_CF(io_write) LJLIB_REC(io_write GCROOT_IO_OUTPUT)
LJLIB_CF(io_flush) LJLIB_REC(io_flush GCROOT_IO_OUTPUT)
{
- return luaL_fileresult(L, fflush(io_stdfile(L, GCROOT_IO_OUTPUT)) == 0, NULL);
+ return luaL_fileresult(L, fflush(io_stdfile(L, GCROOT_IO_OUTPUT)->fp) == 0, NULL);
}
static int io_std_getset(lua_State *L, ptrdiff_t id, const char *mode)
diff --git a/test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua b/test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua
new file mode 100644
index 00000000..795dad6c
--- /dev/null
+++ b/test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua
@@ -0,0 +1,22 @@
+local tap = require('tap')
+
+local test = tap.test('lj-735-io-close-on-closed-file')
+test:plan(1)
+
+local TEST_FILE = 'lj-735-io-close-on-closed-file.tmp'
+
+local oldstdout = io.output()
+io.output(TEST_FILE)
+
+local status, err = io.close()
+assert(status, err)
+
+status = pcall(io.close)
+
+io.output(oldstdout)
+
+test:ok(not status, 'close already closed file')
+
+assert(os.remove(TEST_FILE))
+
+os.exit(test:check() and 0 or 1)
--
2.34.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Tarantool-patches] [PATCH luajit 2/2] Fix io.close() error message.
2022-05-18 8:58 [Tarantool-patches] [PATCH luajit 0/2] Fix io.close() Sergey Kaplun via Tarantool-patches
2022-05-18 8:58 ` [Tarantool-patches] [PATCH luajit 1/2] " Sergey Kaplun via Tarantool-patches
@ 2022-05-18 8:58 ` Sergey Kaplun via Tarantool-patches
2022-07-13 11:58 ` sergos via Tarantool-patches
2022-07-13 11:58 ` [Tarantool-patches] [PATCH luajit 0/2] Fix io.close() sergos via Tarantool-patches
2022-11-23 7:50 ` Igor Munkin via Tarantool-patches
3 siblings, 1 reply; 10+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2022-05-18 8:58 UTC (permalink / raw)
To: Sergey Ostanevich, Igor Munkin; +Cc: tarantool-patches
From: Mike Pall <mike>
Reported by François Perrad.
When `io.close()` is called without arguments on already closed default
output the error message is `LJ_ERR_IOSTDCL` ("standard file is closed")
instead of `LJ_ERR_IOCLFL` ("attempt to use a closed file"). It is
never a "real" standard file, because trying to close a standard throws
an error ("cannot close standard file"). Also, this is inconsistent with
PUC Lua.
This patch adds the corresponding check and code branch for this corner
case.
Sergey Kaplun:
* added the description and the test for the problem
---
src/lib_io.c | 10 ++++++++--
.../lj-735-io-close-on-closed-file.test.lua | 5 +++--
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/lib_io.c b/src/lib_io.c
index d9028938..f0108227 100644
--- a/src/lib_io.c
+++ b/src/lib_io.c
@@ -288,8 +288,14 @@ static int io_file_lines(lua_State *L)
LJLIB_CF(io_method_close)
{
- IOFileUD *iof = L->base < L->top ? io_tofile(L) :
- io_stdfile(L, GCROOT_IO_OUTPUT);
+ IOFileUD *iof;
+ if (L->base < L->top) {
+ iof = io_tofile(L);
+ } else {
+ iof = IOSTDF_IOF(L, GCROOT_IO_OUTPUT);
+ if (iof->fp == NULL)
+ lj_err_caller(L, LJ_ERR_IOCLFL);
+ }
return io_file_close(L, iof);
}
diff --git a/test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua b/test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua
index 795dad6c..5e031c48 100644
--- a/test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua
+++ b/test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua
@@ -1,7 +1,7 @@
local tap = require('tap')
local test = tap.test('lj-735-io-close-on-closed-file')
-test:plan(1)
+test:plan(2)
local TEST_FILE = 'lj-735-io-close-on-closed-file.tmp'
@@ -11,11 +11,12 @@ io.output(TEST_FILE)
local status, err = io.close()
assert(status, err)
-status = pcall(io.close)
+status, err = pcall(io.close)
io.output(oldstdout)
test:ok(not status, 'close already closed file')
+test:ok(err:match('attempt to use a closed file'), 'correct error message')
assert(os.remove(TEST_FILE))
--
2.34.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 0/2] Fix io.close().
2022-05-18 8:58 [Tarantool-patches] [PATCH luajit 0/2] Fix io.close() Sergey Kaplun via Tarantool-patches
2022-05-18 8:58 ` [Tarantool-patches] [PATCH luajit 1/2] " Sergey Kaplun via Tarantool-patches
2022-05-18 8:58 ` [Tarantool-patches] [PATCH luajit 2/2] Fix io.close() error message Sergey Kaplun via Tarantool-patches
@ 2022-07-13 11:58 ` sergos via Tarantool-patches
2022-07-13 17:45 ` Sergey Kaplun via Tarantool-patches
2022-11-23 7:50 ` Igor Munkin via Tarantool-patches
3 siblings, 1 reply; 10+ messages in thread
From: sergos via Tarantool-patches @ 2022-07-13 11:58 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: tarantool-patches
Hi!
> On 18 May 2022, at 11:58, Sergey Kaplun <skaplun@tarantool.org> wrote:
>
>
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-735-io-close-on-closed-file-full-ci
> Issue: https://github.com/LuaJIT/LuaJIT/issues/735
> PR: https://github.com/tarantool/tarantool/pull/7148
>
> CI is read due to known issue with ubuntu_22_04.
^^^^ red?
>
> Mike Pall (2):
> Fix io.close().
> Fix io.close() error message.
>
> src/lib_io.c | 28 ++++++++++++-------
> .../lj-735-io-close-on-closed-file.test.lua | 23 +++++++++++++++
> 2 files changed, 41 insertions(+), 10 deletions(-)
> create mode 100644 test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 1/2] Fix io.close().
2022-05-18 8:58 ` [Tarantool-patches] [PATCH luajit 1/2] " Sergey Kaplun via Tarantool-patches
@ 2022-07-13 11:58 ` sergos via Tarantool-patches
2022-07-14 8:04 ` Sergey Kaplun via Tarantool-patches
0 siblings, 1 reply; 10+ messages in thread
From: sergos via Tarantool-patches @ 2022-07-13 11:58 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: tarantool-patches
Hi!
Thanks for the patch!
Just some minor nits.
LGTM
Sergos
> On 18 May 2022, at 11:58, Sergey Kaplun <skaplun@tarantool.org> wrote:
>
> From: Mike Pall <mike>
>
> Reported by farmboy0.
>
> When `io.close()` is called without arguments on already closed default
^ ^
an an
> output `iof->fp` is already NULL. So, the forward call to `fclose()`
^ ^ ^
the underlying following(?) the
> leads to SEGFAULT.
^
a
>
> This patch adds the corresponding check by using `io_stdfile()` instead
^
of the
> `IOSTDF_IOF()`.
^
of the
>
> Also, this patch refactors several internal functions by changing
> the argument type from `FILE *` to `IOFileUD *`.
^ ^
the the
>
> Sergey Kaplun:
> * added the description and the test for the problem
> ---
> src/lib_io.c | 20 +++++++++--------
> .../lj-735-io-close-on-closed-file.test.lua | 22 +++++++++++++++++++
> 2 files changed, 33 insertions(+), 9 deletions(-)
> create mode 100644 test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua
>
> diff --git a/src/lib_io.c b/src/lib_io.c
> index 9763ed46..d9028938 100644
> --- a/src/lib_io.c
> +++ b/src/lib_io.c
> @@ -60,12 +60,12 @@ static IOFileUD *io_tofile(lua_State *L)
> return iof;
> }
>
> -static FILE *io_stdfile(lua_State *L, ptrdiff_t id)
> +static IOFileUD *io_stdfile(lua_State *L, ptrdiff_t id)
> {
> IOFileUD *iof = IOSTDF_IOF(L, id);
> if (iof->fp == NULL)
> lj_err_caller(L, LJ_ERR_IOSTDCL);
> - return iof->fp;
> + return iof;
> }
>
> static IOFileUD *io_file_new(lua_State *L)
> @@ -189,8 +189,9 @@ static int io_file_readlen(lua_State *L, FILE *fp, MSize m)
> }
> }
>
> -static int io_file_read(lua_State *L, FILE *fp, int start)
> +static int io_file_read(lua_State *L, IOFileUD *iof, int start)
> {
> + FILE *fp = iof->fp;
> int ok, n, nargs = (int)(L->top - L->base) - start;
> clearerr(fp);
> if (nargs == 0) {
> @@ -226,8 +227,9 @@ static int io_file_read(lua_State *L, FILE *fp, int start)
> return n - start;
> }
>
> -static int io_file_write(lua_State *L, FILE *fp, int start)
> +static int io_file_write(lua_State *L, IOFileUD *iof, int start)
> {
> + FILE *fp = iof->fp;
> cTValue *tv;
> int status = 1;
> for (tv = L->base+start; tv < L->top; tv++) {
> @@ -261,7 +263,7 @@ static int io_file_iter(lua_State *L)
> memcpy(L->top, &fn->c.upvalue[1], n*sizeof(TValue));
> L->top += n;
> }
> - n = io_file_read(L, iof->fp, 0);
> + n = io_file_read(L, iof, 0);
> if (ferror(iof->fp))
> lj_err_callermsg(L, strVdata(L->top-2));
> if (tvisnil(L->base) && (iof->type & IOFILE_FLAG_CLOSE)) {
> @@ -287,18 +289,18 @@ static int io_file_lines(lua_State *L)
> LJLIB_CF(io_method_close)
> {
> IOFileUD *iof = L->base < L->top ? io_tofile(L) :
> - IOSTDF_IOF(L, GCROOT_IO_OUTPUT);
> + io_stdfile(L, GCROOT_IO_OUTPUT);
> return io_file_close(L, iof);
> }
>
> LJLIB_CF(io_method_read)
> {
> - return io_file_read(L, io_tofile(L)->fp, 1);
> + return io_file_read(L, io_tofile(L), 1);
> }
>
> LJLIB_CF(io_method_write) LJLIB_REC(io_write 0)
> {
> - return io_file_write(L, io_tofile(L)->fp, 1);
> + return io_file_write(L, io_tofile(L), 1);
> }
>
> LJLIB_CF(io_method_flush) LJLIB_REC(io_flush 0)
> @@ -452,7 +454,7 @@ LJLIB_CF(io_write) LJLIB_REC(io_write GCROOT_IO_OUTPUT)
>
> LJLIB_CF(io_flush) LJLIB_REC(io_flush GCROOT_IO_OUTPUT)
> {
> - return luaL_fileresult(L, fflush(io_stdfile(L, GCROOT_IO_OUTPUT)) == 0, NULL);
> + return luaL_fileresult(L, fflush(io_stdfile(L, GCROOT_IO_OUTPUT)->fp) == 0, NULL);
> }
>
> static int io_std_getset(lua_State *L, ptrdiff_t id, const char *mode)
> diff --git a/test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua b/test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua
> new file mode 100644
> index 00000000..795dad6c
> --- /dev/null
> +++ b/test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua
> @@ -0,0 +1,22 @@
> +local tap = require('tap')
> +
> +local test = tap.test('lj-735-io-close-on-closed-file')
> +test:plan(1)
> +
> +local TEST_FILE = 'lj-735-io-close-on-closed-file.tmp'
> +
> +local oldstdout = io.output()
> +io.output(TEST_FILE)
> +
> +local status, err = io.close()
> +assert(status, err)
> +
> +status = pcall(io.close)
> +
> +io.output(oldstdout)
Was not clear that you keep an old one for the test output, comment?
> +
> +test:ok(not status, 'close already closed file')
> +
> +assert(os.remove(TEST_FILE))
> +
> +os.exit(test:check() and 0 or 1)
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 2/2] Fix io.close() error message.
2022-05-18 8:58 ` [Tarantool-patches] [PATCH luajit 2/2] Fix io.close() error message Sergey Kaplun via Tarantool-patches
@ 2022-07-13 11:58 ` sergos via Tarantool-patches
2022-07-14 7:54 ` Sergey Kaplun via Tarantool-patches
0 siblings, 1 reply; 10+ messages in thread
From: sergos via Tarantool-patches @ 2022-07-13 11:58 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: tarantool-patches
Hi!
Thanks for the patch!
Just minor comments.
LGTM
Sergos
> On 18 May 2022, at 11:58, Sergey Kaplun <skaplun@tarantool.org> wrote:
>
> From: Mike Pall <mike>
>
> Reported by François Perrad.
>
> When `io.close()` is called without arguments on already closed default
an an
> output the error message is `LJ_ERR_IOSTDCL` ("standard file is closed”)
returned
> instead of `LJ_ERR_IOCLFL` ("attempt to use a closed file"). It is
> never a "real" standard file, because trying to close a standard throws
> an error ("cannot close standard file"). Also, this is inconsistent with
> PUC Lua.
>
> This patch adds the corresponding check and code branch for this corner
a
> case.
>
> Sergey Kaplun:
> * added the description and the test for the problem
> ---
> src/lib_io.c | 10 ++++++++--
> .../lj-735-io-close-on-closed-file.test.lua | 5 +++--
> 2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/src/lib_io.c b/src/lib_io.c
> index d9028938..f0108227 100644
> --- a/src/lib_io.c
> +++ b/src/lib_io.c
> @@ -288,8 +288,14 @@ static int io_file_lines(lua_State *L)
>
> LJLIB_CF(io_method_close)
> {
> - IOFileUD *iof = L->base < L->top ? io_tofile(L) :
> - io_stdfile(L, GCROOT_IO_OUTPUT);
> + IOFileUD *iof;
> + if (L->base < L->top) {
> + iof = io_tofile(L);
> + } else {
> + iof = IOSTDF_IOF(L, GCROOT_IO_OUTPUT);
> + if (iof->fp == NULL)
> + lj_err_caller(L, LJ_ERR_IOCLFL);
> + }
> return io_file_close(L, iof);
> }
>
> diff --git a/test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua b/test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua
> index 795dad6c..5e031c48 100644
> --- a/test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua
> +++ b/test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua
> @@ -1,7 +1,7 @@
> local tap = require('tap')
>
> local test = tap.test('lj-735-io-close-on-closed-file')
> -test:plan(1)
> +test:plan(2)
>
> local TEST_FILE = 'lj-735-io-close-on-closed-file.tmp'
>
> @@ -11,11 +11,12 @@ io.output(TEST_FILE)
> local status, err = io.close()
> assert(status, err)
>
> -status = pcall(io.close)
> +status, err = pcall(io.close)
>
> io.output(oldstdout)
>
> test:ok(not status, 'close already closed file')
> +test:ok(err:match('attempt to use a closed file'), 'correct error message')
>
> assert(os.remove(TEST_FILE))
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 0/2] Fix io.close().
2022-07-13 11:58 ` [Tarantool-patches] [PATCH luajit 0/2] Fix io.close() sergos via Tarantool-patches
@ 2022-07-13 17:45 ` Sergey Kaplun via Tarantool-patches
0 siblings, 0 replies; 10+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2022-07-13 17:45 UTC (permalink / raw)
To: sergos; +Cc: tarantool-patches
Hi, Sergos!
On 13.07.22, sergos wrote:
> Hi!
>
>
> > On 18 May 2022, at 11:58, Sergey Kaplun <skaplun@tarantool.org> wrote:
> >
> >
> > Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-735-io-close-on-closed-file-full-ci
> > Issue: https://github.com/LuaJIT/LuaJIT/issues/735
> > PR: https://github.com/tarantool/tarantool/pull/7148
> >
> > CI is read due to known issue with ubuntu_22_04.
>
> ^^^^ red?
Yes, typo:)
> >
> > Mike Pall (2):
> > Fix io.close().
> > Fix io.close() error message.
> >
> > src/lib_io.c | 28 ++++++++++++-------
> > .../lj-735-io-close-on-closed-file.test.lua | 23 +++++++++++++++
> > 2 files changed, 41 insertions(+), 10 deletions(-)
> > create mode 100644 test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua
> >
> > --
> > 2.34.1
> >
>
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 2/2] Fix io.close() error message.
2022-07-13 11:58 ` sergos via Tarantool-patches
@ 2022-07-14 7:54 ` Sergey Kaplun via Tarantool-patches
0 siblings, 0 replies; 10+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2022-07-14 7:54 UTC (permalink / raw)
To: sergos; +Cc: tarantool-patches
Hi, Sergos!
Thanks for the review!
On 13.07.22, sergos wrote:
> Hi!
>
> Thanks for the patch!
>
> Just minor comments.
I've updated commit message to the following:
===================================================================
Fix io.close() error message.
Reported by François Perrad.
When an `io.close()` is called without arguments on an already closed
default output, the returned error message is `LJ_ERR_IOSTDCL`
("standard file is closed") instead of `LJ_ERR_IOCLFL` ("attempt to use
a closed file"). It is never a "real" standard file, because trying to
close a standard throws an error ("cannot close standard file"). Also,
this is inconsistent with PUC Lua.
This patch adds the corresponding check and a code branch for this
corner case.
Sergey Kaplun:
* added the description and the test for the problem
Part of tarantool/tarantool#7230
===================================================================
>
> LGTM
> Sergos
>
>
> > On 18 May 2022, at 11:58, Sergey Kaplun <skaplun@tarantool.org> wrote:
> >
> > From: Mike Pall <mike>
> >
> > Reported by François Perrad.
> >
> > When `io.close()` is called without arguments on already closed default
> an an
>
> > output the error message is `LJ_ERR_IOSTDCL` ("standard file is closed”)
> returned
>
> > instead of `LJ_ERR_IOCLFL` ("attempt to use a closed file"). It is
> > never a "real" standard file, because trying to close a standard throws
> > an error ("cannot close standard file"). Also, this is inconsistent with
> > PUC Lua.
> >
> > This patch adds the corresponding check and code branch for this corner
> a
>
> > case.
> >
> > Sergey Kaplun:
> > * added the description and the test for the problem
> > ---
> > src/lib_io.c | 10 ++++++++--
> > .../lj-735-io-close-on-closed-file.test.lua | 5 +++--
> > 2 files changed, 11 insertions(+), 4 deletions(-)
<snipped>
> >
>
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 1/2] Fix io.close().
2022-07-13 11:58 ` sergos via Tarantool-patches
@ 2022-07-14 8:04 ` Sergey Kaplun via Tarantool-patches
0 siblings, 0 replies; 10+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2022-07-14 8:04 UTC (permalink / raw)
To: sergos; +Cc: tarantool-patches
Hi, Sergos!
Thanks for the review!
On 13.07.22, sergos wrote:
> Hi!
>
> Thanks for the patch!
>
> Just some minor nits.
Updated the commit message to the following:
===================================================================
Fix io.close().
Reported by farmboy0.
When an `io.close()` is called without arguments on an already closed
default output, the underlying `iof->fp` is already NULL. So, the
following call to the `fclose()` leads to a SEGFAULT.
This patch adds the corresponding check by using of the `io_stdfile()`
instead of the `IOSTDF_IOF()`.
Also, this patch refactors several internal functions by changing the
argument type from the `FILE *` to the `IOFileUD *`.
Sergey Kaplun:
* added the description and the test for the problem
Part of tarantool/tarantool#7230
===================================================================
>
> LGTM
> Sergos
>
> > On 18 May 2022, at 11:58, Sergey Kaplun <skaplun@tarantool.org> wrote:
> >
> > From: Mike Pall <mike>
> >
> > Reported by farmboy0.
> >
> > When `io.close()` is called without arguments on already closed default
> ^ ^
> an an
> > output `iof->fp` is already NULL. So, the forward call to `fclose()`
> ^ ^ ^
> the underlying following(?) the
> > leads to SEGFAULT.
> ^
> a
> >
> > This patch adds the corresponding check by using `io_stdfile()` instead
> ^
> of the
> > `IOSTDF_IOF()`.
> ^
> of the
> >
> > Also, this patch refactors several internal functions by changing
> > the argument type from `FILE *` to `IOFileUD *`.
> ^ ^
> the the
> >
> > Sergey Kaplun:
> > * added the description and the test for the problem
> > ---
> > src/lib_io.c | 20 +++++++++--------
> > .../lj-735-io-close-on-closed-file.test.lua | 22 +++++++++++++++++++
> > 2 files changed, 33 insertions(+), 9 deletions(-)
> > create mode 100644 test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua
> >
> > diff --git a/src/lib_io.c b/src/lib_io.c
> > index 9763ed46..d9028938 100644
> > --- a/src/lib_io.c
> > +++ b/src/lib_io.c
<snipped>
> > diff --git a/test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua b/test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua
> > new file mode 100644
> > index 00000000..795dad6c
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua
> > @@ -0,0 +1,22 @@
> > +local tap = require('tap')
> > +
> > +local test = tap.test('lj-735-io-close-on-closed-file')
> > +test:plan(1)
> > +
> > +local TEST_FILE = 'lj-735-io-close-on-closed-file.tmp'
> > +
> > +local oldstdout = io.output()
> > +io.output(TEST_FILE)
> > +
> > +local status, err = io.close()
> > +assert(status, err)
> > +
> > +status = pcall(io.close)
> > +
> > +io.output(oldstdout)
>
> Was not clear that you keep an old one for the test output, comment?
Yep, add the corresponding comment:
===================================================================
diff --git a/test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua b/test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua
index 795dad6c..758c67a8 100644
--- a/test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua
+++ b/test/tarantool-tests/lj-735-io-close-on-closed-file.test.lua
@@ -5,6 +5,7 @@ test:plan(1)
local TEST_FILE = 'lj-735-io-close-on-closed-file.tmp'
+-- Save the old stdout for the TAP output.
local oldstdout = io.output()
io.output(TEST_FILE)
===================================================================
>
> > +
> > +test:ok(not status, 'close already closed file')
> > +
> > +assert(os.remove(TEST_FILE))
> > +
> > +os.exit(test:check() and 0 or 1)
> > --
> > 2.34.1
> >
>
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 0/2] Fix io.close().
2022-05-18 8:58 [Tarantool-patches] [PATCH luajit 0/2] Fix io.close() Sergey Kaplun via Tarantool-patches
` (2 preceding siblings ...)
2022-07-13 11:58 ` [Tarantool-patches] [PATCH luajit 0/2] Fix io.close() sergos via Tarantool-patches
@ 2022-11-23 7:50 ` Igor Munkin via Tarantool-patches
3 siblings, 0 replies; 10+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2022-11-23 7:50 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: tarantool-patches
Sergey,
I've checked the patches into all long-term branches in tarantool/luajit
and bumped a new version in master, 2.10 and 1.10.
--
Best regards,
IM
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-11-23 8:04 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-18 8:58 [Tarantool-patches] [PATCH luajit 0/2] Fix io.close() Sergey Kaplun via Tarantool-patches
2022-05-18 8:58 ` [Tarantool-patches] [PATCH luajit 1/2] " Sergey Kaplun via Tarantool-patches
2022-07-13 11:58 ` sergos via Tarantool-patches
2022-07-14 8:04 ` Sergey Kaplun via Tarantool-patches
2022-05-18 8:58 ` [Tarantool-patches] [PATCH luajit 2/2] Fix io.close() error message Sergey Kaplun via Tarantool-patches
2022-07-13 11:58 ` sergos via Tarantool-patches
2022-07-14 7:54 ` Sergey Kaplun via Tarantool-patches
2022-07-13 11:58 ` [Tarantool-patches] [PATCH luajit 0/2] Fix io.close() sergos via Tarantool-patches
2022-07-13 17:45 ` Sergey Kaplun via Tarantool-patches
2022-11-23 7:50 ` Igor Munkin via Tarantool-patches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox