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

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

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