From: Alberto Leiva Popper <ydahhrk@gmail.com>
Date: Mon, 21 Jun 2021 21:14:50 +0000 (-0500)
Subject: Global: Ignore SIGPIPE
X-Git-Tag: v1.5.1~12
X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=b42de525cf6a92c3207e7e2f73f9cad0f98e8a55;p=thirdparty%2FFORT-validator.git

Global: Ignore SIGPIPE

I think I finally found #49. The signal is not SIGSYS; it's SIGPIPE.
That's why dorpauli was getting no core dumps.

Apparently, this is a typical newbie trap for libcurl users.
CURLOPT_NOSIGNAL is incapable of supressing all SIGPIPEs, because some
of them are OS-generated.

I can't believe how dumb SIGPIPE has turned out to be. I/O functions
should return EPIPE; not interrupt the whole program to a handler that
defaults to "die silently."

What the hell, POSIX.
---

diff --git a/src/http/http.c b/src/http/http.c
index 4249f94a..793d9826 100644
--- a/src/http/http.c
+++ b/src/http/http.c
@@ -10,11 +10,6 @@
 #include "file.h"
 #include "log.h"
 
-#define TA_DEBUG \
-	do { if (is_ta) PR_DEBUG; } while (0)
-#define TA_DEBUG_MSG(fmt, ...) \
-	do { if (is_ta) PR_DEBUG_MSG(fmt, ##__VA_ARGS__); } while (0)
-
 /* HTTP Response Code 200 (OK) */
 #define HTTP_OK			200
 /* HTTP Response Code 304 (Not Modified) */
@@ -151,31 +146,22 @@ http_fetch(struct http_handler *handler, char const *uri, long *response_code,
 	CURLcode res, res2;
 	long unmet = 0;
 
-	TA_DEBUG;
-	TA_DEBUG_MSG("%s %d", uri, log_operation);
-
 	handler->errbuf[0] = 0;
 	setopt_str(handler->curl, CURLOPT_URL, uri);
 	setopt_writedata(handler->curl, file);
 
-	TA_DEBUG_MSG("HTTP GET: %s", uri);
 	res = curl_easy_perform(handler->curl);
-	TA_DEBUG_MSG("%d", res);
 
 	res2 = curl_easy_getinfo(handler->curl, CURLINFO_RESPONSE_CODE,
 	    response_code);
-	TA_DEBUG_MSG("%d", res2);
 	if (res2 != CURLE_OK) {
 		return pr_op_err("curl_easy_getinfo(CURLINFO_RESPONSE_CODE) returned %d (%s). I think this is supposed to be illegal, so I'll have to drop URI '%s'.",
 		    res2, curl_err_string(handler, res2), uri);
 	}
 
-	TA_DEBUG_MSG("%ld", *response_code);
 	if (res == CURLE_OK) {
-		if (*response_code != HTTP_OK) {
-			TA_DEBUG;
+		if (*response_code != HTTP_OK)
 			return 0;
-		}
 
 		/*
 		 * Scenario: Received an OK code, but the time condition
@@ -212,26 +198,21 @@ http_fetch(struct http_handler *handler, char const *uri, long *response_code,
 		 * 		unmet: 0
 		 * 		writefunction called
 		 */
-		TA_DEBUG;
 		res = curl_easy_getinfo(handler->curl, CURLINFO_CONDITION_UNMET,
 		    &unmet);
 		if (res == CURLE_OK && unmet == 1)
 			*cond_met = 0;
-		TA_DEBUG_MSG("%ld", unmet);
 		return 0;
 	}
 
 	if (*response_code >= HTTP_BAD_REQUEST) {
-		TA_DEBUG;
 		return pr_val_err("Error requesting URL %s (received HTTP code %ld): %s",
 		    uri, *response_code, curl_err_string(handler, res));
 	}
 
 	pr_val_err("Error requesting URL %s: %s", uri,
 	    curl_err_string(handler, res));
-	TA_DEBUG;
 	if (log_operation) {
-		TA_DEBUG;
 		pr_op_err("Error requesting URL %s: %s", uri,
 		    curl_err_string(handler, res));
 	}
@@ -262,24 +243,16 @@ __http_download_file(struct rpki_uri *uri, long *response_code, long ims_value,
 	*cond_met = 1;
 	if (!config_get_http_enabled()) {
 		*response_code = 0; /* Not 200 code, but also not an error */
-		TA_DEBUG;
 		return 0;
 	}
 
-	TA_DEBUG;
-
 	original_file = uri_get_local(uri);
 	tmp_file = strdup(original_file);
-	if (tmp_file == NULL) {
-		TA_DEBUG;
+	if (tmp_file == NULL)
 		return pr_enomem();
-	}
-
-	TA_DEBUG;
 
 	tmp = realloc(tmp_file, strlen(tmp_file) + strlen(tmp_suffix) + 1);
 	if (tmp == NULL) {
-		TA_DEBUG;
 		error = pr_enomem();
 		goto release_tmp;
 	}
@@ -287,55 +260,36 @@ __http_download_file(struct rpki_uri *uri, long *response_code, long ims_value,
 	tmp_file = tmp;
 	strcat(tmp_file, tmp_suffix);
 
-	TA_DEBUG;
-
 	error = create_dir_recursive(tmp_file);
-	if (error) {
-		TA_DEBUG_MSG("%d", error);
+	if (error)
 		goto release_tmp;
-	}
-
-	TA_DEBUG;
 
 	error = file_write(tmp_file, &out, &stat);
-	if (error) {
-		TA_DEBUG_MSG("%d", error);
+	if (error)
 		goto delete_dir;
-	}
 
 	do {
-		TA_DEBUG;
-
 		error = http_easy_init(&handler);
-		if (error) {
-			TA_DEBUG_MSG("%d", error);
+		if (error)
 			goto close_file;
-		}
 
 		/* Set "If-Modified-Since" header only if a value is specified */
 		if (ims_value > 0) {
-			TA_DEBUG_MSG("%ld", ims_value);
 			setopt_long(handler.curl, CURLOPT_TIMEVALUE, ims_value);
 			setopt_long(handler.curl, CURLOPT_TIMECONDITION,
 			    CURL_TIMECOND_IFMODSINCE);
-			TA_DEBUG;
 		}
 		error = http_fetch(&handler, uri_get_global(uri), response_code,
 		    cond_met, log_operation, out, is_ta);
 		if (error != EREQFAILED) {
-			TA_DEBUG_MSG("%d", error);
 			break; /* Note: Usually happy path */
 		}
 
-		TA_DEBUG_MSG("%d", error);
-
 		if (retries == config_get_http_retry_count()) {
-			TA_DEBUG_MSG("%u", retries);
 			pr_val_warn("Max HTTP retries (%u) reached requesting for '%s', won't retry again.",
 			    retries, uri_get_global(uri));
 			break;
 		}
-		TA_DEBUG;
 		pr_val_warn("Retrying HTTP request '%s' in %u seconds, %u attempts remaining.",
 		    uri_get_global(uri),
 		    config_get_http_retry_interval(),
@@ -348,26 +302,18 @@ __http_download_file(struct rpki_uri *uri, long *response_code, long ims_value,
 	http_easy_cleanup(&handler);
 	file_close(out);
 
-	if (error) {
-		TA_DEBUG_MSG("%d", error);
+	if (error)
 		goto delete_dir;
-	}
-
-	TA_DEBUG;
-	TA_DEBUG_MSG("%s %s", tmp_file, original_file);
 
 	/* Overwrite the original file */
 	error = rename(tmp_file, original_file);
 	if (error) {
 		error = errno;
-		TA_DEBUG_MSG("%d", error);
 		pr_val_errno(error, "Renaming temporal file from '%s' to '%s'",
 		    tmp_file, original_file);
 		goto delete_dir;
 	}
 
-	TA_DEBUG;
-
 	free(tmp_file);
 	return 0;
 close_file:
@@ -376,7 +322,6 @@ delete_dir:
 	delete_dir_recursive_bottom_up(tmp_file);
 release_tmp:
 	free(tmp_file);
-	TA_DEBUG_MSG("%d", error);
 	return ENSURE_NEGATIVE(error);
 }
 
diff --git a/src/log.c b/src/log.c
index b2cb2cd4..83293e43 100644
--- a/src/log.c
+++ b/src/log.c
@@ -109,7 +109,7 @@ static void init_config(struct log_config *cfg)
 }
 
 static void
-signal_handler(int signum)
+sigsegv_handler(int signum)
 {
 	/*
 	 * IMPORTANT: See https://stackoverflow.com/questions/29982643
@@ -125,22 +125,6 @@ signal_handler(int signum)
 #define STACK_SIZE 64
 	void *array[STACK_SIZE];
 	size_t size;
-//	ssize_t trash;
-
-	int tmp;
-	unsigned char digit;
-
-//	/* Don't know why, but casting to void is not silencing the warning. */
-//	trash = write(STDERR_FILENO, title, strlen(title));
-//	trash++;
-
-	tmp = signum;
-	while (tmp != 0) {
-		digit = tmp % 10 + 48;
-		digit = write(STDERR_FILENO, &digit, 1);
-		tmp /= 10;
-	}
-	digit = write(STDERR_FILENO, "\n", 1);
 
 	size = backtrace(array, STACK_SIZE);
 	backtrace_symbols_fd(array, size, STDERR_FILENO);
@@ -150,61 +134,62 @@ signal_handler(int signum)
 	kill(getpid(), signum);
 }
 
-static void
-register_signal_handler(int signum)
-{
-	struct sigaction handler;
-
-	handler.sa_handler = signal_handler;
-	sigemptyset(&handler.sa_mask);
-	handler.sa_flags = 0;
-	sigaction(signum, &handler, NULL);
-}
-
 /*
- * Register stack trace printer on segmentation fault listener.
+ * Register better handlers for some signals.
+ *
  * Remember to enable -rdynamic (See print_stack_trace()).
  */
 static void
 register_signal_handlers(void)
 {
+	struct sigaction action;
 	void* dummy;
 
-	/* Make sure libgcc is loaded; otherwise the handler might allocate. */
+	/*
+	 * Make sure libgcc is loaded; otherwise backtrace() might allocate
+	 * during a signal handler. (Which is illegal.)
+	 */
 	dummy = NULL;
 	backtrace(&dummy, 1);
 
-	register_signal_handler(SIGHUP);
-	register_signal_handler(SIGINT);
-	register_signal_handler(SIGQUIT);
-	register_signal_handler(SIGILL);
-	register_signal_handler(SIGTRAP);
-	register_signal_handler(SIGABRT);
-	register_signal_handler(SIGBUS);
-	register_signal_handler(SIGFPE);
-	/* register_signal_handler(SIGKILL); */
-	register_signal_handler(SIGUSR1);
-	register_signal_handler(SIGSEGV);
-	register_signal_handler(SIGUSR2);
-	register_signal_handler(SIGPIPE);
-	register_signal_handler(SIGALRM);
-	register_signal_handler(SIGTERM);
-	register_signal_handler(SIGSTKFLT);
-	register_signal_handler(SIGCHLD);
-	register_signal_handler(SIGCONT);
-	/* register_signal_handler(SIGSTOP); */
-	register_signal_handler(SIGTSTP);
-	register_signal_handler(SIGTTIN);
-	register_signal_handler(SIGTTOU);
-	register_signal_handler(SIGURG);
-	register_signal_handler(SIGXCPU);
-	register_signal_handler(SIGXFSZ);
-	register_signal_handler(SIGVTALRM);
-	register_signal_handler(SIGPROF);
-	register_signal_handler(SIGWINCH);
-	register_signal_handler(SIGIO);
-	register_signal_handler(SIGPWR);
-	register_signal_handler(SIGSYS);
+	/* Register the segmentation fault handler */
+	memset(&action, 0, sizeof(action));
+	action.sa_handler = sigsegv_handler;
+	sigemptyset(&action.sa_mask);
+	action.sa_flags = 0;
+	if (sigaction(SIGSEGV, &action, NULL) == -1) {
+		/*
+		 * Not fatal; it just means we will not print stack traces on
+		 * Segmentation Faults.
+		 */
+		pr_op_errno(errno, "SIGSEGV handler registration failure");
+	}
+
+	/*
+	 * SIGPIPE is sometimes triggered by libcurl:
+	 *
+	 * > libcurl makes an effort to never cause such SIGPIPEs to trigger,
+	 * > but some operating systems have no way to avoid them and even on
+	 * > those that have there are some corner cases when they may still
+	 * > happen
+	 * (Documentation of CURLOPT_NOSIGNAL)
+	 *
+	 * All SIGPIPE usually means is "the server closed the connection for
+	 * some reason, fuck you."
+	 * Which is a normal I/O error, and should be handled by the normal
+	 * error propagation logic, not by a signal handler.
+	 * So, ignore SIGPIPE.
+	 *
+	 * https://github.com/NICMx/FORT-validator/issues/49
+	 */
+	if (signal(SIGPIPE, SIG_IGN) == SIG_ERR) {
+		/*
+		 * Not fatal. It just means that, if a broken pipe happens, we
+		 * will die silently.
+		 * But they're somewhat rare, so whatever.
+		 */
+		pr_op_errno(errno, "SIGPIPE handler registration failure");
+	}
 }
 
 int
diff --git a/src/rsync/rsync.c b/src/rsync/rsync.c
index f5129a61..4756043e 100644
--- a/src/rsync/rsync.c
+++ b/src/rsync/rsync.c
@@ -15,11 +15,6 @@
 #include "str_token.h"
 #include "thread_var.h"
 
-#define TA_DEBUG \
-	do { if (is_ta) PR_DEBUG; } while (0)
-#define TA_DEBUG_MSG(fmt, ...) \
-	do { if (is_ta) PR_DEBUG_MSG(fmt, ##__VA_ARGS__); } while (0)
-
 struct uri {
 	struct rpki_uri *uri;
 	SLIST_ENTRY(uri) next;
@@ -378,7 +373,6 @@ do_rsync(struct rpki_uri *uri, bool is_ta, bool log_operation)
 	int fork_fds[2][2];
 	pid_t child_pid;
 	unsigned int retries;
-	unsigned int i;
 	int child_status;
 	int error;
 
@@ -389,11 +383,6 @@ do_rsync(struct rpki_uri *uri, bool is_ta, bool log_operation)
 	if (error)
 		return error;
 
-	TA_DEBUG_MSG("Executing RSYNC:");
-	for (i = 0; i < args_len + 1; i++) {
-		TA_DEBUG_MSG("    %s", args[i]);
-	}
-
 	retries = 0;
 	do {
 		child_status = 0;
@@ -401,19 +390,13 @@ do_rsync(struct rpki_uri *uri, bool is_ta, bool log_operation)
 		if (error)
 			goto release_args;
 
-		TA_DEBUG;
-
 		error = create_pipes(fork_fds);
-		if (error) {
-			TA_DEBUG_MSG("%d", error);
+		if (error)
 			goto release_args;
-		}
 
 		/* Flush output (avoid locks between father and child) */
 		log_flush();
 
-		TA_DEBUG;
-
 		/* We need to fork because execvp() magics the thread away. */
 		child_pid = fork();
 		if (child_pid == 0) {
@@ -440,21 +423,15 @@ do_rsync(struct rpki_uri *uri, bool is_ta, bool log_operation)
 			goto release_args;
 		}
 
-		TA_DEBUG;
-
 		/* This code is run by us. */
 		error = read_pipes(fork_fds, log_operation);
-		if (error) {
-			TA_DEBUG;
+		if (error)
 			kill(child_pid, SIGCHLD); /* Stop the child */
-			TA_DEBUG;
-		}
 
 		error = waitpid(child_pid, &child_status, 0);
 		do {
 			if (error == -1) {
 				error = errno;
-				TA_DEBUG_MSG("%d", error);
 				pr_op_err("The rsync sub-process returned error %d (%s)",
 				    error, strerror(error));
 				if (child_status > 0)
@@ -466,7 +443,6 @@ do_rsync(struct rpki_uri *uri, bool is_ta, bool log_operation)
 		if (WIFEXITED(child_status)) {
 			/* Happy path (but also sad path sometimes). */
 			error = WEXITSTATUS(child_status);
-			TA_DEBUG_MSG("%d", error);
 			pr_val_debug("Child terminated with error code %d.",
 			    error);
 			if (!error)
@@ -494,7 +470,6 @@ do_rsync(struct rpki_uri *uri, bool is_ta, bool log_operation)
 	release_args(args, args_len);
 
 	if (WIFSIGNALED(child_status)) {
-		TA_DEBUG_MSG("%d", WTERMSIG(child_status));
 		switch (WTERMSIG(child_status)) {
 		case SIGINT:
 			pr_op_err("RSYNC was user-interrupted. Guess I'll interrupt myself too.");
@@ -513,11 +488,9 @@ do_rsync(struct rpki_uri *uri, bool is_ta, bool log_operation)
 		return -EINTR; /* Meh? */
 	}
 
-	TA_DEBUG_MSG("%d", WIFSIGNALED(child_status));
 	pr_op_err("The RSYNC command died in a way I don't have a handler for. Dunno; guess I'll die as well.");
 	return -EINVAL;
 release_args:
-	TA_DEBUG_MSG("%d", error);
 	/* The happy path also falls here */
 	release_args(args, args_len);
 	return error;
@@ -596,79 +569,51 @@ rsync_download_files(struct rpki_uri *requested_uri, bool is_ta, bool force)
 	bool to_op_log;
 	int error;
 
-	if (!config_get_rsync_enabled()) {
-		TA_DEBUG;
+	if (!config_get_rsync_enabled())
 		return 0;
-	}
-	TA_DEBUG;
 
 	state = state_retrieve();
-	if (state == NULL) {
-		TA_DEBUG;
+	if (state == NULL)
 		return -EINVAL;
-	}
-	TA_DEBUG;
 
 	visited_uris = validation_rsync_visited_uris(state);
 
-	TA_DEBUG;
-
 	if (!force && is_already_downloaded(requested_uri, visited_uris)) {
 		pr_val_debug("No need to redownload '%s'.",
 		    uri_val_get_printable(requested_uri));
-		TA_DEBUG;
 		return check_ancestor_error(requested_uri);
 	}
 
 	if (!force) {
-		TA_DEBUG;
 		error = get_rsync_uri(requested_uri, is_ta, &rsync_uri);
-		TA_DEBUG_MSG("%d", error);
 	} else {
-		TA_DEBUG;
 		error = check_ancestor_error(requested_uri);
 		if (error) {
-			TA_DEBUG;
 			return error;
 		}
-		TA_DEBUG;
 		error = handle_strict_strategy(requested_uri, &rsync_uri);
-		TA_DEBUG_MSG("%d", error);
 	}
 
-	if (error) {
-		TA_DEBUG;
+	if (error)
 		return error;
-	}
 
 	pr_val_debug("Going to RSYNC '%s'.", uri_val_get_printable(rsync_uri));
-	TA_DEBUG;
 
 	to_op_log = reqs_errors_log_uri(uri_get_global(rsync_uri));
-	TA_DEBUG_MSG("%d", to_op_log);
 	error = do_rsync(rsync_uri, is_ta, to_op_log);
-	TA_DEBUG_MSG("%d", error);
 	switch(error) {
 	case 0:
 		/* Don't store when "force" and if its already downloaded */
-		if (!(force && is_already_downloaded(rsync_uri, visited_uris))) {
-			TA_DEBUG;
+		if (!(force && is_already_downloaded(rsync_uri, visited_uris)))
 			error = mark_as_downloaded(rsync_uri, visited_uris);
-			TA_DEBUG_MSG("%d", error);
-		}
 		reqs_errors_rem_uri(uri_get_global(rsync_uri));
-		TA_DEBUG;
 		break;
 	case EREQFAILED:
 		/* All attempts failed, avoid future requests */
 		error = reqs_errors_add_uri(uri_get_global(rsync_uri));
-		if (error) {
-			TA_DEBUG_MSG("%d", error);
+		if (error)
 			break;
-		}
-		TA_DEBUG;
 		error = mark_as_downloaded(rsync_uri, visited_uris);
-		TA_DEBUG_MSG("%d", error);
 		/* Everything went ok? Return the original error */
 		if (!error)
 			error = EREQFAILED;
@@ -676,7 +621,6 @@ rsync_download_files(struct rpki_uri *requested_uri, bool is_ta, bool force)
 	}
 
 	uri_refput(rsync_uri);
-	TA_DEBUG;
 	return error;
 }