[Tarantool-patches] [PATCH v20 3/7] box/func: fix modules functions restore

[Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

In commit 96938fafb an ability to hot reload of modules
has been introduced. When module is been reloaded his
functions are resolved to new symbols but if something
went wrong it is supposed to restore old symbols from
the old module. Actually current code restores only
one function and may crash if there a bunch of functions
to restore. Lets fix it.

Part-of #4642

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 changelogs/unreleased/fix-module-reload.md |   4 +
 src/box/func.c                             |  13 ++-
 test/box/CMakeLists.txt                    |   2 +
 test/box/func_restore.result               | 103 +++++++++++++++++++++
 test/box/func_restore.test.lua             |  43 +++++++++
 test/box/func_restore1.c                   |  34 +++++++
 test/box/func_restore2.c                   |  28 ++++++
 7 files changed, 220 insertions(+), 7 deletions(-)
 create mode 100644 changelogs/unreleased/fix-module-reload.md
 create mode 100644 test/box/func_restore.result
 create mode 100644 test/box/func_restore.test.lua
 create mode 100644 test/box/func_restore1.c
 create mode 100644 test/box/func_restore2.c

diff --git a/changelogs/unreleased/fix-module-reload.md b/changelogs/unreleased/fix-module-reload.md
new file mode 100644
index 000000000..7e189617f
--- /dev/null
+++ b/changelogs/unreleased/fix-module-reload.md
@@ -0,0 +1,4 @@
+## bugfix/core
+
+* Fix module reloading procedure which may crash in case if
+  new module is corrupted (gh-4642).
diff --git a/src/box/func.c b/src/box/func.c
index 233696a4f..1cd7073de 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -387,13 +387,14 @@ module_reload(const char *package, const char *package_end)
 
 	struct func_c *func, *tmp_func;
 	rlist_foreach_entry_safe(func, &old_module->funcs, item, tmp_func) {
+		/* Move immediately for restore sake. */
+		rlist_move(&new_module->funcs, &func->item);
 		struct func_name name;
 		func_split_name(func->base.def->name, &name);
 		func->func = module_sym(new_module, name.sym);
 		if (func->func == NULL)
 			goto restore;
 		func->module = new_module;
-		rlist_move(&new_module->funcs, &func->item);
 	}
 	module_cache_del(package, package_end);
 	if (module_cache_put(new_module) != 0)
@@ -402,10 +403,10 @@ module_reload(const char *package, const char *package_end)
 	return 0;
 restore:
 	/*
-	 * Some old-dso func can't be load from new module, restore old
-	 * functions.
+	 * Some old functions are not found int the new module,
+	 * thus restore all migrated functions back to original.
 	 */
-	do {
+	rlist_foreach_entry_safe(func, &new_module->funcs, item, tmp_func) {
 		struct func_name name;
 		func_split_name(func->base.def->name, &name);
 		func->func = module_sym(old_module, name.sym);
@@ -419,9 +420,7 @@ module_reload(const char *package, const char *package_end)
 		}
 		func->module = old_module;
 		rlist_move(&old_module->funcs, &func->item);
-	} while (func != rlist_first_entry(&old_module->funcs,
-					   struct func_c, item));
-	assert(rlist_empty(&new_module->funcs));
+	}
 	module_delete(new_module);
 	return -1;
 }
diff --git a/test/box/CMakeLists.txt b/test/box/CMakeLists.txt
index 06bfbbe9d..944831af2 100644
--- a/test/box/CMakeLists.txt
+++ b/test/box/CMakeLists.txt
@@ -2,4 +2,6 @@ include_directories(${MSGPUCK_INCLUDE_DIRS})
 build_module(function1 function1.c)
 build_module(reload1 reload1.c)
 build_module(reload2 reload2.c)
+build_module(func_restore1 func_restore1.c)
+build_module(func_restore2 func_restore2.c)
 build_module(tuple_bench tuple_bench.c)
diff --git a/test/box/func_restore.result b/test/box/func_restore.result
new file mode 100644
index 000000000..e679f4eb8
--- /dev/null
+++ b/test/box/func_restore.result
@@ -0,0 +1,103 @@
+-- test-run result file version 2
+--
+-- Test that function can be restored to the
+-- former values when new module can't be
+-- loaded for some reason (say there some
+-- missing functions).
+--
+build_path = os.getenv("BUILDDIR")
+ | ---
+ | ...
+package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
+ | ---
+ | ...
+
+fio = require('fio')
+ | ---
+ | ...
+
+ext = (jit.os == "OSX" and "dylib" or "so")
+ | ---
+ | ...
+
+path_func_restore = fio.pathjoin(build_path, "test/box/func_restore.") .. ext
+ | ---
+ | ...
+path_func_good = fio.pathjoin(build_path, "test/box/func_restore1.") .. ext
+ | ---
+ | ...
+path_func_bad = fio.pathjoin(build_path, "test/box/func_restore2.") .. ext
+ | ---
+ | ...
+
+_ = pcall(fio.unlink(path_func_restore))
+ | ---
+ | ...
+fio.symlink(path_func_good, path_func_restore)
+ | ---
+ | - true
+ | ...
+
+box.schema.func.create('func_restore.echo_1', {language = "C"})
+ | ---
+ | ...
+box.schema.func.create('func_restore.echo_2', {language = "C"})
+ | ---
+ | ...
+box.schema.func.create('func_restore.echo_3', {language = "C"})
+ | ---
+ | ...
+
+box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_3')
+ | ---
+ | ...
+box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_2')
+ | ---
+ | ...
+box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_1')
+ | ---
+ | ...
+
+-- Order *does* matter since we bind functions in
+-- a list where entries are added to the top.
+box.func['func_restore.echo_3']:call()
+ | ---
+ | - 3
+ | ...
+box.func['func_restore.echo_2']:call()
+ | ---
+ | - 2
+ | ...
+box.func['func_restore.echo_1']:call()
+ | ---
+ | - 1
+ | ...
+
+_ = pcall(fio.unlink(path_func_restore))
+ | ---
+ | ...
+fio.symlink(path_func_bad, path_func_restore)
+ | ---
+ | - true
+ | ...
+
+ok, _ = pcall(box.schema.func.reload, "func_restore")
+ | ---
+ | ...
+assert(not ok)
+ | ---
+ | - true
+ | ...
+
+box.func['func_restore.echo_1']:call()
+ | ---
+ | - 1
+ | ...
+box.func['func_restore.echo_2']:call()
+ | ---
+ | - 2
+ | ...
+box.func['func_restore.echo_3']:call()
+ | ---
+ | - 3
+ | ...
diff --git a/test/box/func_restore.test.lua b/test/box/func_restore.test.lua
new file mode 100644
index 000000000..8a38a6074
--- /dev/null
+++ b/test/box/func_restore.test.lua
@@ -0,0 +1,43 @@
+--
+-- Test that function can be restored to the
+-- former values when new module can't be
+-- loaded for some reason (say there some
+-- missing functions).
+--
+build_path = os.getenv("BUILDDIR")
+package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
+
+fio = require('fio')
+
+ext = (jit.os == "OSX" and "dylib" or "so")
+
+path_func_restore = fio.pathjoin(build_path, "test/box/func_restore.") .. ext
+path_func_good = fio.pathjoin(build_path, "test/box/func_restore1.") .. ext
+path_func_bad = fio.pathjoin(build_path, "test/box/func_restore2.") .. ext
+
+_ = pcall(fio.unlink(path_func_restore))
+fio.symlink(path_func_good, path_func_restore)
+
+box.schema.func.create('func_restore.echo_1', {language = "C"})
+box.schema.func.create('func_restore.echo_2', {language = "C"})
+box.schema.func.create('func_restore.echo_3', {language = "C"})
+
+box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_3')
+box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_2')
+box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_1')
+
+-- Order *does* matter since we bind functions in
+-- a list where entries are added to the top.
+box.func['func_restore.echo_3']:call()
+box.func['func_restore.echo_2']:call()
+box.func['func_restore.echo_1']:call()
+
+_ = pcall(fio.unlink(path_func_restore))
+fio.symlink(path_func_bad, path_func_restore)
+
+ok, _ = pcall(box.schema.func.reload, "func_restore")
+assert(not ok)
+
+box.func['func_restore.echo_1']:call()
+box.func['func_restore.echo_2']:call()
+box.func['func_restore.echo_3']:call()
diff --git a/test/box/func_restore1.c b/test/box/func_restore1.c
new file mode 100644
index 000000000..ffd69fd2b
--- /dev/null
+++ b/test/box/func_restore1.c
@@ -0,0 +1,34 @@
+#include <stdio.h>
+#include <stdbool.h>
+#include <msgpuck.h>
+
+#include "module.h"
+
+static int
+echo_num(box_function_ctx_t *ctx, const char *args,
+	 const char *args_end, unsigned int num)
+{
+	char res[16];
+	char *end = mp_encode_uint(res, num);
+	box_return_mp(ctx, res, end);
+	return 0;
+}
+
+
+int
+echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	return echo_num(ctx, args, args_end, 1);
+}
+
+int
+echo_2(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	return echo_num(ctx, args, args_end, 2);
+}
+
+int
+echo_3(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	return echo_num(ctx, args, args_end, 3);
+}
diff --git a/test/box/func_restore2.c b/test/box/func_restore2.c
new file mode 100644
index 000000000..0d77e78b2
--- /dev/null
+++ b/test/box/func_restore2.c
@@ -0,0 +1,28 @@
+#include <stdio.h>
+#include <stdbool.h>
+#include <msgpuck.h>
+
+#include "module.h"
+
+static int
+echo_num(box_function_ctx_t *ctx, const char *args,
+	 const char *args_end, unsigned int num)
+{
+	char res[16];
+	char *end = mp_encode_uint(res, num);
+	box_return_mp(ctx, res, end);
+	return 0;
+}
+
+
+int
+echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	return echo_num(ctx, args, args_end, 1);
+}
+
+int
+echo_2(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	return echo_num(ctx, args, args_end, 2);
+}
-- 
2.30.2