[PATCH] lua/trigger: cleanup lua stack after trigger run

[PATCH] lua/trigger: cleanup lua stack after trigger run

[Serge Petrenko]

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)

[tarantool-patches] Re: [PATCH] lua/trigger: cleanup lua stack after trigger run

[Serge Petrenko]

[-- Attachment #1: Type: text/plain, Size: 5248 bytes --]

Resend
--
Serge Petrenko
sergepetrenko@tarantool.org




> 25 июня 2019 г., в 17:32, Serge Petrenko <sergepetrenko@tarantool.org> написал(а):
> 
> 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)
> 
> 


[-- Attachment #2: Type: text/html, Size: 9526 bytes --]

Re: [PATCH] lua/trigger: cleanup lua stack after trigger run

[Vladimir Davydov]

On Tue, Jun 25, 2019 at 05:32:50PM +0300, Serge Petrenko wrote:
> 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"),

Please run the test in a separate instance instead of patching the
default configuration. This will allow us to remove one 'restart',
thus decreasing the test execution time.

Other than that, the patch looks fine to me.

Re: [tarantool-patches] Re: [PATCH] lua/trigger: cleanup lua stack after trigger run

[Serge Petrenko]

[-- Attachment #1: Type: text/plain, Size: 3629 bytes --]

Hi! Thank you for review!

The changes are on the branch. I also added a more informative comment to the test case.

+...
+-- For this test the number of invocations of the before_replace
+-- trigger during recovery multiplied by the amount of return values
+-- in before_replace trigger must be greater than 66000.
+-- A total of 1 + 16999 + 17000 + 1= 34001
+-- insertion: 16999 updates from gen_inserts(), starting value is 1,
+-- plus additional 17000 runs of before_replace trigger, each updating
+-- the value by adding 1.
+-- recovery: 17000 invocations of before replace trigger.
+-- Each invocation updates the recovered tuple, but only in-memory, the
+-- WAL remains intact. Thus, we only see the last update. During recovery
+-- the value is only increased by 1, and this change is visible only in memory.
+s:get{1}[2] == 1 + 16999 + 17000 + 1 or s:get{1}[2]

--
Serge Petrenko
sergepetrenko@tarantool.org




> 5 июля 2019 г., в 12:00, Vladimir Davydov <vdavydov.dev@gmail.com> написал(а):
> 
> On Tue, Jun 25, 2019 at 05:32:50PM +0300, Serge Petrenko wrote:
>> 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"),
> 
> Please run the test in a separate instance instead of patching the
> default configuration. This will allow us to remove one 'restart',
> thus decreasing the test execution time.
> 
> Other than that, the patch looks fine to me.
> 


[-- Attachment #2: Type: text/html, Size: 6000 bytes --]

Re: [tarantool-patches] Re: [PATCH] lua/trigger: cleanup lua stack after trigger run

[Vladimir Davydov]

On Fri, Jul 05, 2019 at 02:20:47PM +0300, Serge Petrenko wrote:
> Hi! Thank you for review!
> 
> The changes are on the branch. I also added a more informative comment to the test case.

Thank you. I pushed the patch to master and 2.1.