openrc-run: fix rc_parallel race in svc_exec

svc_exec waits until SIGCHLD comes in to close its input, but in rc_parallel case the SIGCHLD might be unrelated. Checking the proper pid is found in signal handler and only signaling signal_pipe the status code directly avoids this problem.
openrc-run: silence lock failures with --no-deps
2023-04-24 22:06:53 -05:00 · 2023-04-24 22:06:41 -05:00 · 2023-04-24 22:06:41 -05:00 · 2023-04-24 19:20:19 -05:00 · 2023-04-24 19:20:19 -05:00 · 2023-04-24 19:20:19 -05:00
7 changed files with 110 additions and 90 deletions
--- a/src/fstabinfo/fstabinfo.c
+++ b/src/fstabinfo/fstabinfo.c
@ -23,6 +23,7 @@
 #include <string.h>
 #include <unistd.h>
 #include <stdbool.h>
+#include <spawn.h>

 /* Yay for linux and its non liking of POSIX functions.
   Okay, we could use getfsent but the man page says use getmntent instead
@ -63,6 +64,8 @@
 #include "_usage.h"
 #include "helpers.h"

+extern char **environ;
+
 const char *applet = NULL;
 const char *extraopts = NULL;
 const char getoptstring[] = "MRbmop:t:" getoptstring_COMMON;
@ -112,7 +115,7 @@ do_mount(struct ENT *ent, bool remount)
 {
 	char *argv[10];
 	pid_t pid;
-	int status;
+	int status, err;

 	argv[0] = UNCONST("mount");
 	argv[1] = UNCONST("-o");
@ -137,23 +140,14 @@ do_mount(struct ENT *ent, bool remount)
 		argv[8] = NULL;
 #endif
 	}
-	switch (pid = vfork()) {
-	case -1:
-		eerrorx("%s: vfork: %s", applet, strerror(errno));
-		/* NOTREACHED */
-	case 0:
-		execvp(argv[0], argv);
-		eerror("%s: execvp: %s", applet, strerror(errno));
-		_exit(EXIT_FAILURE);
-		/* NOTREACHED */
-	default:
-		waitpid(pid, &status, 0);
+	err = posix_spawnp(&pid, argv[0], NULL, NULL, argv, environ);
+	if (err)
+		eerrorx("%s: posix_spawnp: %s", applet, strerror(err));
+	while (waitpid(pid, &status, 0) < 0 && errno == EINTR);
 	if (WIFEXITED(status))
 		return WEXITSTATUS(status);
 	else
 		return -1;
-		/* NOTREACHED */
-	}
 }

 #define OUTPUT_FILE      (1 << 1)
--- a/src/libeinfo/einfo.h
+++ b/src/libeinfo/einfo.h
@ -69,22 +69,22 @@ void elog(int, const char * EINFO_RESTRICT, ...) EINFO_PRINTF(2, 3);
 * The x suffix means function will exit() returning failure.
 */
 /*@{*/
-int einfon(const char * __EINFO_RESTRICT, ...) EINFO_PRINTF(1, 2);
-int ewarnn(const char * __EINFO_RESTRICT, ...) EINFO_PRINTF(1, 2);
-int eerrorn(const char * __EINFO_RESTRICT, ...) EINFO_PRINTF(1, 2);
-int einfo(const char * __EINFO_RESTRICT, ...) EINFO_PRINTF(1, 2);
-int ewarn(const char * __EINFO_RESTRICT, ...) EINFO_PRINTF(1, 2);
-void ewarnx(const char * __EINFO_RESTRICT, ...) EINFO_XPRINTF(1, 2);
-int eerror(const char * __EINFO_RESTRICT, ...) EINFO_PRINTF(1, 2);
-void eerrorx(const char * __EINFO_RESTRICT, ...) EINFO_XPRINTF(1, 2);
+int einfon(const char * EINFO_RESTRICT, ...) EINFO_PRINTF(1, 2);
+int ewarnn(const char * EINFO_RESTRICT, ...) EINFO_PRINTF(1, 2);
+int eerrorn(const char * EINFO_RESTRICT, ...) EINFO_PRINTF(1, 2);
+int einfo(const char * EINFO_RESTRICT, ...) EINFO_PRINTF(1, 2);
+int ewarn(const char * EINFO_RESTRICT, ...) EINFO_PRINTF(1, 2);
+void ewarnx(const char * EINFO_RESTRICT, ...) EINFO_XPRINTF(1, 2);
+int eerror(const char * EINFO_RESTRICT, ...) EINFO_PRINTF(1, 2);
+void eerrorx(const char * EINFO_RESTRICT, ...) EINFO_XPRINTF(1, 2);

-int einfovn(const char * __EINFO_RESTRICT, ...) EINFO_PRINTF(1, 2);
-int ewarnvn(const char * __EINFO_RESTRICT, ...) EINFO_PRINTF(1, 2);
-int ebeginvn(const char * __EINFO_RESTRICT, ...) EINFO_PRINTF(1, 2);
-int eendvn(int, const char * __EINFO_RESTRICT, ...) EINFO_PRINTF(2, 3);
-int ewendvn(int, const char * __EINFO_RESTRICT, ...) EINFO_PRINTF(2, 3);
-int einfov(const char * __EINFO_RESTRICT, ...) EINFO_PRINTF(1, 2);
-int ewarnv(const char * __EINFO_RESTRICT, ...) EINFO_PRINTF(1, 2);
+int einfovn(const char * EINFO_RESTRICT, ...) EINFO_PRINTF(1, 2);
+int ewarnvn(const char * EINFO_RESTRICT, ...) EINFO_PRINTF(1, 2);
+int ebeginvn(const char * EINFO_RESTRICT, ...) EINFO_PRINTF(1, 2);
+int eendvn(int, const char * EINFO_RESTRICT, ...) EINFO_PRINTF(2, 3);
+int ewendvn(int, const char * EINFO_RESTRICT, ...) EINFO_PRINTF(2, 3);
+int einfov(const char * EINFO_RESTRICT, ...) EINFO_PRINTF(1, 2);
+int ewarnv(const char * EINFO_RESTRICT, ...) EINFO_PRINTF(1, 2);
 /*@}*/

 /*! @ingroup ebegin
--- a/src/openrc-run/openrc-run.c
+++ b/src/openrc-run/openrc-run.c
@ -107,7 +107,8 @@ static RC_STRINGLIST *deptypes_mwua;	/* need+want+use+after deps for stopping */
 static void
 handle_signal(int sig)
 {
-	int serrno = errno;
+	int serrno = errno, status;
+	pid_t pid;
 	const char *signame = NULL;
 	struct winsize ws;

@ -117,12 +118,13 @@ handle_signal(int sig)
 		break;

 	case SIGCHLD:
-		if (signal_pipe[1] > -1) {
-			if (write(signal_pipe[1], &sig, sizeof(sig)) == -1)
+		while ((pid = waitpid(-1, &status, WNOHANG)) > 0) {
+			if (signal_pipe[1] > -1 && pid == service_pid) {
+				if (write(signal_pipe[1], &status, sizeof(status)) == -1)
 					eerror("%s: send: %s",
 					    service, strerror(errno));
-		} else
-			rc_waitpid(-1);
+			}
+		}
 		break;

 	case SIGWINCH:
@ -438,6 +440,7 @@ svc_exec(const char *arg1, const char *arg2)
 			if (errno != EINTR) {
 				eerror("%s: poll: %s",
 				    service, strerror(errno));
+				ret = -1;
 				break;
 			}
 		}
@ -448,10 +451,21 @@ svc_exec(const char *arg1, const char *arg2)
 				write_prefix(buffer, bytes, &prefixed);
 			}

-			/* Only SIGCHLD signals come down this pipe */
-			if (fd[0].revents & (POLLIN | POLLHUP))
+			/* signal_pipe receives service_pid's exit status */
+			if (fd[0].revents & (POLLIN | POLLHUP)) {
+				if ((s = read(signal_pipe[0], &ret, sizeof(ret))) != sizeof(ret)) {
+					eerror("%s: receive failed: %s", service,
+						s < 0 ? strerror(errno) : "short read");
+					ret = -1;
 					break;
 				}
+				ret = WEXITSTATUS(ret);
+				if (ret != 0 && errno == ECHILD)
+					/* killall5 -9 could cause this */
+					ret = 0;
+				break;
+			}
+		}
 	}

 	free(buffer);
@ -473,11 +487,6 @@ svc_exec(const char *arg1, const char *arg2)
 		master_tty = -1;
 	}

-	ret = rc_waitpid(service_pid);
-	ret = WEXITSTATUS(ret);
-	if (ret != 0 && errno == ECHILD)
-		/* killall5 -9 could cause this */
-		ret = 0;
 	service_pid = 0;

 	return ret;
@ -612,7 +621,7 @@ svc_start_check(void)
 	}

 	if (exclusive_fd == -1)
-		exclusive_fd = svc_lock(applet);
+		exclusive_fd = svc_lock(applet, !deps);
 	if (exclusive_fd == -1) {
 		if (errno == EACCES)
 			eerrorx("%s: superuser access required", applet);
@ -864,7 +873,7 @@ svc_stop_check(RC_SERVICE *state)
 		exit(EXIT_FAILURE);

 	if (exclusive_fd == -1)
-		exclusive_fd = svc_lock(applet);
+		exclusive_fd = svc_lock(applet, !deps);
 	if (exclusive_fd == -1) {
 		if (errno == EACCES)
 			eerrorx("%s: superuser access required", applet);
@ -1041,26 +1050,6 @@ svc_stop(void)
 static void
 svc_restart(void)
 {
-	/* This is hairy and a better way needs to be found I think!
-	 * The issue is this - openvpn need net and dns. net can restart
-	 * dns via resolvconf, so you could have openvpn trying to restart
-	 * dnsmasq which in turn is waiting on net which in turn is waiting
-	 * on dnsmasq.
-	 * The work around is for resolvconf to restart its services with
-	 * --nodeps which means just that.
-	 * The downside is that there is a small window when our status is
-	 * invalid.
-	 * One workaround would be to introduce a new status,
-	 * or status locking. */
-	if (!deps) {
-		RC_SERVICE state = rc_service_state(service);
-		if (state & RC_SERVICE_STARTED || state & RC_SERVICE_INACTIVE)
-			svc_exec("stop", "start");
-		else
-			svc_exec("start", NULL);
-		return;
-	}
-
 	if (!(rc_service_state(service) & RC_SERVICE_STOPPED)) {
 		get_started_services();
 		svc_stop();
--- a/src/openrc/rc.c
+++ b/src/openrc/rc.c
@ -87,6 +87,7 @@ static RC_HOOK hook_out;
 struct termios *termios_orig = NULL;

 RC_PIDLIST service_pids;
+RC_PIDLIST free_these_pids;

 static void
 clean_failed(void)
@ -116,8 +117,7 @@ clean_failed(void)
 static void
 cleanup(void)
 {
-	RC_PID *p1 = LIST_FIRST(&service_pids);
-	RC_PID *p2;
+	RC_PID *p, *tmp;

 	if (!rc_in_logger && !rc_in_plugin &&
 	    applet && (strcmp(applet, "rc") == 0 || strcmp(applet, "openrc") == 0))
@ -139,10 +139,13 @@ cleanup(void)
 		rc_logger_close();
 	}

-	while (p1) {
-		p2 = LIST_NEXT(p1, entries);
-		free(p1);
-		p1 = p2;
+	LIST_FOREACH_SAFE(p, &service_pids, entries, tmp) {
+		LIST_REMOVE(p, entries);
+		free(p);
+	}
+	LIST_FOREACH_SAFE(p, &free_these_pids, entries, tmp) {
+		LIST_REMOVE(p, entries);
+		free(p);
 	}

 	rc_stringlist_free(main_hotplugged_services);
@ -344,23 +347,47 @@ static char *get_krunlevel(void)
 static void
 add_pid(pid_t pid)
 {
+	sigset_t sset, old;
 	RC_PID *p = xmalloc(sizeof(*p));
 	p->pid = pid;
+
+	/* this list will be accessed inside the SIGCHLD signal handler.
+	 * so we need to ensure that the SIGCHLD handler doesn't get invoked
+	 * while the list is at an inconsistent state.
+	 */
+	sigemptyset(&sset);
+	sigaddset(&sset, SIGCHLD);
+	sigprocmask(SIG_SETMASK, &sset, &old);
 	LIST_INSERT_HEAD(&service_pids, p, entries);
+	sigprocmask(SIG_SETMASK, &old, NULL);
 }

 static void
-remove_pid(pid_t pid)
+remove_pid(pid_t pid, bool inside_signal)
 {
-	RC_PID *p;
+	sigset_t sset, old;
+	RC_PID *p, *tmp;

-	LIST_FOREACH(p, &service_pids, entries)
+	/* same rationale for blocking SIGCHLD as add_pid() */
+	sigemptyset(&sset);
+	sigaddset(&sset, SIGCHLD);
+	sigprocmask(SIG_SETMASK, &sset, &old);
+	LIST_FOREACH(p, &service_pids, entries) {
 		if (p->pid == pid) {
 			LIST_REMOVE(p, entries);
-		    free(p);
-		    return;
+			LIST_INSERT_HEAD(&free_these_pids, p, entries);
+			break;
 		}
 	}
+	/* only call free if we're not inside a signal handler */
+	if (!inside_signal) {
+		LIST_FOREACH_SAFE(p, &free_these_pids, entries, tmp) {
+			LIST_REMOVE(p, entries);
+			free(p);
+		}
+	}
+	sigprocmask(SIG_SETMASK, &old, NULL);
+}

 static void
 wait_for_services(void)
@ -397,7 +424,7 @@ handle_signal(int sig)

 		/* Remove that pid from our list */
 		if (pid > 0)
-			remove_pid(pid);
+			remove_pid(pid, true);
 		break;

 	case SIGWINCH:
@ -606,7 +633,7 @@ stop:
 			add_pid(pid);
 			if (!parallel) {
 				rc_waitpid(pid);
-				remove_pid(pid);
+				remove_pid(pid, false);
 			}
 		}
 	}
@ -672,7 +699,7 @@ do_start_services(const RC_STRINGLIST *start_services, bool parallel)
 			add_pid(pid);
 			if (!parallel) {
 				rc_waitpid(pid);
-				remove_pid(pid);
+				remove_pid(pid, false);
 			}
 		}
 	}
@ -745,6 +772,7 @@ int main(int argc, char **argv)

 	applet = basename_c(argv[0]);
 	LIST_INIT(&service_pids);
+	LIST_INIT(&free_these_pids);
 	atexit(cleanup);
 	if (!applet)
 		eerrorx("arguments required");
--- a/src/shared/helpers.h
+++ b/src/shared/helpers.h
@ -21,6 +21,7 @@
 #include <stdarg.h>
 #include <stdbool.h>
 #include <stddef.h>
+#include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@ -28,7 +29,7 @@

 #define ERRX fprintf (stderr, "out of memory\n"); exit (1)

-#define UNCONST(a)		((void *)(unsigned long)(const void *)(a))
+#define UNCONST(a)		((void *)(uintptr_t)(const void *)(a))

 #define RC_UNUSED __attribute__((__unused__))
 #define RC_NORETURN __attribute__((__noreturn__))
--- a/src/shared/misc.c
+++ b/src/shared/misc.c
@ -234,7 +234,7 @@ signal_setup_restart(int sig, void (*handler)(int))
 }

 int
-svc_lock(const char *applet)
+svc_lock(const char *applet, bool ignore_lock_failure)
 {
 	char *file = NULL;
 	int fd;
@ -245,6 +245,14 @@ svc_lock(const char *applet)
 	if (fd == -1)
 		return -1;
 	if (flock(fd, LOCK_EX | LOCK_NB) == -1) {
+		if (ignore_lock_failure) {
+			/* Two services with a need b, and b's start()
+			 * calling restart --no-deps on a would cause
+			 * harmless errors: just ignore them.
+			 * See https://github.com/OpenRC/openrc/issues/224
+			 */
+			exit(EXIT_SUCCESS);
+		}
 		eerror("Call to flock failed: %s", strerror(errno));
 		close(fd);
 		return -1;
@ -274,7 +282,7 @@ exec_service(const char *service, const char *arg)
 	sigset_t old;
 	struct sigaction sa;

-	fd = svc_lock(basename_c(service));
+	fd = svc_lock(basename_c(service), false);
 	if (fd == -1)
 		return -1;

--- a/src/shared/misc.h
+++ b/src/shared/misc.h
@ -50,7 +50,7 @@ void env_filter(void);
 void env_config(void);
 int signal_setup(int sig, void (*handler)(int));
 int signal_setup_restart(int sig, void (*handler)(int));
-int svc_lock(const char *);
+int svc_lock(const char *, bool);
 int svc_unlock(const char *, int);
 pid_t exec_service(const char *, const char *);
Author	SHA1	Message	Date
Dominique Martinet	863e1a5c87	openrc-run: fix rc_parallel race in svc_exec svc_exec waits until SIGCHLD comes in to close its input, but in rc_parallel case the SIGCHLD might be unrelated. Checking the proper pid is found in signal handler and only signaling signal_pipe the status code directly avoids this problem.	2023-04-24 22:06:53 -05:00
Dominique Martinet	0b9c3c0803	openrc-run: silence lock failures with --no-deps work around scary warnings described in previous commit	2023-04-24 22:06:41 -05:00
Dominique Martinet	a3c721b682	openrc-run: remove kludge in restart --no-deps restarting a service with --no-deps ran into a "hairy workaround", which had a few problems discussed in [1]: - it ignores --dry-run, really restarting the requested service - if the service was stopped, the program is started but the service status stays stopped. This makes long-lived services impossible to (re)start again (pid already exists and running), and the service also won't stop on shutdown. The kludge had a long comment describing the following situation: - openvpn needs net and dns - net restarts dns - dns needs net If the restart in net handled deps, openrc would deadlock waiting for net in dns' restart, as net won't be started until that is done. Restarting with --nodeps works around the deadlock, but can display errors without the kludge (note that the services did start properly anyway, the problem is that the default service path tries to lock dns twice from openvn's dep's start and net's start's restart): --- alpine:~# rc-service openvn start openvn \| * Caching service dependencies ... [ ok ] net \|net starting net \|dns \| * Call to flock failed: Resource temporarily unavailable net \|dns \| * ERROR: dns stopped by something else net \|net started dns \|dns started openvn \|openvn started alpine:~# rc-status \| grep s[1-3] net [ started ] dns [ started ] openvn [ started ] --- Locking again in restart --nodep can fail in two patterns: - openvpn's need dependency start was first, and the restart in net failed (case above): we can just silence locking failures and exit quietly with restart --no-deps, which is not worse than trying to restart while another process hold the lock. - the restart in net's start was first, and openvpn's need dependency start failed: not much can be done here short of adding a new status that a no-deps restart is in progress as in the comment, but this case can actually just be solved by adjusting dependencies -- and it actually has already been fixed: the current openvpn init script in alpine only 'use dns', so it will not try to start it, and that will start just fine with openvpn -> net -> dns only each starting each other once sequentially. Another failure pattern is just starting dns directly: that will start net, which will try to restart dns while we are starting it. Silencing messages on restart also solves this. Link: https://github.com/OpenRC/openrc/issues/224 [1]	2023-04-24 22:06:41 -05:00
NRK	3f82d5b1a3	rc: use LIST_FOREACH_SAFE in cleanup() according to the linux manpage, the "safe" variant may not be available on all platform. however we bundle our own `queue.h` so this should not be an issue.	2023-04-24 19:20:19 -05:00
NRK	0b4732520f	rc: block SIGCHLD during pid list operations the pid list will be accessed inside the SIGCHLD signal handler. so we must ensure SIGCHLD handler doesn't get invoked while the list is at an inconsistent state making it unsafe to interact with. Co-authored-by: Dominique MARTINET <dominique.martinet@atmark-techno.com> Bug: https://github.com/OpenRC/openrc/issues/589#issuecomment-1406588576	2023-04-24 19:20:19 -05:00
NRK	bbd3acfc67	rc: avoid calling free inside SIGCHLD handler `free` is not async-signal-safe and calling it inside a signal handler can have bad effects, as reported in the musl ML: https://www.openwall.com/lists/musl/2023/01/23/1 the solution: - keep track of weather remove_pid() is being called from inside a signal handler or not. - if it's inside a signal handler then DO NOT call free - instead put that pointer into a "to be freed later" list. - if it's not inside a signal handler then take the "to be freed later" list and free anything in it. Bug: https://github.com/OpenRC/openrc/issues/589 Reported-by: Dominique MARTINET <dominique.martinet@atmark-techno.com>	2023-04-24 19:20:19 -05:00
NRK	910e3e2a0e	fstabinfo: deal with EINTR in waitpid call	2023-04-24 19:18:18 -05:00
NRK	5f04dcc951	fstabinfo: replace vfork with posix_spawnp problem: * vfork has been removed from POSIX [0]. * clang-tidy flags the `strerror` and `eerror` call inside the vfork-ed child as undefined behavior. solution: use posix_spawnp, which is serves similar purpose and is specified in posix. and as an added bonus, it's also easier to use and less lines of code. [0]: https://www.man7.org/linux/man-pages/man2/vfork.2.html#CONFORMING_TO	2023-04-24 19:18:18 -05:00
Oskari Pirhonen	8f52c64c37	einfo.h: fix EINFO_RESTRICT macro usage Make function declarations use the EINFO_RESTRICT macro instead of __EINFO_RESTRICT which gets treated as the name of the argument.	2023-04-23 16:11:15 -04:00
Sam James	09d405fb3e	shared: fix pointer type in UNCONST Thanks to vapier for noticing.	2023-04-20 07:50:49 -04:00