From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 32D2C45C304 for ; Sun, 20 Dec 2020 18:16:48 +0300 (MSK) References: <20201210161832.729439-1-gorcunov@gmail.com> <20201210161832.729439-5-gorcunov@gmail.com> <3d201857-f809-c758-297c-e3e896fbf06c@tarantool.org> <20201216111606.GA14556@grain> <20201216203106.GC14556@grain> From: Vladislav Shpilevoy Message-ID: <96e492a5-3866-91ac-801b-c26c1fce96e2@tarantool.org> Date: Sun, 20 Dec 2020 16:16:46 +0100 MIME-Version: 1.0 In-Reply-To: <20201216203106.GC14556@grain> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v4 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: Cyrill Gorcunov Cc: TML I see you didn't push it on the branch. Please, do. Otherwise I can't validate if it works. See 6 comments below. > diff --git a/src/box/box.cc b/src/box/box.cc > index a8bc3471d..27079fd46 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -1213,6 +1214,21 @@ box_set_prepared_stmt_cache_size(void) > return 0; > } > > +void > +box_set_crash_params(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", > + "the address is too long"); 1. Please, don't use exceptions in the new code. We are getting rid of them, and such changes complicate it in future. > + } > + > + crash_cfg_set_params(host, is_enabled_1 && is_enabled_2); 2. Just 'crash_cfg', please. We have lots of 'cfg' functions for various modules, and none of them has 'set_params' suffix. Probably because this is what 'cfg' means anyway. > +} > diff --git a/src/lib/core/crash.c b/src/lib/core/crash.c > index a15a13e8e..f6dd91987 100644 > --- a/src/lib/core/crash.c > +++ b/src/lib/core/crash.c > @@ -81,13 +96,75 @@ static struct crash_info { > int sicode; > #ifdef ENABLE_BACKTRACE > /* > - * 4K of memory should be enough to keep the backtrace. > + * 1K of memory should be enough to keep the backtrace. > * In worst case it gonna be simply trimmed. > */ > - char backtrace_buf[4096]; > + char backtrace_buf[1024]; 3. I am begging you. Please. Stop mixing independent changes with each other. Please. How can I ask otherwise? What can I do to explain this? How can I help to understand? > #endif> @@ -126,6 +209,196 @@ 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; > +} > + > +/** > + * Report crash information to the feedback daemon > + * (ie send it to feedback daemon). > + */ > +static void > +crash_report_feedback_daemon(struct crash_info *cinfo) > +{ > + if (crash_mark_env_mode() != 0) > + return; > + > + /* > + * Update to a new number if format get changed. > + */ > + const int crashinfo_version = 1; > + > + char *p = static_alloc(SMALL_STATIC_SIZE); > + char *e = &p[SMALL_STATIC_SIZE - 1]; > + char *head = p; > + char *tail = &p[SMALL_STATIC_SIZE - 8]; > + > + /* > + * Note that while we encode the report we > + * intensively use a tail of the allocated > + * buffer as a temporary store. > + */ > + > +#define snprintf_safe(__end, __fmt, ...) \ > + do { \ > + size_t size = (char *)(void *)__end - p; \ 4. Please, remove the double cast. You can use (char *) as is. Here and in other places. > + p += snprintf(p, size, __fmt, ##__VA_ARGS__); \ > + if (p >= (char *)(void *)__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) > + goto out; > + > + if (uname(uname_ptr) != 0) { > + pr_syserr("uname call failed, ignore"); > + memset(uname_ptr, 0, sizeof(struct utsname)); > + } > + > + snprintf_safe(uname_ptr, "{"); > + snprintf_safe(uname_ptr, "\"uname\":{"); > + snprintf_safe(uname_ptr, "\"sysname\":\"%s\",", uname_ptr->sysname); > + /* > + * nodename might a sensitive information, skip. 5. 'might' what? You missed a verb. > + */ > + 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, "},"); > + > + 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); > + if (cinfo->signo == SIGSEGV) { > + if (cinfo->sicode == SEGV_MAPERR) { > + snprintf_safe(e, "\"si_code_str\":\"%s\",", > + "SEGV_MAPERR"); > + } else if (cinfo->sicode == SEGV_ACCERR) { > + snprintf_safe(e, "\"si_code_str\":\"%s\",", > + "SEGV_ACCERR"); > + } > + snprintf_safe(e, "\"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); > +#endif > + > + /* 64 bytes should be enough for longest localtime */ > + char *timestamp_rt_str = &tail[-64]; > + 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); > + 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. > + */ > + 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); 6. I think now I understood all these buffer allocs. Except that I still don't understand why is tail initialized as char *tail = &p[SMALL_STATIC_SIZE - 8]; instead of just `tail = end;`.