Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Kaplun <skaplun@tarantool.org>
To: Sergey Ostanevich <sergos@tarantool.org>,
	Igor Munkin <imun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: [Tarantool-patches] [PATCH luajit 2/3] core: fix resources leak in memory profiler
Date: Wed, 30 Dec 2020 01:22:57 +0300	[thread overview]
Message-ID: <061599ac978b162d42ce80ac65106ba67c6d5fcb.1609278043.git.skaplun@tarantool.org> (raw)
In-Reply-To: <cover.1609278043.git.skaplun@tarantool.org>

When the profiler is failing to start with error different from
PROFILE_ERRIO neither a file stream is closed nor ctx is freed
in case of incorrect return status checking.

To avoid this behaviour on_stop callback is called manually inside
the profiler when error on start is occurring. Checks in
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.

 src/lib_misc.c   | 4 ----
 src/lj_memprof.c | 8 ++++++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/lib_misc.c b/src/lib_misc.c
index f89827e..1dab08c 100644
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -177,10 +177,6 @@ LJLIB_CF(misc_memprof_start)
   memprof_status = lj_memprof_start(L, &opt);
 
   if (LJ_UNLIKELY(memprof_status != PROFILE_SUCCESS)) {
-    if (memprof_status == PROFILE_ERRIO) {
-      fclose(ctx->stream);
-      lj_mem_free(ctx->g, ctx, sizeof(*ctx));
-    }
     switch (memprof_status) {
     case PROFILE_ERRUSE:
       lua_pushnil(L);
diff --git a/src/lj_memprof.c b/src/lj_memprof.c
index 4994de5..c4d2645 100644
--- a/src/lj_memprof.c
+++ b/src/lj_memprof.c
@@ -228,8 +228,11 @@ int lj_memprof_start(struct lua_State *L, const struct lj_memprof_options *opt)
   lua_assert(opt->buf != NULL);
   lua_assert(opt->len != 0);
 
-  if (mp->state != MPS_IDLE)
+  if (mp->state != MPS_IDLE) {
+    /* Clean up resourses. Ignore possible errors. */
+    opt->on_stop(opt->ctx, opt->buf);
     return PROFILE_ERRRUN;
+  }
 
   /* Discard possible old errno. */
   mp->saved_errno = 0;
@@ -331,7 +334,8 @@ errio:
 int lj_memprof_start(struct lua_State *L, const struct lj_memprof_options *opt)
 {
   UNUSED(L);
-  UNUSED(opt);
+  /* Clean up resourses. Ignore possible errors. */
+  opt->on_stop(opt->ctx, opt->buf);
   return PROFILE_ERRUSE;
 }
 
-- 
2.28.0

  parent reply	other threads:[~2020-12-29 22:23 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-29 22:22 [Tarantool-patches] [PATCH luajit 0/3] LuaJIT memory profiler bug fixes Sergey Kaplun
2020-12-29 22:22 ` [Tarantool-patches] [PATCH luajit 1/3] misc: fix build with disabled memory profiler Sergey Kaplun
2020-12-30  8:49   ` Igor Munkin
2020-12-30  8:52     ` Sergey Kaplun
2020-12-30  9:42       ` Sergey Ostanevich
2020-12-29 22:22 ` Sergey Kaplun [this message]
2020-12-30  9:06   ` [Tarantool-patches] [PATCH luajit 2/3] core: fix resources leak in " Igor Munkin
2020-12-30  9:31     ` Sergey Ostanevich
2020-12-30  9:33       ` Sergey Kaplun
2020-12-30  9:32     ` Sergey Kaplun
2020-12-30  9:53       ` Sergey Ostanevich
2020-12-29 22:22 ` [Tarantool-patches] [PATCH luajit 3/3] core: remove excess assertion inside memprof Sergey Kaplun
2020-12-30  9:39   ` Igor Munkin
2020-12-30  9:50     ` Sergey Kaplun
2020-12-30 10:50       ` Sergey Ostanevich
2020-12-30 11:06         ` Sergey Kaplun
2020-12-30  8:24 ` [Tarantool-patches] [PATCH luajit 0/3] LuaJIT memory profiler bug fixes Alexander V. Tikhonov
2020-12-30 11:20 ` Igor Munkin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=061599ac978b162d42ce80ac65106ba67c6d5fcb.1609278043.git.skaplun@tarantool.org \
    --to=skaplun@tarantool.org \
    --cc=imun@tarantool.org \
    --cc=sergos@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit 2/3] core: fix resources leak in memory profiler' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox