[Tarantool-patches] [PATCH v5 4/4] crash: report crash data to the feedback server
Cyrill Gorcunov
gorcunov at gmail.com
Thu Dec 24 00:22:53 MSK 2020
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 <sys/types.h>
#include <sys/utsname.h>
-#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);
More information about the Tarantool-patches
mailing list