From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f43.google.com (mail-lf1-f43.google.com [209.85.167.43]) (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 4B4754765E0 for ; Thu, 24 Dec 2020 00:22:57 +0300 (MSK) Received: by mail-lf1-f43.google.com with SMTP id m12so526666lfo.7 for ; Wed, 23 Dec 2020 13:22:57 -0800 (PST) Date: Thu, 24 Dec 2020 00:22:53 +0300 From: Cyrill Gorcunov Message-ID: <20201223212253.GA298600@grain> References: <20201223154155.234884-1-gorcunov@gmail.com> <20201223154155.234884-5-gorcunov@gmail.com> <30a781d7-4f4e-3543-f67e-2fb1f7cce172@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <30a781d7-4f4e-3543-f67e-2fb1f7cce172@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v5 4/4] crash: report crash data to the feedback server List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tml On Wed, Dec 23, 2020 at 07:47:44PM +0100, Vladislav Shpilevoy wrote: > Hi! Thanks for the patch! > > See 3 comments and fixes below, and top of the branch in a > separate commit. > Vlad, since we can use json_escape I dropped usage of base64 encoder for backtrace. The final interdiff I pushed into gorcunov/gh-5261-crash-report-6 is the following -- diff --git a/src/lib/core/CMakeLists.txt b/src/lib/core/CMakeLists.txt index 358e98ea7..30cf0dd15 100644 --- a/src/lib/core/CMakeLists.txt +++ b/src/lib/core/CMakeLists.txt @@ -40,7 +40,7 @@ endif() add_library(core STATIC ${core_sources}) -target_link_libraries(core salad small uri decNumber bit misc ${LIBEV_LIBRARIES} +target_link_libraries(core salad small uri decNumber bit ${LIBEV_LIBRARIES} ${LIBEIO_LIBRARIES} ${LIBCORO_LIBRARIES} ${MSGPUCK_LIBRARIES} ${ICU_LIBRARIES}) diff --git a/src/lib/core/crash.c b/src/lib/core/crash.c index 19d3d49ea..e62af6836 100644 --- a/src/lib/core/crash.c +++ b/src/lib/core/crash.c @@ -12,7 +12,6 @@ #include #include -#include "third_party/base64.h" #include "small/static.h" #include "trivia/util.h" @@ -218,63 +217,13 @@ crash_collect(int signo, siginfo_t *siginfo, void *ucontext) return cinfo; } -/** - * Mark an environment that we're in crashinfo handling, this - * allows us to escape recursive attempts to send report, - * if the action of sending report is failing itself. - */ -static int -crash_mark_env_mode(void) -{ - const char *env_name = "TT_CRASHINFO_MODE"; - if (getenv(env_name) != NULL) { - pr_crit("recursive failure detected"); - return -1; - } - - if (setenv(env_name, "y", 0) != 0) { - pr_crit("unable to setup %s", env_name); - return -1; - } - - 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). */ -static void +static int crash_report_feedback_daemon(struct crash_info *cinfo) { - if (crash_mark_env_mode() != 0) - return; - /* * Update to a new number if the format get changed. */ @@ -285,26 +234,18 @@ crash_report_feedback_daemon(struct crash_info *cinfo) char *e = &p[SMALL_STATIC_SIZE]; char *head = p; - /* - * Note that while we encode the report we - * intensively use a tail of the allocated - * buffer as a temporary store. - */ + int total = 0; + int size = 0; -#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) +#define snprintf_safe(...) SNPRINT(total, snprintf, p, size, __VA_ARGS__) +#define jnprintf_safe(str) SNPRINT(total, json_escape, p, size, str) /* * Lets reuse tail of the buffer as a temp space. */ struct utsname *uname_ptr = (void *)&tail[-sizeof(struct utsname)]; if (p >= (char *)uname_ptr) - goto out; + return -1; if (uname(uname_ptr) != 0) { pr_syserr("uname call failed, ignore"); @@ -316,75 +257,75 @@ crash_report_feedback_daemon(struct crash_info *cinfo) * filled as a separate code block for easier * modifications in future. */ - snprintf_safe(uname_ptr, - "require(\'http.client\').post(\'%s\'," + size = (char *)uname_ptr - p; + snprintf_safe("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\",", - json_zap(uname_ptr->sysname)); + snprintf_safe("{"); + snprintf_safe("\"uname\":{"); + snprintf_safe("\"sysname\":\""); + jnprintf_safe(uname_ptr->sysname); + snprintf_safe("\","); /* * nodename might contain a sensitive information, skip. */ - 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\":{"); - snprintf_safe(e, "\"version\":\"%s\",", PACKAGE_VERSION); - snprintf_safe(e, "\"cmake_type\":\"%s\"", BUILD_INFO); - snprintf_safe(e, "},"); - - snprintf_safe(e, "\"signal\":{"); - snprintf_safe(e, "\"signo\":%d,", cinfo->signo); - snprintf_safe(e, "\"si_code\":%d,", cinfo->sicode); + snprintf_safe("\"release\":\""); + jnprintf_safe(uname_ptr->release); + snprintf_safe("\","); + + snprintf_safe("\"version\":\""); + jnprintf_safe(uname_ptr->version); + snprintf_safe("\","); + + snprintf_safe("\"machine\":\""); + jnprintf_safe(uname_ptr->machine); + snprintf_safe("\""); + snprintf_safe("},"); + + /* Extend size, because now uname_ptr is not needed. */ + size = e - p; + snprintf_safe("\"build\":{"); + snprintf_safe("\"version\":\"%s\",", PACKAGE_VERSION); + snprintf_safe("\"cmake_type\":\"%s\"", BUILD_INFO); + snprintf_safe("},"); + + snprintf_safe("\"signal\":{"); + snprintf_safe("\"signo\":%d,", cinfo->signo); + snprintf_safe("\"si_code\":%d,", cinfo->sicode); if (cinfo->signo == SIGSEGV) { if (cinfo->sicode == SEGV_MAPERR) { - snprintf_safe(e, "\"si_code_str\":\"%s\",", + snprintf_safe("\"si_code_str\":\"%s\",", "SEGV_MAPERR"); } else if (cinfo->sicode == SEGV_ACCERR) { - snprintf_safe(e, "\"si_code_str\":\"%s\",", + snprintf_safe("\"si_code_str\":\"%s\",", "SEGV_ACCERR"); } - snprintf_safe(e, "\"si_addr\":\"0x%llx\",", + snprintf_safe("\"si_addr\":\"0x%llx\",", (long long)cinfo->siaddr); } #ifdef ENABLE_BACKTRACE - /* - * The backtrace itself is encoded into base64 form - * since it might have arbitrary symbols not suitable - * for json encoding (newlines and etc). - */ - size_t bt_len = strlen(cinfo->backtrace_buf); - size_t bt_elen = base64_bufsize(bt_len, BASE64_NOWRAP); - char *bt_base64 = &tail[-bt_elen]; - if (p >= bt_base64) - goto out; - base64_encode(cinfo->backtrace_buf, bt_len, - bt_base64, bt_elen, BASE64_NOWRAP); - bt_base64[bt_elen] = '\0'; - snprintf_safe(bt_base64, "\"backtrace\":\"%s\",", bt_base64); + snprintf_safe("\"backtrace\":\""); + jnprintf_safe(cinfo->backtrace_buf); + snprintf_safe("\","); #endif /* 64 bytes should be enough for longest localtime */ const int ts_size = 64; char *timestamp_rt_str = &tail[-ts_size]; if (p >= timestamp_rt_str) - goto out; + return -1; 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 = timestamp_rt_str - p; + snprintf_safe("\"timestamp\":\""); + jnprintf_safe(timestamp_rt_str); + snprintf_safe("\""); + snprintf_safe("}"); + snprintf_safe("}"); /* * Finalize the "data" key and the script. @@ -392,7 +333,8 @@ crash_report_feedback_daemon(struct crash_info *cinfo) * The timeout is choosen to be 1 second as * main feedback daemon uses. */ - snprintf_safe(e, "}}',{timeout=1});os.exit(1);"); + size = e - p; + snprintf_safe("}}',{timeout=1});os.exit(1);"); pr_debug("crashinfo script: %s", head); @@ -410,11 +352,6 @@ crash_report_feedback_daemon(struct crash_info *cinfo) */ pid_t pid = vfork(); if (pid == 0) { - /* - * Environment is needed for recursive - * crash protection. See crash_mark_env_mode - * above. - */ extern char **environ; /* * The script must exit at the end but there @@ -429,11 +366,10 @@ crash_report_feedback_daemon(struct crash_info *cinfo) _exit(1); } else if (pid < 0) { pr_crit("unable to vfork (errno %d)", errno); + return -1; } - return; -out: - pr_crit("unable to prepare a crash report"); + return 0; } /** @@ -542,8 +478,10 @@ crash_signal_cb(int signo, siginfo_t *siginfo, void *context) in_cb = 1; cinfo = crash_collect(signo, siginfo, context); crash_report_stderr(cinfo); - if (send_crashinfo) - crash_report_feedback_daemon(cinfo); + if (send_crashinfo && + crash_report_feedback_daemon(cinfo) != 0) { + pr_crit("unable to send a crash report"); + } } else { /* Got a signal while running the handler. */ fprintf(stderr, "Fatal %d while backtracing", signo);