Re: [Tarantool-patches] [PATCH luajit 2/3] core: fix resources leak in memory profiler

[Sergey Kaplun <skaplun@tarantool.org>]

Hi!

On 30.12.20, Sergey Ostanevich wrote:
> Serge, can you point me to a branch please?

https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-misc-memprof-fixes

> 
> > On 30 Dec 2020, at 12:06, Igor Munkin <imun@tarantool.org> wrote:
> > 
> > Sergey,
> > 
> > Thanks for the patch! LGTM except the wording in commit message
> > (consider the comments below).
> > 
> > Side note: I want to notice that with on_start callback things would be
> > clearer. Let's return to this again later.
> > 
> > On 30.12.20, Sergey Kaplun wrote:
> >> When the profiler is failing to start with error different from
> > 
> > Typo: s/is failing/fails/.
> > Typo: s/with error/with the error/.
> > 
> >> PROFILE_ERRIO neither a file stream is closed nor ctx is freed
> >> in case of incorrect return status checking.
> > 
> > Strictly saying there are two problems:
> > * Possible leakage for PROFILE_ERRRUN
> > * Double free for PROFILE_ERRIO
> > 
> > So I propose the following wording:
> > 
> > | When memory profiler fails to start with PROFILE_ERRRUN status both
> > | stream and ctx are not released. At the same time when memory profiler
> > | fails to start with the PROFILE_ERRIO status both stream and ctx are
> > | released twice. Both cases occur due to invalid return status checking.
> > 
> >> 
> >> To avoid this behaviour on_stop callback is called manually inside
> > 
> > Minor: s/To avoid this behaviour/To fix the leakage/.
> > 
> >> the profiler when error on start is occurring. Checks in
> > 
> > Typo: s/is occuring/occurs/.
> > 
> >> misc.memprof.start() are omitted.
> >> 
> >> Follows up tarantool/tarantool#5442
> >> ---
> >> 
> >> * How patch was checked:
> >> Before patch you can occur the error like:
> >> | $ src/luajit -e '
> >> | local f, msg, errno = misc.memprof.start("/tmp/tmp_memprofile.bin")
> >> | misc.memprof.start("/tmp/tmp_memprofile.bin") print(f,msg,errno)
> >> | '
> >> | true    nil     nil
> >> | luajit: lj_state.c:178: close_state: Assertion `g->gc.total == sizeof(GG_State)' failed.
> >> This patch fixes it.
> >> 
> >> * Why this assertion is not failed in tests (we have the test with same
> >> functionality)?
> >> This assertion failed inside close_state. Tarantool in some reason
> >> doesn't call lua_close on stop. It's weird to me. I'll try to find an
> >> explanation and will create a ticket.
> >> 
> >> * Why I don't create a test case.
> >> The best idea is to do something like this and waiting for OOM:
> >> | for _ = 1, 10000 do
> >> |   misc.memprof.start("/tmp/tmp_memprofile.bin")
> >> | end
> >> But it's disgusting, so as I've discussed with Igor offline test case
> >> will be ommited.
> > 
> > Side note: The test aims to hit LUA_ERRMEM and need to be run for a long
> > time. The exact reason we didn't face this failure is the omitted
> > <lua_close> in Tarantool.
> > 
> >> 
> >> src/lib_misc.c   | 4 ----
> >> src/lj_memprof.c | 8 ++++++--
> >> 2 files changed, 6 insertions(+), 6 deletions(-)
> >> 
> > 
> > <snipped>
> > 
> >> 
> >> -- 
> >> 2.28.0
> >> 
> > 
> > -- 
> > Best regards,
> > IM
> 

-- 
Best regards,
Sergey Kaplun