Tarantool development patches archive
 help / color / mirror / Atom feed
From: Serge Petrenko <sergepetrenko@tarantool.org>
To: vdavydov.dev@gmail.com
Cc: tarantool-patches@freelists.org,
	Serge Petrenko <sergepetrenko@tarantool.org>
Subject: [PATCH] lua/trigger: cleanup lua stack after trigger run
Date: Tue, 25 Jun 2019 17:32:50 +0300	[thread overview]
Message-ID: <20190625143250.97948-1-sergepetrenko@tarantool.org> (raw)

This patch adds a stack cleanup after a trigger is run and its return
values, if any, have been read.

This problem was found in a case when on_schema_init trigger set an
on_replace trigger on a space, and the trigger ran during recovery.
This lead to Lua stack overflows for the aforementioned reasons.

Closes #4275
---
https://github.com/tarantool/tarantool/issues/4275
https://github.com/tarantool/tarantool/tree/sp/gh-4275-before-replace-on-recovery

 src/lua/trigger.c                |  6 ++++
 test/box/before_replace.result   | 48 ++++++++++++++++++++++++++++++++
 test/box/before_replace.test.lua | 27 ++++++++++++++++++
 test/box/box.lua                 | 23 +++++++++++++++
 4 files changed, 104 insertions(+)

diff --git a/src/lua/trigger.c b/src/lua/trigger.c
index ec4d8aab3..4803e85c5 100644
--- a/src/lua/trigger.c
+++ b/src/lua/trigger.c
@@ -102,9 +102,15 @@ lbox_trigger_run(struct trigger *ptr, void *event)
 	int nret = lua_gettop(L) - top;
 	if (trigger->pop_event != NULL &&
 	    trigger->pop_event(L, nret, event) != 0) {
+		lua_settop(L, top);
 		luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref);
 		diag_raise();
 	}
+	/*
+	 * Clear the stack after pop_event saves all
+	 * the needed return values.
+	 */
+	lua_settop(L, top);
 	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref);
 }
 
diff --git a/test/box/before_replace.result b/test/box/before_replace.result
index 5054fdefd..661d999bb 100644
--- a/test/box/before_replace.result
+++ b/test/box/before_replace.result
@@ -858,3 +858,51 @@ save_type
 s:drop()
 ---
 ...
+--
+-- gh-4275 segfault if before_replace
+-- trigger set in on_schema_init triggers runs on recovery.
+--
+s = box.schema.space.create('test_on_schema_init')
+---
+...
+_ = s:create_index('pk')
+---
+...
+test_run:cmd('setopt delimiter ";"')
+---
+- true
+...
+function gen_inserts()
+    box.begin()
+    for i = 1,1000 do
+        s:upsert({1, 1}, {{'+', 2, 1}})
+    end
+    box.commit()
+end;
+---
+...
+test_run:cmd('setopt delimiter ""');
+---
+- true
+...
+for i = 1,66 do gen_inserts() end
+---
+...
+test_run:cmd('restart server default with args="true"')
+s = box.space.test_on_schema_init
+---
+...
+s:count()
+---
+- 1
+...
+-- insertion: 65999 updates from gen_inserts(), starting value is 1.
+-- recovery: 66000 invocations of before replace trigger, each
+-- invocation updates the tuple, but only in memory: the WAL remains
+-- unchanged.
+-- A total of 1 + 65999 + 66000 = 132000
+s:get{1}[2] == 2 * 66000 or s:get{1}[2]
+---
+- true
+...
+test_run:cmd('restart server default with cleanup=1')
diff --git a/test/box/before_replace.test.lua b/test/box/before_replace.test.lua
index c46ec25a0..688b012d0 100644
--- a/test/box/before_replace.test.lua
+++ b/test/box/before_replace.test.lua
@@ -306,3 +306,30 @@ _ = s:insert{1}
 save_type
 
 s:drop()
+
+--
+-- gh-4275 segfault if before_replace
+-- trigger set in on_schema_init triggers runs on recovery.
+--
+s = box.schema.space.create('test_on_schema_init')
+_ = s:create_index('pk')
+test_run:cmd('setopt delimiter ";"')
+function gen_inserts()
+    box.begin()
+    for i = 1,1000 do
+        s:upsert({1, 1}, {{'+', 2, 1}})
+    end
+    box.commit()
+end;
+test_run:cmd('setopt delimiter ""');
+for i = 1,66 do gen_inserts() end
+test_run:cmd('restart server default with args="true"')
+s = box.space.test_on_schema_init
+s:count()
+-- insertion: 65999 updates from gen_inserts(), starting value is 1.
+-- recovery: 66000 invocations of before replace trigger, each
+-- invocation updates the tuple, but only in memory: the WAL remains
+-- unchanged.
+-- A total of 1 + 65999 + 66000 = 132000
+s:get{1}[2] == 2 * 66000 or s:get{1}[2]
+test_run:cmd('restart server default with cleanup=1')
diff --git a/test/box/box.lua b/test/box/box.lua
index 2a8e0e4fa..7eaa5043e 100644
--- a/test/box/box.lua
+++ b/test/box/box.lua
@@ -1,5 +1,28 @@
 #!/usr/bin/env tarantool
 os = require('os')
+local enable_before_replace = arg[1] ~= nil
+
+print('\n\n\n', enable_before_replace, '\n\n\n')
+
+function test_before_replace_trig(old, new)
+    return new:update{{'+', 2, 1}}
+end
+
+function space_on_replace_trig(old, new)
+    if new and new[3] == 'test_on_schema_init' then
+        box.on_commit(function()
+            box.space.test_on_schema_init:before_replace(test_before_replace_trig)
+        end)
+    end
+end
+
+function on_init_trig()
+    if enable_before_replace then
+        box.space._space:on_replace(space_on_replace_trig)
+    end
+end
+
+box.ctl.on_schema_init(on_init_trig)
 
 box.cfg{
     listen              = os.getenv("LISTEN"),
-- 
2.20.1 (Apple Git-117)

             reply	other threads:[~2019-06-25 14:32 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-25 14:32 Serge Petrenko [this message]
2019-07-05  8:49 ` [tarantool-patches] " Serge Petrenko
2019-07-05  9:00 ` Vladimir Davydov
2019-07-05 11:20   ` [tarantool-patches] " Serge Petrenko
2019-07-05 12:45     ` Vladimir Davydov

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=20190625143250.97948-1-sergepetrenko@tarantool.org \
    --to=sergepetrenko@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --cc=vdavydov.dev@gmail.com \
    --subject='Re: [PATCH] lua/trigger: cleanup lua stack after trigger run' \
    /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