[Tarantool-patches] [PATCH v21 1/6] box/func: fix modules functions restore

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

In commit 96938fafb (Add hot function reload for C procedures)
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.

Fixes #5968

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 changelogs/unreleased/fix-module-reload.md |  8 ++
 src/box/func.c                             | 13 ++-
 test/box/CMakeLists.txt                    |  4 +
 test/box/func_restore.result               | 95 ++++++++++++++++++++++
 test/box/func_restore.test.lua             | 50 ++++++++++++
 test/box/func_restore1.c                   | 33 ++++++++
 test/box/func_restore2.c                   | 27 ++++++
 test/box/func_restore3.c                   | 27 ++++++
 test/box/func_restore4.c                   | 27 ++++++
 9 files changed, 277 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
 create mode 100644 test/box/func_restore3.c
 create mode 100644 test/box/func_restore4.c

diff --git a/changelogs/unreleased/fix-module-reload.md b/changelogs/unreleased/fix-module-reload.md
new file mode 100644
index 000000000..e44118aa8
--- /dev/null
+++ b/changelogs/unreleased/fix-module-reload.md
@@ -0,0 +1,8 @@
+## bugfix/core
+
+* Fix crash in case of reloading a compiled module when the
+  new module lacks some of functions which were present in the
+  former code. In turn this event triggers a fallback procedure
+  where we restore old functions but instead of restoring each
+  function we process a sole entry only leading to the crash
+  later when these restored functions are called (gh-5968).
diff --git a/src/box/func.c b/src/box/func.c
index 9909cee45..94b14c56c 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 module **modu
 
 	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)
@@ -403,10 +404,10 @@ module_reload(const char *package, const char *package_end, struct module **modu
 	return 0;
 restore:
 	/*
-	 * Some old-dso func can't be load from new module, restore old
-	 * functions.
+	 * Some old functions are not found in the new module,
+	 * thus restore all migrated functions back to the 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);
@@ -420,9 +421,7 @@ module_reload(const char *package, const char *package_end, struct module **modu
 		}
 		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..4216a0ba9 100644
--- a/test/box/CMakeLists.txt
+++ b/test/box/CMakeLists.txt
@@ -2,4 +2,8 @@ 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(func_restore3 func_restore3.c)
+build_module(func_restore4 func_restore4.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..7cd9e67c4
--- /dev/null
+++ b/test/box/func_restore.result
@@ -0,0 +1,95 @@
+-- test-run result file version 2
+--
+-- Test that compiled C 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
+ | ---
+ | ...
+
+_ = 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')
+ | ---
+ | ...
+
+box.func['func_restore.echo_3']:call()
+ | ---
+ | - 3
+ | ...
+box.func['func_restore.echo_2']:call()
+ | ---
+ | - 2
+ | ...
+box.func['func_restore.echo_1']:call()
+ | ---
+ | - 1
+ | ...
+
+function run_restore(path)                                      \
+    _ = pcall(fio.unlink(path_func_restore))                    \
+    fio.symlink(path, path_func_restore)                        \
+                                                                \
+    local 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()                      \
+end
+ | ---
+ | ...
+
+bad_modules = {                                                 \
+    fio.pathjoin(build_path, "test/box/func_restore2.") .. ext, \
+    fio.pathjoin(build_path, "test/box/func_restore3.") .. ext, \
+    fio.pathjoin(build_path, "test/box/func_restore4.") .. ext, \
+}
+ | ---
+ | ...
+
+for k, v in ipairs(bad_modules) do run_restore(v) end
+ | ---
+ | ...
diff --git a/test/box/func_restore.test.lua b/test/box/func_restore.test.lua
new file mode 100644
index 000000000..4bd05769d
--- /dev/null
+++ b/test/box/func_restore.test.lua
@@ -0,0 +1,50 @@
+--
+-- Test that compiled C 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
+
+_ = 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')
+
+box.func['func_restore.echo_3']:call()
+box.func['func_restore.echo_2']:call()
+box.func['func_restore.echo_1']:call()
+
+function run_restore(path)                                      \
+    _ = pcall(fio.unlink(path_func_restore))                    \
+    fio.symlink(path, path_func_restore)                        \
+                                                                \
+    local 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()                      \
+end
+
+bad_modules = {                                                 \
+    fio.pathjoin(build_path, "test/box/func_restore2.") .. ext, \
+    fio.pathjoin(build_path, "test/box/func_restore3.") .. ext, \
+    fio.pathjoin(build_path, "test/box/func_restore4.") .. ext, \
+}
+
+for k, v in ipairs(bad_modules) do run_restore(v) end
diff --git a/test/box/func_restore1.c b/test/box/func_restore1.c
new file mode 100644
index 000000000..891ec0136
--- /dev/null
+++ b/test/box/func_restore1.c
@@ -0,0 +1,33 @@
+#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..d9d6de8df
--- /dev/null
+++ b/test/box/func_restore2.c
@@ -0,0 +1,27 @@
+#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);
+}
diff --git a/test/box/func_restore3.c b/test/box/func_restore3.c
new file mode 100644
index 000000000..e38b44400
--- /dev/null
+++ b/test/box/func_restore3.c
@@ -0,0 +1,27 @@
+#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_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_restore4.c b/test/box/func_restore4.c
new file mode 100644
index 000000000..4e97ff0a0
--- /dev/null
+++ b/test/box/func_restore4.c
@@ -0,0 +1,27 @@
+#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_3(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	return echo_num(ctx, args, args_end, 3);
+}
-- 
2.30.2