From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id EA8176EC5D; Thu, 8 Apr 2021 19:42:27 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org EA8176EC5D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1617900148; bh=u0PCLTTO8KDLfkELxoOUMKvfnulWc3ijBU6eyWyK+FI=; h=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=FlpnVNwGpxixVdN/sZzZ11PZgXlahd6JCQWIkwZZLRjRCs5dKDvFMPi5UimMEr5lx nsYTo4uxo5s59sViddE3Gnl1KrUj8Az6nMqix7Dgns2TBW9cDuFvWsjdL93+vIsyF9 35wP2Tk6GOasEU0PyGSMWdT0fDd+h9oMyIFzeq+8= Received: from mail-lf1-f46.google.com (mail-lf1-f46.google.com [209.85.167.46]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 7CB366EC5D for ; Thu, 8 Apr 2021 19:42:08 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7CB366EC5D Received: by mail-lf1-f46.google.com with SMTP id n138so5136290lfa.3 for ; Thu, 08 Apr 2021 09:42:08 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=UChX3jmlcCbmbQqgVA0oo1AxKYpLkAJCcidZenhy0jM=; b=BtMpeASOPJypL1z2IE6A6k8j8gUdFlr+YxCEpkdrDCaPqQ9wMztfwLIgMmu6ETuEqg vnunPzbouZTY/FYFGnUizcKqNwHYF8QMmklLtADGSFm9IylEq3b+l6qM89A1TMvqLB3U L0hJ2mYFVafAwoOhhUtTZbxBwpD1iXsIXNPR1KQoAEJ/YpDtg5y6gLfK0HLPQbliEA4F IDc9ZzQFJVlzt3gwdDPHpXU+pLiDv9LPNtwD+agL/hKHmcTMSezLbXUTeh6vqim7knFk UVZpvtFaCuHSFHaUas5xuRH4Q72o7CpYdjsIwa+D6PjLRPKcUWrJOuhyIu5DBG21TD1d FOWA== X-Gm-Message-State: AOAM532OgNe40P/82qtNuNq1PBRa2Uku3aMx58cL6nZ2MsaHjQsfL0BO slo2y5CKmDVtBK5ukzDgMfZ9mIIoEmc= X-Google-Smtp-Source: ABdhPJxCsCz8rf8FPaRYjfAFyV1v0rXTSmNEuo+3H25nWRpgv+4/39PVrFsVFQr4Xoj/MS9pQnTcAg== X-Received: by 2002:a05:6512:2208:: with SMTP id h8mr3528096lfu.336.1617900127118; Thu, 08 Apr 2021 09:42:07 -0700 (PDT) Received: from grain.localdomain ([5.18.199.94]) by smtp.gmail.com with ESMTPSA id e18sm2885023ljl.92.2021.04.08.09.42.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 08 Apr 2021 09:42:05 -0700 (PDT) Received: by grain.localdomain (Postfix, from userid 1000) id 7B2DA56010E; Thu, 8 Apr 2021 19:41:52 +0300 (MSK) To: tml Date: Thu, 8 Apr 2021 19:41:46 +0300 Message-Id: <20210408164151.1759348-2-gorcunov@gmail.com> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20210408164151.1759348-1-gorcunov@gmail.com> References: <20210408164151.1759348-1-gorcunov@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [Tarantool-patches] [PATCH v21 1/6] box/func: fix modules functions restore X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Cyrill Gorcunov via Tarantool-patches Reply-To: Cyrill Gorcunov Cc: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 --- 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 +#include +#include + +#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 +#include +#include + +#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 +#include +#include + +#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 +#include +#include + +#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