Re: [Tarantool-patches] [PATCH luajit 1/2] Fix io.close().

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

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
>