Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v5 0/4] Our feedback daemon sends only a few portions of usage
@ 2020-12-23 15:41 Cyrill Gorcunov
  2020-12-23 15:41 ` [Tarantool-patches] [PATCH v5 1/4] util: introduce strlcpy helper Cyrill Gorcunov
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Cyrill Gorcunov @ 2020-12-23 15:41 UTC (permalink / raw)
  To: tml; +Cc: Vladislav Shpilevoy

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-12-24 18:33 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox