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 474AA6EC60; Fri, 2 Apr 2021 15:35:56 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 474AA6EC60 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1617366956; bh=ZM5UTmjjDY9RCTLQM/QQ/hClWxx9M+O2mzb8cFlO8Bw=; 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=YXfH3bNUcAEVCiPZtbI3SWFT2c09euWt4UNKXOacKCC0sCD5YJftJYjMSBEi9bf/Q gO1GoPt0iqooOSkBQMM/NmitLgkaGMiDU4gzQ8ptgidH0hb0nQ6a5mVMHYofS9ngBL l2Kd6ZJjFVKS319um0FqUFdv0/SNwDMT2lb6LdtQ= Received: from mail-lj1-f174.google.com (mail-lj1-f174.google.com [209.85.208.174]) (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 930466EC60 for ; Fri, 2 Apr 2021 15:35:01 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 930466EC60 Received: by mail-lj1-f174.google.com with SMTP id u4so5474746ljo.6 for ; Fri, 02 Apr 2021 05:35:01 -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=dnlnLm77N5Rj4JkN2p8OyM2jhg9ZVvSOTSelRVhDBhA=; b=BRq3hIkW14WWDYtBK36wnIUCkKX26zaFjA3SL4nrgObl0qk8e4nGzAzT7kycyBR7Wr uZGOagnoKLAHDkGIHZtphulEFXMEkvbWUaTFNrJGvBSCvDc3Lx7pvAwQ+hSW94GzYzdg pryNYdmR1OjBse07DWHDnJ0uesRfDgeKUdVTpeTiXygQQOnfG1wQ7goCKzKuyTrTY8Ij 0J8ahM4IslqY4Nv+ELkH22Uoa2i660b4rHBXvYz5XoES0WvLkloM9HuletyYnNFwRnFN KESKB9FdNXkUzIeajpebWP38oNdBrF9z1pQ8z8vHqX0hldkwByRzjpOItKigTmZHTOIT k6Ig== X-Gm-Message-State: AOAM533d4LCzBwl6q408y3zvYJSDXWsPNcJqNF7Z5iQA7RNyANPk6qm1 kda9FxDys5uhfYlgXGks7TupW/8s5etsfg== X-Google-Smtp-Source: ABdhPJxpBHVqPMOEAALm1/3dRycevb+YV9sWEOXwX+/eWAFTRz3+5AGyloAfS7QfmtnRL2AwKDO+1A== X-Received: by 2002:a05:651c:103a:: with SMTP id w26mr8239621ljm.273.1617366900359; Fri, 02 Apr 2021 05:35:00 -0700 (PDT) Received: from grain.localdomain ([5.18.171.94]) by smtp.gmail.com with ESMTPSA id h6sm847646lfd.77.2021.04.02.05.34.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 02 Apr 2021 05:34:59 -0700 (PDT) Received: by grain.localdomain (Postfix, from userid 1000) id 24F215601D9; Fri, 2 Apr 2021 15:34:22 +0300 (MSK) To: tml Date: Fri, 2 Apr 2021 15:34:16 +0300 Message-Id: <20210402123420.885834-4-gorcunov@gmail.com> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20210402123420.885834-1-gorcunov@gmail.com> References: <20210402123420.885834-1-gorcunov@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [Tarantool-patches] [PATCH v20 3/7] 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 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 --- 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 +#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..0d77e78b2 --- /dev/null +++ b/test/box/func_restore2.c @@ -0,0 +1,28 @@ +#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); +} -- 2.30.2