From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f50.google.com (mail-lf1-f50.google.com [209.85.167.50]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 1B8AD4765E0 for ; Wed, 23 Dec 2020 18:42:00 +0300 (MSK) Received: by mail-lf1-f50.google.com with SMTP id x20so40973735lfe.12 for ; Wed, 23 Dec 2020 07:42:00 -0800 (PST) From: Cyrill Gorcunov Date: Wed, 23 Dec 2020 18:41:51 +0300 Message-Id: <20201223154155.234884-1-gorcunov@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [Tarantool-patches] [PATCH v5 0/4] Our feedback daemon sends only a few portions of usage List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tml Cc: Vladislav Shpilevoy For this sake, if crash happens we can send it to our feedback server. In this series we implement only base functionality and may extend it later. I didn't find yet a simple way to test this code anything but manually. Any comments are highly appreciated. v2: - left for internal use v3 (by Mons): - enable sending crash report by default but give an ability to disable it - use vfork when running subsequent exeutable of tarantool to send report, since plain execve won't produce local crashdump v4 (by Vlad): - move all fatal's signal handling to crash.c - export only those helpers which are really needed - update backtrace to use size as an arument - provide strlcpy if not present on the system - use static allocated buffer for report - use base64 library instead of own implmentation v5: - by Vlad: - use crash_cfg name for crash engine configuration - configure crash engine via box helpers - drop unneded gaps from static allocations - encode in base64 form the backtrace only no need for second round - drop unused headers - prevent from chained crash storm - make sure no attempts to send feedback is done if feedback host was not properly configured (iow, if box.cfg{} was not even called) - do not use tnt_raise() but diag_set instead - by me: - zap special json characters Vlad, take a look please, I put interdiff with gorcunov/gh-5261-crash-report-4-rev2 below (note it is interdiff with previous version not master branch). issue https://github.com/tarantool/tarantool/issues/5261 branch gorcunov/gh-5261-crash-report-5 Cyrill Gorcunov (4): util: introduce strlcpy helper backtrace: allow to specify destination buffer crash: move fatal signal handling in crash: report crash data to the feedback server CMakeLists.txt | 1 + src/box/box.cc | 18 + src/box/box.h | 1 + src/box/lua/cfg.cc | 9 + src/box/lua/load_cfg.lua | 6 +- src/lib/core/CMakeLists.txt | 3 +- src/lib/core/backtrace.cc | 12 +- src/lib/core/backtrace.h | 3 + src/lib/core/crash.c | 603 ++++++++++++++++++++++++++++++++ src/lib/core/crash.h | 44 +++ src/lib/core/util.c | 14 + src/main.cc | 139 +------- src/trivia/config.h.cmake | 5 + src/trivia/util.h | 15 + test/app-tap/init_script.result | 1 + test/box/admin.result | 2 + test/box/cfg.result | 4 + 17 files changed, 739 insertions(+), 141 deletions(-) create mode 100644 src/lib/core/crash.c create mode 100644 src/lib/core/crash.h base-commit: 28f3b2f1e845aff49048d92f9062a4dfa365bf57 -- diff --git a/src/box/box.cc b/src/box/box.cc index 27079fd46..440bfa305 100644 --- a/src/box/box.cc +++ b/src/box/box.cc @@ -1214,19 +1214,21 @@ box_set_prepared_stmt_cache_size(void) return 0; } -void -box_set_crash_params(void) +int +box_set_crash(void) { const char *host = cfg_gets("feedback_host"); bool is_enabled_1 = cfg_getb("feedback_enabled"); bool is_enabled_2 = cfg_getb("feedback_crashinfo"); if (host != NULL && strlen(host) >= CRASH_FEEDBACK_HOST_MAX) { - tnt_raise(ClientError, ER_CFG, "feedback_host", + diag_set(ClientError, ER_CFG, "feedback_host", "the address is too long"); + return -1; } - crash_cfg_set_params(host, is_enabled_1 && is_enabled_2); + crash_cfg(host, is_enabled_1 && is_enabled_2); + return 0; } /* }}} configuration bindings */ diff --git a/src/box/box.h b/src/box/box.h index 6792ade95..69fa096a1 100644 --- a/src/box/box.h +++ b/src/box/box.h @@ -257,7 +257,7 @@ void box_set_replication_sync_timeout(void); void box_set_replication_skip_conflict(void); void box_set_replication_anon(void); void box_set_net_msg_max(void); -void box_set_crash_params(void); +int box_set_crash(void); int box_set_prepared_stmt_cache_size(void); diff --git a/src/box/lua/cfg.cc b/src/box/lua/cfg.cc index 089c770e8..2d3ccbf0e 100644 --- a/src/box/lua/cfg.cc +++ b/src/box/lua/cfg.cc @@ -376,13 +376,10 @@ lbox_cfg_set_replication_skip_conflict(struct lua_State *L) } static int -lbox_cfg_set_crash_params(struct lua_State *L) +lbox_cfg_set_crash(struct lua_State *L) { - try { - box_set_crash_params(); - } catch (Exception *) { + if (box_set_crash() != 0) luaT_error(L); - } return 0; } @@ -422,7 +419,7 @@ box_lua_cfg_init(struct lua_State *L) {"cfg_set_replication_anon", lbox_cfg_set_replication_anon}, {"cfg_set_net_msg_max", lbox_cfg_set_net_msg_max}, {"cfg_set_sql_cache_size", lbox_set_prepared_stmt_cache_size}, - {"cfg_set_crash_params", lbox_cfg_set_crash_params}, + {"cfg_set_crash", lbox_cfg_set_crash}, {NULL, NULL} }; diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua index 80009c7af..7e41e0999 100644 --- a/src/box/lua/load_cfg.lua +++ b/src/box/lua/load_cfg.lua @@ -34,7 +34,7 @@ end local ifdef_feedback_set_params = private.feedback_daemon ~= nil and private.feedback_daemon.set_feedback_params and - private.cfg_set_crash_params or nil + private.cfg_set_crash or nil -- all available options local default_cfg = { diff --git a/src/lib/core/crash.c b/src/lib/core/crash.c index f6dd91987..19d3d49ea 100644 --- a/src/lib/core/crash.c +++ b/src/lib/core/crash.c @@ -5,7 +5,9 @@ */ #include -#include +#include +#include +#include #include #include #include @@ -13,6 +15,7 @@ #include "third_party/base64.h" #include "small/static.h" #include "trivia/util.h" + #include "backtrace.h" #include "crash.h" #include "say.h" @@ -25,7 +28,7 @@ #define pr_crit(fmt, ...) fprintf(stderr, pr_fmt(fmt) "\n", ##__VA_ARGS__) #define pr_panic(fmt, ...) panic(pr_fmt(fmt), ##__VA_ARGS__) -/* Use strlcpy with destination as an array */ +/** Use strlcpy with destination as an array */ #define strlcpy_a(dst, src) strlcpy(dst, src, sizeof(dst)) #ifdef TARGET_OS_LINUX @@ -67,8 +70,10 @@ struct crash_greg { static struct crash_info { /** * These two are mostly useless as being - * plain addresses but keep for backward - * compatibility. + * plain addresses and without real binary + * crash dump file we can't use them for + * anything suitable (in terms of analysis sake) + * but keep for backward compatibility. */ void *context_addr; void *siginfo_addr; @@ -95,7 +100,7 @@ static struct crash_info { */ int sicode; #ifdef ENABLE_BACKTRACE - /* + /** * 1K of memory should be enough to keep the backtrace. * In worst case it gonna be simply trimmed. */ @@ -142,7 +147,7 @@ crash_init(const char *tarantool_bin) } void -crash_cfg_set_params(const char *host, bool is_enabled) +crash_cfg(const char *host, bool is_enabled) { if (host == NULL || !is_enabled) { if (send_crashinfo) { @@ -155,8 +160,12 @@ crash_cfg_set_params(const char *host, bool is_enabled) if (strcmp(feedback_host, host) != 0) { strlcpy_a(feedback_host, host); - if (strlen(feedback_host) < strlen(host)) - pr_panic("feedback_host is too long"); + /* + * The caller should have tested already + * that there is enough space to keep + * the host address. + */ + assert(strlen(feedback_host) == strlen(host)); } if (!send_crashinfo) { @@ -231,6 +240,31 @@ crash_mark_env_mode(void) return 0; } +/** + * Zap reserved symbols. + */ +static char * +json_zap(char *s) +{ + if (s == NULL) + return NULL; + + /* + * Actually we should escape them but for + * now lets just zap without changing source + * size. + */ + for (size_t i = 0; i < strlen(s); i++) { + if (s[i] == '\"' || s[i] == '\b' || + s[i] == '\f' || s[i] == '\n' || + s[i] == '\r' || s[i] == '\t' || + s[i] == '\\') { + s[i] = ' '; + } + } + return s; +} + /** * Report crash information to the feedback daemon * (ie send it to feedback daemon). @@ -242,14 +276,14 @@ crash_report_feedback_daemon(struct crash_info *cinfo) return; /* - * Update to a new number if format get changed. + * Update to a new number if the format get changed. */ const int crashinfo_version = 1; char *p = static_alloc(SMALL_STATIC_SIZE); - char *e = &p[SMALL_STATIC_SIZE - 1]; + char *tail = &p[SMALL_STATIC_SIZE]; + char *e = &p[SMALL_STATIC_SIZE]; char *head = p; - char *tail = &p[SMALL_STATIC_SIZE - 8]; /* * Note that while we encode the report we @@ -257,19 +291,19 @@ crash_report_feedback_daemon(struct crash_info *cinfo) * buffer as a temporary store. */ -#define snprintf_safe(__end, __fmt, ...) \ - do { \ - size_t size = (char *)(void *)__end - p; \ - p += snprintf(p, size, __fmt, ##__VA_ARGS__); \ - if (p >= (char *)(void *)__end) \ - goto out; \ +#define snprintf_safe(__end, __fmt, ...) \ + do { \ + size_t size = (char *)__end - p; \ + p += snprintf(p, size, __fmt, ##__VA_ARGS__); \ + if (p >= (char *)__end) \ + goto out; \ } while (0) /* * Lets reuse tail of the buffer as a temp space. */ struct utsname *uname_ptr = (void *)&tail[-sizeof(struct utsname)]; - if (p >= (char *)(void *)uname_ptr) + if (p >= (char *)uname_ptr) goto out; if (uname(uname_ptr) != 0) { @@ -277,15 +311,31 @@ crash_report_feedback_daemon(struct crash_info *cinfo) memset(uname_ptr, 0, sizeof(struct utsname)); } + /* + * Start filling the script. The "data" key value is + * filled as a separate code block for easier + * modifications in future. + */ + snprintf_safe(uname_ptr, + "require(\'http.client\').post(\'%s\'," + "'{\"crashdump\":{\"version\":\"%d\"," + "\"data\":", feedback_host, + crashinfo_version); + + /* The "data" key value */ snprintf_safe(uname_ptr, "{"); snprintf_safe(uname_ptr, "\"uname\":{"); - snprintf_safe(uname_ptr, "\"sysname\":\"%s\",", uname_ptr->sysname); + snprintf_safe(uname_ptr, "\"sysname\":\"%s\",", + json_zap(uname_ptr->sysname)); /* - * nodename might a sensitive information, skip. + * nodename might contain a sensitive information, skip. */ - snprintf_safe(uname_ptr, "\"release\":\"%s\",", uname_ptr->release); - snprintf_safe(uname_ptr, "\"version\":\"%s\",", uname_ptr->version); - snprintf_safe(uname_ptr, "\"machine\":\"%s\"", uname_ptr->machine); + snprintf_safe(uname_ptr, "\"release\":\"%s\",", + json_zap(uname_ptr->release)); + snprintf_safe(uname_ptr, "\"version\":\"%s\",", + json_zap(uname_ptr->version)); + snprintf_safe(uname_ptr, "\"machine\":\"%s\"", + json_zap(uname_ptr->machine)); snprintf_safe(uname_ptr, "},"); snprintf_safe(e, "\"build\":{"); @@ -326,36 +376,23 @@ crash_report_feedback_daemon(struct crash_info *cinfo) #endif /* 64 bytes should be enough for longest localtime */ - char *timestamp_rt_str = &tail[-64]; + const int ts_size = 64; + char *timestamp_rt_str = &tail[-ts_size]; if (p >= timestamp_rt_str) goto out; - ns_to_localtime(cinfo->timestamp_rt, timestamp_rt_str, 64); - snprintf_safe(timestamp_rt_str, "\"timestamp\":\"%s\"", timestamp_rt_str); + ns_to_localtime(cinfo->timestamp_rt, timestamp_rt_str, ts_size); + snprintf_safe(timestamp_rt_str, "\"timestamp\":\"%s\"", + json_zap(timestamp_rt_str)); snprintf_safe(timestamp_rt_str, "}"); snprintf_safe(timestamp_rt_str, "}"); - size_t report_len = p - head; - size_t report_elen = base64_bufsize(report_len, BASE64_NOWRAP); - - char *report_base64 = &tail[-report_elen]; - if (p >= report_base64) - goto out; - base64_encode(head, report_len, report_base64, - report_elen, BASE64_NOWRAP); - report_base64[report_elen] = '\0'; - /* - * Encoded report now sits at report_base64 position, - * at the tail of 'small' static buffer. Lets prepare - * the script to run. + * Finalize the "data" key and the script. + * + * The timeout is choosen to be 1 second as + * main feedback daemon uses. */ - p = head; - snprintf_safe(report_base64, - "require(\'http.client\').post(\'%s\'," - "'{\"crashdump\":{\"version\":\"%d\"," - "\"data\": \"%s\"}}',{timeout=1});" - "os.exit(1);", feedback_host, - crashinfo_version, report_base64); + snprintf_safe(e, "}}',{timeout=1});os.exit(1);"); pr_debug("crashinfo script: %s", head); diff --git a/src/lib/core/crash.h b/src/lib/core/crash.h index 800d525e5..195aef10b 100644 --- a/src/lib/core/crash.h +++ b/src/lib/core/crash.h @@ -5,12 +5,6 @@ */ #pragma once -#include -#include -#include - -#include "trivia/config.h" - #if defined(__cplusplus) extern "C" { #endif /* defined(__cplusplus) */ @@ -31,7 +25,7 @@ crash_init(const char *tarantool_bin); * Configure crash parameters. */ void -crash_cfg_set_params(const char *host, bool is_enabled); +crash_cfg(const char *host, bool is_enabled); /** * Initialize crash signal handlers. diff --git a/src/trivia/util.h b/src/trivia/util.h index b8367cc77..15f9881db 100644 --- a/src/trivia/util.h +++ b/src/trivia/util.h @@ -452,7 +452,7 @@ fpconv_strtod(const char *nptr, char **endptr) #ifndef HAVE_STRLCPY /** - * Copy string. Unlike \a strncpy the result string + * Copy string. Unlike @a strncpy the result string * is always null-terminated. * * @param dst destination buffer.