[Tarantool-patches] [PATCH v5 0/4] Our feedback daemon sends only a few portions of usage
Cyrill Gorcunov
gorcunov at gmail.com
Wed Dec 23 18:41:51 MSK 2020
For this sake, if crash happens we can send it to our
feedback server.
In this series we implement only base functionality and
may extend it later.
I didn't find yet a simple way to test this code anything
but manually.
Any comments are highly appreciated.
v2:
- left for internal use
v3 (by Mons):
- enable sending crash report by default but give
an ability to disable it
- use vfork when running subsequent exeutable of
tarantool to send report, since plain execve won't
produce local crashdump
v4 (by Vlad):
- move all fatal's signal handling to crash.c
- export only those helpers which are really needed
- update backtrace to use size as an arument
- provide strlcpy if not present on the system
- use static allocated buffer for report
- use base64 library instead of own implmentation
v5:
- by Vlad:
- use crash_cfg name for crash engine configuration
- configure crash engine via box helpers
- drop unneded gaps from static allocations
- encode in base64 form the backtrace only no need
for second round
- drop unused headers
- prevent from chained crash storm
- make sure no attempts to send feedback is done
if feedback host was not properly configured
(iow, if box.cfg{} was not even called)
- do not use tnt_raise() but diag_set instead
- by me:
- zap special json characters
Vlad, take a look please, I put interdiff with
gorcunov/gh-5261-crash-report-4-rev2 below (note
it is interdiff with previous version not master
branch).
issue https://github.com/tarantool/tarantool/issues/5261
branch gorcunov/gh-5261-crash-report-5
Cyrill Gorcunov (4):
util: introduce strlcpy helper
backtrace: allow to specify destination buffer
crash: move fatal signal handling in
crash: report crash data to the feedback server
CMakeLists.txt | 1 +
src/box/box.cc | 18 +
src/box/box.h | 1 +
src/box/lua/cfg.cc | 9 +
src/box/lua/load_cfg.lua | 6 +-
src/lib/core/CMakeLists.txt | 3 +-
src/lib/core/backtrace.cc | 12 +-
src/lib/core/backtrace.h | 3 +
src/lib/core/crash.c | 603 ++++++++++++++++++++++++++++++++
src/lib/core/crash.h | 44 +++
src/lib/core/util.c | 14 +
src/main.cc | 139 +-------
src/trivia/config.h.cmake | 5 +
src/trivia/util.h | 15 +
test/app-tap/init_script.result | 1 +
test/box/admin.result | 2 +
test/box/cfg.result | 4 +
17 files changed, 739 insertions(+), 141 deletions(-)
create mode 100644 src/lib/core/crash.c
create mode 100644 src/lib/core/crash.h
base-commit: 28f3b2f1e845aff49048d92f9062a4dfa365bf57
--
diff --git a/src/box/box.cc b/src/box/box.cc
index 27079fd46..440bfa305 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -1214,19 +1214,21 @@ box_set_prepared_stmt_cache_size(void)
return 0;
}
-void
-box_set_crash_params(void)
+int
+box_set_crash(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",
+ diag_set(ClientError, ER_CFG, "feedback_host",
"the address is too long");
+ return -1;
}
- crash_cfg_set_params(host, is_enabled_1 && is_enabled_2);
+ crash_cfg(host, is_enabled_1 && is_enabled_2);
+ return 0;
}
/* }}} configuration bindings */
diff --git a/src/box/box.h b/src/box/box.h
index 6792ade95..69fa096a1 100644
--- a/src/box/box.h
+++ b/src/box/box.h
@@ -257,7 +257,7 @@ void box_set_replication_sync_timeout(void);
void box_set_replication_skip_conflict(void);
void box_set_replication_anon(void);
void box_set_net_msg_max(void);
-void box_set_crash_params(void);
+int box_set_crash(void);
int
box_set_prepared_stmt_cache_size(void);
diff --git a/src/box/lua/cfg.cc b/src/box/lua/cfg.cc
index 089c770e8..2d3ccbf0e 100644
--- a/src/box/lua/cfg.cc
+++ b/src/box/lua/cfg.cc
@@ -376,13 +376,10 @@ lbox_cfg_set_replication_skip_conflict(struct lua_State *L)
}
static int
-lbox_cfg_set_crash_params(struct lua_State *L)
+lbox_cfg_set_crash(struct lua_State *L)
{
- try {
- box_set_crash_params();
- } catch (Exception *) {
+ if (box_set_crash() != 0)
luaT_error(L);
- }
return 0;
}
@@ -422,7 +419,7 @@ box_lua_cfg_init(struct lua_State *L)
{"cfg_set_replication_anon", lbox_cfg_set_replication_anon},
{"cfg_set_net_msg_max", lbox_cfg_set_net_msg_max},
{"cfg_set_sql_cache_size", lbox_set_prepared_stmt_cache_size},
- {"cfg_set_crash_params", lbox_cfg_set_crash_params},
+ {"cfg_set_crash", lbox_cfg_set_crash},
{NULL, NULL}
};
diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
index 80009c7af..7e41e0999 100644
--- a/src/box/lua/load_cfg.lua
+++ b/src/box/lua/load_cfg.lua
@@ -34,7 +34,7 @@ end
local ifdef_feedback_set_params =
private.feedback_daemon ~= nil and
private.feedback_daemon.set_feedback_params and
- private.cfg_set_crash_params or nil
+ private.cfg_set_crash or nil
-- all available options
local default_cfg = {
diff --git a/src/lib/core/crash.c b/src/lib/core/crash.c
index f6dd91987..19d3d49ea 100644
--- a/src/lib/core/crash.c
+++ b/src/lib/core/crash.c
@@ -5,7 +5,9 @@
*/
#include <string.h>
-#include <unistd.h>
+#include <signal.h>
+#include <stdint.h>
+#include <limits.h>
#include <time.h>
#include <sys/types.h>
#include <sys/utsname.h>
@@ -13,6 +15,7 @@
#include "third_party/base64.h"
#include "small/static.h"
#include "trivia/util.h"
+
#include "backtrace.h"
#include "crash.h"
#include "say.h"
@@ -25,7 +28,7 @@
#define pr_crit(fmt, ...) fprintf(stderr, pr_fmt(fmt) "\n", ##__VA_ARGS__)
#define pr_panic(fmt, ...) panic(pr_fmt(fmt), ##__VA_ARGS__)
-/* Use strlcpy with destination as an array */
+/** Use strlcpy with destination as an array */
#define strlcpy_a(dst, src) strlcpy(dst, src, sizeof(dst))
#ifdef TARGET_OS_LINUX
@@ -67,8 +70,10 @@ struct crash_greg {
static struct crash_info {
/**
* These two are mostly useless as being
- * plain addresses but keep for backward
- * compatibility.
+ * plain addresses and without real binary
+ * crash dump file we can't use them for
+ * anything suitable (in terms of analysis sake)
+ * but keep for backward compatibility.
*/
void *context_addr;
void *siginfo_addr;
@@ -95,7 +100,7 @@ static struct crash_info {
*/
int sicode;
#ifdef ENABLE_BACKTRACE
- /*
+ /**
* 1K of memory should be enough to keep the backtrace.
* In worst case it gonna be simply trimmed.
*/
@@ -142,7 +147,7 @@ crash_init(const char *tarantool_bin)
}
void
-crash_cfg_set_params(const char *host, bool is_enabled)
+crash_cfg(const char *host, bool is_enabled)
{
if (host == NULL || !is_enabled) {
if (send_crashinfo) {
@@ -155,8 +160,12 @@ crash_cfg_set_params(const char *host, bool is_enabled)
if (strcmp(feedback_host, host) != 0) {
strlcpy_a(feedback_host, host);
- if (strlen(feedback_host) < strlen(host))
- pr_panic("feedback_host is too long");
+ /*
+ * The caller should have tested already
+ * that there is enough space to keep
+ * the host address.
+ */
+ assert(strlen(feedback_host) == strlen(host));
}
if (!send_crashinfo) {
@@ -231,6 +240,31 @@ crash_mark_env_mode(void)
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).
@@ -242,14 +276,14 @@ crash_report_feedback_daemon(struct crash_info *cinfo)
return;
/*
- * Update to a new number if format get changed.
+ * Update to a new number if the format get changed.
*/
const int crashinfo_version = 1;
char *p = static_alloc(SMALL_STATIC_SIZE);
- char *e = &p[SMALL_STATIC_SIZE - 1];
+ char *tail = &p[SMALL_STATIC_SIZE];
+ char *e = &p[SMALL_STATIC_SIZE];
char *head = p;
- char *tail = &p[SMALL_STATIC_SIZE - 8];
/*
* Note that while we encode the report we
@@ -257,19 +291,19 @@ crash_report_feedback_daemon(struct crash_info *cinfo)
* buffer as a temporary store.
*/
-#define snprintf_safe(__end, __fmt, ...) \
- do { \
- size_t size = (char *)(void *)__end - p; \
- p += snprintf(p, size, __fmt, ##__VA_ARGS__); \
- if (p >= (char *)(void *)__end) \
- goto out; \
+#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)
/*
* 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)
+ if (p >= (char *)uname_ptr)
goto out;
if (uname(uname_ptr) != 0) {
@@ -277,15 +311,31 @@ crash_report_feedback_daemon(struct crash_info *cinfo)
memset(uname_ptr, 0, sizeof(struct utsname));
}
+ /*
+ * Start filling the script. The "data" key value is
+ * filled as a separate code block for easier
+ * modifications in future.
+ */
+ snprintf_safe(uname_ptr,
+ "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\",", uname_ptr->sysname);
+ snprintf_safe(uname_ptr, "\"sysname\":\"%s\",",
+ json_zap(uname_ptr->sysname));
/*
- * nodename might a sensitive information, skip.
+ * nodename might contain a sensitive information, skip.
*/
- 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, "\"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\":{");
@@ -326,36 +376,23 @@ crash_report_feedback_daemon(struct crash_info *cinfo)
#endif
/* 64 bytes should be enough for longest localtime */
- char *timestamp_rt_str = &tail[-64];
+ const int ts_size = 64;
+ char *timestamp_rt_str = &tail[-ts_size];
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);
+ 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_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.
+ * Finalize the "data" key and the script.
+ *
+ * The timeout is choosen to be 1 second as
+ * main feedback daemon uses.
*/
- 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);
+ snprintf_safe(e, "}}',{timeout=1});os.exit(1);");
pr_debug("crashinfo script: %s", head);
diff --git a/src/lib/core/crash.h b/src/lib/core/crash.h
index 800d525e5..195aef10b 100644
--- a/src/lib/core/crash.h
+++ b/src/lib/core/crash.h
@@ -5,12 +5,6 @@
*/
#pragma once
-#include <stdint.h>
-#include <signal.h>
-#include <limits.h>
-
-#include "trivia/config.h"
-
#if defined(__cplusplus)
extern "C" {
#endif /* defined(__cplusplus) */
@@ -31,7 +25,7 @@ crash_init(const char *tarantool_bin);
* Configure crash parameters.
*/
void
-crash_cfg_set_params(const char *host, bool is_enabled);
+crash_cfg(const char *host, bool is_enabled);
/**
* Initialize crash signal handlers.
diff --git a/src/trivia/util.h b/src/trivia/util.h
index b8367cc77..15f9881db 100644
--- a/src/trivia/util.h
+++ b/src/trivia/util.h
@@ -452,7 +452,7 @@ fpconv_strtod(const char *nptr, char **endptr)
#ifndef HAVE_STRLCPY
/**
- * Copy string. Unlike \a strncpy the result string
+ * Copy string. Unlike @a strncpy the result string
* is always null-terminated.
*
* @param dst destination buffer.
More information about the Tarantool-patches
mailing list