Re: [Tarantool-patches] [PATCH v5 4/4] crash: report crash data to the feedback server

[Cyrill Gorcunov <gorcunov@gmail.com>]

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);