[Tarantool-patches] [PATCH v5 4/4] crash: report crash data to the feedback server
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed Dec 23 21:47:44 MSK 2020
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);
More information about the Tarantool-patches
mailing list