From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: Cyrill Gorcunov <gorcunov@gmail.com>, tml <tarantool-patches@dev.tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v5 4/4] crash: report crash data to the feedback server Date: Wed, 23 Dec 2020 19:47:44 +0100 [thread overview] Message-ID: <30a781d7-4f4e-3543-f67e-2fb1f7cce172@tarantool.org> (raw) In-Reply-To: <20201223154155.234884-5-gorcunov@gmail.com> 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);
next prev parent reply other threads:[~2020-12-23 18:47 UTC|newest] Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-12-23 15:41 [Tarantool-patches] [PATCH v5 0/4] Our feedback daemon sends only a few portions of usage Cyrill Gorcunov 2020-12-23 15:41 ` [Tarantool-patches] [PATCH v5 1/4] util: introduce strlcpy helper Cyrill Gorcunov 2020-12-23 15:41 ` [Tarantool-patches] [PATCH v5 2/4] backtrace: allow to specify destination buffer Cyrill Gorcunov 2020-12-23 15:41 ` [Tarantool-patches] [PATCH v5 3/4] crash: move fatal signal handling in Cyrill Gorcunov 2020-12-23 15:41 ` [Tarantool-patches] [PATCH v5 4/4] crash: report crash data to the feedback server Cyrill Gorcunov 2020-12-23 18:47 ` Vladislav Shpilevoy [this message] 2020-12-23 18:57 ` Cyrill Gorcunov 2020-12-23 21:22 ` Cyrill Gorcunov 2020-12-24 13:16 ` Cyrill Gorcunov 2020-12-24 17:15 ` Vladislav Shpilevoy 2020-12-24 17:33 ` Cyrill Gorcunov 2020-12-24 18:22 ` Vladislav Shpilevoy 2020-12-24 18:33 ` Cyrill Gorcunov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=30a781d7-4f4e-3543-f67e-2fb1f7cce172@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=gorcunov@gmail.com \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v5 4/4] crash: report crash data to the feedback server' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox