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 6F2886EC5B; Sat, 10 Apr 2021 18:02:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 6F2886EC5B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1618066968; bh=XZJXmvbjtRixDCvyWfgOCx4SgpVvJaxy0wzinxYAVU8=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=TGLb95RFBYZkzrlfUsG181kNfwgGtXBG+/TMwLJykKWd+sBa/E0trE3LmPLSIUJEy 6POA7wD+o82BcDnpaXnhlo1piJumCZbVRMYHRcBggb3Smc65eV6J/NxgFfbXYMy7CI fNdJeFw7peVKQMIGBePeUgflLgH/HnQSIvGgYEpc= Received: from mail-lf1-f52.google.com (mail-lf1-f52.google.com [209.85.167.52]) (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 276296EC5B for ; Sat, 10 Apr 2021 18:02:47 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 276296EC5B Received: by mail-lf1-f52.google.com with SMTP id w8so5799745lfr.0 for ; Sat, 10 Apr 2021 08:02:47 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=lF/cV89eCPHNfx1MPLbwBT3mWrLNmZmh941QdlgTt88=; b=W590IS4mbq2y13BqK7+94kyXyI5iUJkVvTLWdriULreOMVGsPauGAeP/YhkmopDZJV XfTagjYxDSgvYO//MfcLflupz99HQnjJWplrJOC4gRz+bAcD8hACA3vSniWA4J3Qjicw WDdPxa1Rx6GGNvoDRew/24FvhTIxiLYgM0jTneRXtV/9FQ4xXnxu9FxouoWz73pSZL1U y80tdm1pDNYP/TU/lZwxHdtvnBAgCnN1Mrwq8kTkomyDBElJIBDg0YkOjQfNHbknIkBi RU3U/uh/VDeKxnofckrVFscoagW48ggtSS18DrsQk13R+rwTILaGDLIVOA7u50lyMntj pKew== X-Gm-Message-State: AOAM532cYDo4mCM46tVVg83dDIF5ZVQnI3L9p00KYKdx3HPodcE9F6bX 2Vi4THb6ZG8htlgGzoai02G6VmEgobs= X-Google-Smtp-Source: ABdhPJxQ2tqk3TYGlF562eLkiYHlXDN9UwFFh3EitfdGXqOgpmwtsiO832F6LSiaGF4Mp03H5WAvKQ== X-Received: by 2002:ac2:4309:: with SMTP id l9mr12792411lfh.226.1618066965714; Sat, 10 Apr 2021 08:02:45 -0700 (PDT) Received: from grain.localdomain ([5.18.199.94]) by smtp.gmail.com with ESMTPSA id z14sm1270682ljk.33.2021.04.10.08.02.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 10 Apr 2021 08:02:44 -0700 (PDT) Received: by grain.localdomain (Postfix, from userid 1000) id CD4685601CD; Sat, 10 Apr 2021 18:02:43 +0300 (MSK) Date: Sat, 10 Apr 2021 18:02:43 +0300 To: Vladislav Shpilevoy Cc: tml Message-ID: References: <20210408164151.1759348-1-gorcunov@gmail.com> <20210408164151.1759348-2-gorcunov@gmail.com> <98549c9b-2c93-684b-2b97-2e1359fa725e@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <98549c9b-2c93-684b-2b97-2e1359fa725e@tarantool.org> User-Agent: Mutt/2.0.5 (2021-01-21) Subject: Re: [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 Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 --- 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 + +#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 + +#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 + +#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