From e97b01f69a6b8cc15d2510708d68fce0f4de42bd Mon Sep 17 00:00:00 2001
From: Nikolay Shirokovskiy <nshirokovskiy@tarantool.org>
Date: Tue, 2 Jul 2024 16:06:13 +0300
Subject: [PATCH] fiber: fix leak on dead joinable fiber search
When fiber is accessed from Lua we create a userdata object and keep the
reference for future accesses. The reference is cleared when fiber is
stopped. But if fiber is joinable is still can be found with
`fiber.find`. In this case we create userdata object again.
Unfortunately as fiber is already stopped we fail to clear the
reference. The trigger memory that clear the reference is also leaked.
As well as fiber storage if it is accessed after fiber is stopped.
Let's add `on_destroy` trigger to fiber and clear the references there.
Note that with current set of LSAN suppressions the trigger memory leak
of the issue is not reported.
Closes #10187
NO_DOC=bugfix
(cherry picked from commit 7db4de75f8bd975066635f942ee49f33a2209353)
---
...-10187-fix-memleaks-on-dead-fiber-usage.md | 3 ++
src/lib/core/fiber.c | 7 +++
src/lib/core/fiber.h | 2 +
src/lua/fiber.c | 43 ++++++++++++-------
test/app-luatest/fiber_test.lua | 24 +++++++++++
5 files changed, 64 insertions(+), 15 deletions(-)
create mode 100644 changelogs/unreleased/gh-10187-fix-memleaks-on-dead-fiber-usage.md
diff --git a/changelogs/unreleased/gh-10187-fix-memleaks-on-dead-fiber-usage.md b/changelogs/unreleased/gh-10187-fix-memleaks-on-dead-fiber-usage.md
new file mode 100644
index 0000000000..eaa4d2e96e
--- /dev/null
+++ b/changelogs/unreleased/gh-10187-fix-memleaks-on-dead-fiber-usage.md
@@ -0,0 +1,3 @@
+## bugfix/core
+
+* Fixed memory leaks on using dead fiber (gh-10187).
diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index 0f01f019d8..6e01b6f209 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -960,6 +960,7 @@ fiber_reset(struct fiber *fiber)
{
rlist_create(&fiber->on_yield);
rlist_create(&fiber->on_stop);
+ rlist_create(&fiber->on_destroy);
clock_stat_reset(&fiber->clock_stat);
}
@@ -972,6 +973,11 @@ fiber_recycle(struct fiber *fiber)
assert(diag_is_empty(&fiber->diag));
/* no pending wakeup */
assert(rlist_empty(&fiber->state));
+ assert(rlist_empty(&fiber->on_stop));
+ if (trigger_run(&fiber->on_destroy, fiber) != 0)
+ panic("on_destroy triggers can't fail");
+ /* On destroy triggers are expected to remove themselves. */
+ assert(rlist_empty(&fiber->on_destroy));
fiber_stack_recycle(fiber);
fiber_reset(fiber);
fiber->name[0] = '\0';
@@ -1528,6 +1534,7 @@ fiber_destroy(struct cord *cord, struct fiber *f)
assert(f != cord->fiber);
trigger_destroy(&f->on_yield);
trigger_destroy(&f->on_stop);
+ trigger_destroy(&f->on_destroy);
rlist_del(&f->state);
rlist_del(&f->link);
rlist_del(&f->wake);
diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
index 494babd11b..da9f342e4a 100644
--- a/src/lib/core/fiber.h
+++ b/src/lib/core/fiber.h
@@ -625,6 +625,8 @@ struct fiber {
* request, even if they are never destroyed.
*/
struct rlist on_stop;
+ /** Triggers invoked before fiber is recycled. */
+ struct rlist on_destroy;
/**
* The list of fibers awaiting for this fiber's timely
* (or untimely) death.
diff --git a/src/lua/fiber.c b/src/lua/fiber.c
index 8a8f413084..abe0c4efa4 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -88,14 +88,12 @@ luaL_testcancel(struct lua_State *L)
static const char *fiberlib_name = "fiber";
/**
- * Trigger invoked when the fiber has stopped execution of its
- * current request. Only purpose - delete storage.lua.fid_ref and
- * storage.lua.storage_ref keeping a reference of Lua
- * fiber and fiber.storage objects. Unlike Lua stack,
- * Lua fiber storage may be created not only for fibers born from
- * Lua land. For example, an IProto request may execute a Lua
- * function, which can create the storage. Trigger guarantees,
- * that even for non-Lua fibers the Lua storage is destroyed.
+ * Trigger invoked when the fiber is stopped. Only purpose - delete
+ * storage.lua.storage_ref keeping a reference of Lua fiber.storage object.
+ * Unlike Lua stack, Lua fiber storage may be created not only for fibers born
+ * from Lua land. For example, an IProto request may execute a Lua function,
+ * which can create the storage. Trigger guarantees, that even for non-Lua
+ * fibers the Lua storage is destroyed.
*/
static int
lbox_fiber_on_stop(struct trigger *trigger, void *event)
@@ -103,6 +101,16 @@ lbox_fiber_on_stop(struct trigger *trigger, void *event)
struct fiber *f = event;
luaL_unref(tarantool_L, LUA_REGISTRYINDEX, f->storage.lua.storage_ref);
f->storage.lua.storage_ref = FIBER_LUA_NOREF;
+ trigger_clear(trigger);
+ free(trigger);
+ return 0;
+}
+
+/** Cleanup Lua fiber data when fiber is destroyed. */
+static int
+lbox_fiber_on_destroy(struct trigger *trigger, void *event)
+{
+ struct fiber *f = event;
luaL_unref(tarantool_L, LUA_REGISTRYINDEX, f->storage.lua.fid_ref);
f->storage.lua.fid_ref = FIBER_LUA_NOREF;
trigger_clear(trigger);
@@ -118,13 +126,10 @@ lbox_pushfiber(struct lua_State *L, struct fiber *f)
{
int fid_ref = f->storage.lua.fid_ref;
if (fid_ref == FIBER_LUA_NOREF) {
- struct trigger *t = malloc(sizeof(*t));
- if (t == NULL) {
- diag_set(OutOfMemory, sizeof(*t), "malloc", "t");
- luaT_error(L);
- }
- trigger_create(t, lbox_fiber_on_stop, NULL, (trigger_f0)free);
- trigger_add(&f->on_stop, t);
+ struct trigger *on_destroy = xmalloc(sizeof(*on_destroy));
+ trigger_create(on_destroy, lbox_fiber_on_destroy, NULL,
+ (trigger_f0)free);
+ trigger_add(&f->on_destroy, on_destroy);
uint64_t fid = f->fid;
/* create a new userdata */
@@ -642,6 +647,14 @@ lbox_fiber_storage(struct lua_State *L)
struct fiber *f = lbox_checkfiber(L, 1);
int storage_ref = f->storage.lua.storage_ref;
if (storage_ref == FIBER_LUA_NOREF) {
+ if (f->flags & FIBER_IS_DEAD) {
+ diag_set(IllegalParams, "the fiber is dead");
+ luaT_error(L);
+ }
+ struct trigger *on_stop = xmalloc(sizeof(*on_stop));
+ trigger_create(on_stop, lbox_fiber_on_stop, NULL,
+ (trigger_f0)free);
+ trigger_add(&f->on_stop, on_stop);
lua_newtable(L); /* create local storage on demand */
storage_ref = luaL_ref(L, LUA_REGISTRYINDEX);
f->storage.lua.storage_ref = storage_ref;
diff --git a/test/app-luatest/fiber_test.lua b/test/app-luatest/fiber_test.lua
index 8a681b48ed..4aa262617e 100644
--- a/test/app-luatest/fiber_test.lua
+++ b/test/app-luatest/fiber_test.lua
@@ -57,3 +57,27 @@ g.test_gh_9406_shutdown_with_lingering_fiber_join = function()
local cmd = string.format('%s -e "%s"', tarantool_bin, script)
t.assert(os.execute(cmd) == 0)
end
+
+g.test_gh_10187_no_memory_leak_on_dead_fiber_search = function()
+ local f = fiber.new(function() end)
+ f:set_joinable(true)
+ f:wakeup()
+ fiber.yield()
+ local x = fiber.find(f:id())
+ -- Check found the fiber object is the same as the one created after the
+ -- fiber became dead.
+ t.assert_equals(x, f)
+ -- Check we cannot access dead fiber storage.
+ t.assert_error_covers({
+ type = 'IllegalParams',
+ message = 'the fiber is dead',
+ }, x.__index, x, 'storage')
+ -- Check fiber object is GC after fiber is joined.
+ local weak_table = setmetatable({}, {__mode = 'v'})
+ weak_table.fiber = f
+ x:join()
+ f = nil -- luacheck: no unused
+ x = nil -- luacheck: no unused
+ collectgarbage()
+ t.assert_equals(weak_table.fiber, nil)
+end
--
GitLab