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 C842B4765E0 for ; Wed, 23 Dec 2020 21:47:45 +0300 (MSK) References: <20201223154155.234884-1-gorcunov@gmail.com> <20201223154155.234884-5-gorcunov@gmail.com> From: Vladislav Shpilevoy Message-ID: <30a781d7-4f4e-3543-f67e-2fb1f7cce172@tarantool.org> Date: Wed, 23 Dec 2020 19:47:44 +0100 MIME-Version: 1.0 In-Reply-To: <20201223154155.234884-5-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Cyrill Gorcunov , tml Hi! Thanks for the patch! See 3 comments and fixes below, and top of the branch in a separate commit. > diff --git a/src/lib/core/crash.c b/src/lib/core/crash.c > index 3929463f3..19d3d49ea 100644 > --- a/src/lib/core/crash.c > +++ b/src/lib/core/crash.c > @@ -130,6 +218,224 @@ 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) 1. You don't need to change 'env'. Because in the new process send_crashinfo will be false. Therefore it won't send the crash anyway. > +{ > + 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) 2. Why can't you simply call json_escape(), which we already have? Yes, it may add a few more bytes, but it is not something super bad. json_escape() returns number of written bytes, so you can use it quite easy. ==================== @@ -325,8 +325,11 @@ crash_report_feedback_daemon(struct crash_info *cinfo) /* 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(uname_ptr, "\"sysname\":\""); + p += json_escape(p, (char *)uname_ptr - p, uname_ptr->sysname); + if (p >= (char *)uname_ptr) + goto out; + snprintf_safe(uname_ptr, "\","); ==================== It would look even simpler, if you would use the existing SNPRINT() macros. It returns -1 on fail, but you could patch it or introduce SNPRINT_GO which would goto to a given label. Or keep return -1, and do it like in sio_socketname_to_buffer() - it has a separate function to do all the prints. I tried to do that in my diff. > +{ > + 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++) { 3. Strlen() is going to be called on each iteration. Please, save the result into a variable. Or just use json_escape(). > + if (s[i] == '\"' || s[i] == '\b' || > + s[i] == '\f' || s[i] == '\n' || > + s[i] == '\r' || s[i] == '\t' || > + s[i] == '\\') { > + s[i] = ' '; > + } > + } Consider my fixes below and on top of the branch in a separate commit. I didn't test them though. So there are likely to be a few typos. ==================== diff --git a/src/lib/core/crash.c b/src/lib/core/crash.c index 914c84a1b..4b821d44a 100644 --- a/src/lib/core/crash.c +++ b/src/lib/core/crash.c @@ -218,63 +218,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. */ @@ -291,20 +241,17 @@ crash_report_feedback_daemon(struct crash_info *cinfo) * buffer as a temporary store. */ -#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) + int total = 0; + int size = 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,48 +263,53 @@ 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\":\""); - p += json_escape(p, (char *)uname_ptr - p, uname_ptr->sysname); - if (p >= (char *)uname_ptr) - goto out; - snprintf_safe(uname_ptr, "\","); + 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); } @@ -371,23 +323,28 @@ crash_report_feedback_daemon(struct crash_info *cinfo) size_t bt_elen = base64_bufsize(bt_len, BASE64_NOWRAP); char *bt_base64 = &tail[-bt_elen]; if (p >= bt_base64) - goto out; + return -1; 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); + + size = bt_base64 - p; + snprintf_safe("\"backtrace\":\"%s\",", bt_base64); #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. @@ -395,7 +352,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); @@ -413,30 +371,22 @@ 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 * is no simple way to make sure from inside * of a signal crash handler. So just hope it * is running fine. */ - execve(exec_argv[0], exec_argv, environ); + execve(exec_argv[0], exec_argv, NULL); pr_crit("exec(%s,[%s,%s,%s]) failed", exec_argv[0], exec_argv[0], exec_argv[1], exec_argv[2]); _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; } /** @@ -545,8 +495,8 @@ 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);