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

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

On Sat, Apr 10, 2021 at 01:31:06AM +0200, Vladislav Shpilevoy wrote:
...
> 
> 7. It is better to make these functions return something different from
> the valid module. For example, make them all return 0. Or even make them
> crash.
> 
> See below the diff I suggest you to apply. I didn't rename the test files
> though. Please, do it locally.
...
Thanks a huge for review, Vlad! I updated the series and force pushed it.
Here is a new version.
---
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_restore1.c                   |  22 ++++
 test/box/func_restore2.c                   |  17 ++++
 test/box/func_restore3.c                   |  17 ++++
 test/box/func_restore4.c                   |  17 ++++
 test/box/gh-5968-func-restore.result       | 111 +++++++++++++++++++++
 test/box/gh-5968-func-restore.test.lua     |  56 +++++++++++
 9 files changed, 258 insertions(+), 7 deletions(-)
 create mode 100644 changelogs/unreleased/fix-module-reload.md
 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
 create mode 100644 test/box/gh-5968-func-restore.result
 create mode 100644 test/box/gh-5968-func-restore.test.lua

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_restore1.c b/test/box/func_restore1.c
new file mode 100644
index 000000000..f2d0aaf78
--- /dev/null
+++ b/test/box/func_restore1.c
@@ -0,0 +1,22 @@
+#include "module.h"
+
+int
+echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	const char *mp1 = "\x01";
+	return box_return_mp(ctx, mp1, mp1 + 1);
+}
+
+int
+echo_2(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	const char *mp1 = "\x02";
+	return box_return_mp(ctx, mp1, mp1 + 1);
+}
+
+int
+echo_3(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	const char *mp1 = "\x03";
+	return box_return_mp(ctx, mp1, mp1 + 1);
+}
diff --git a/test/box/func_restore2.c b/test/box/func_restore2.c
new file mode 100644
index 000000000..c77ff4b42
--- /dev/null
+++ b/test/box/func_restore2.c
@@ -0,0 +1,17 @@
+#include <stdlib.h>
+
+#include "module.h"
+
+int
+echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	abort();
+	return 0;
+}
+
+int
+echo_2(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	abort();
+	return 0;
+}
diff --git a/test/box/func_restore3.c b/test/box/func_restore3.c
new file mode 100644
index 000000000..7dea37817
--- /dev/null
+++ b/test/box/func_restore3.c
@@ -0,0 +1,17 @@
+#include <stdlib.h>
+
+#include "module.h"
+
+int
+echo_2(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	abort();
+	return 0;
+}
+
+int
+echo_3(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	abort();
+	return 0;
+}
diff --git a/test/box/func_restore4.c b/test/box/func_restore4.c
new file mode 100644
index 000000000..545d9e8e3
--- /dev/null
+++ b/test/box/func_restore4.c
@@ -0,0 +1,17 @@
+#include <stdlib.h>
+
+#include "module.h"
+
+int
+echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	abort();
+	return 0;
+}
+
+int
+echo_3(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	abort();
+	return 0;
+}
diff --git a/test/box/gh-5968-func-restore.result b/test/box/gh-5968-func-restore.result
new file mode 100644
index 000000000..0914e780a
--- /dev/null
+++ b/test/box/gh-5968-func-restore.result
@@ -0,0 +1,111 @@
+-- test-run result file version 2
+--
+-- gh-5968: Test that compiled C function can be restored to the former
+-- values when a new module can't be loaded for some reason (say there are
+-- missing functions).
+--
+build_path = os.getenv("BUILDDIR")
+ | ---
+ | ...
+old_cpath = package.cpath
+ | ---
+ | ...
+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')
+ | ---
+ | ...
+
+assert(box.func['func_restore.echo_3']:call() == 3)
+ | ---
+ | - true
+ | ...
+assert(box.func['func_restore.echo_2']:call() == 2)
+ | ---
+ | - true
+ | ...
+assert(box.func['func_restore.echo_1']:call() == 1)
+ | ---
+ | - true
+ | ...
+
+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)                                              \
+                                                                \
+    assert(box.func['func_restore.echo_1']:call() == 1)         \
+    assert(box.func['func_restore.echo_2']:call() == 2)         \
+    assert(box.func['func_restore.echo_3']:call() == 3)         \
+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
+ | ---
+ | ...
+
+box.schema.func.drop('func_restore.echo_1')
+ | ---
+ | ...
+box.schema.func.drop('func_restore.echo_2')
+ | ---
+ | ...
+box.schema.func.drop('func_restore.echo_3')
+ | ---
+ | ...
+
+package.cpath = old_cpath
+ | ---
+ | ...
diff --git a/test/box/gh-5968-func-restore.test.lua b/test/box/gh-5968-func-restore.test.lua
new file mode 100644
index 000000000..c710db8b4
--- /dev/null
+++ b/test/box/gh-5968-func-restore.test.lua
@@ -0,0 +1,56 @@
+--
+-- gh-5968: Test that compiled C function can be restored to the former
+-- values when a new module can't be loaded for some reason (say there are
+-- missing functions).
+--
+build_path = os.getenv("BUILDDIR")
+old_cpath = package.cpath
+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')
+
+assert(box.func['func_restore.echo_3']:call() == 3)
+assert(box.func['func_restore.echo_2']:call() == 2)
+assert(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)                                              \
+                                                                \
+    assert(box.func['func_restore.echo_1']:call() == 1)         \
+    assert(box.func['func_restore.echo_2']:call() == 2)         \
+    assert(box.func['func_restore.echo_3']:call() == 3)         \
+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
+
+box.schema.func.drop('func_restore.echo_1')
+box.schema.func.drop('func_restore.echo_2')
+box.schema.func.drop('func_restore.echo_3')
+
+package.cpath = old_cpath
-- 
2.30.2