From 47f895d4cda01df2eaadd24d709e731c037beaf1 Mon Sep 17 00:00:00 2001 From: "Anna (navi) Figueiredo Gomes" Date: Sun, 6 Apr 2025 15:17:26 +0200 Subject: [PATCH 1/7] shared/misc: close lockfd in parent of exec_service it makes no sense for the parent to hold the lock, even with cloexec, and that makes attempts to wait on flock useless. it only worked so far due to busy-polling with open, so basically, we were waiting for an unlink, not a lockfile. --- src/shared/misc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/misc.c b/src/shared/misc.c index be8cc5f33..0398748bf 100644 --- a/src/shared/misc.c +++ b/src/shared/misc.c @@ -367,7 +367,7 @@ exec_service(const char *service, const char *arg) fprintf(stderr, "fork: %s\n",strerror (errno)); svc_unlock(basename_c(service), fd); } else - fcntl(fd, F_SETFD, fcntl(fd, F_GETFD, 0) | FD_CLOEXEC); + close(fd); sigprocmask(SIG_SETMASK, &old, NULL); free(file); From 8c4c63ded6e786aea03b746e05674fe327cf3fb0 Mon Sep 17 00:00:00 2001 From: "Anna (navi) Figueiredo Gomes" Date: Sun, 6 Apr 2025 15:19:46 +0200 Subject: [PATCH 2/7] shared/misc: remove semicolon from service_{start,stop} macros --- src/shared/misc.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/shared/misc.h b/src/shared/misc.h index e5cf01606..bee987a41 100644 --- a/src/shared/misc.h +++ b/src/shared/misc.h @@ -73,8 +73,8 @@ static const rc_service_state_name_t rc_service_state_names[] = { */ int is_writable(const char *); -#define service_start(service) exec_service(service, "start"); -#define service_stop(service) exec_service(service, "stop"); +#define service_start(service) exec_service(service, "start") +#define service_stop(service) exec_service(service, "stop") int parse_mode(mode_t *, char *); From fec76ad7a7f953b887533ef35dd6825e0de16660 Mon Sep 17 00:00:00 2001 From: "Anna (navi) Figueiredo Gomes" Date: Sun, 6 Apr 2025 18:53:07 +0200 Subject: [PATCH 3/7] openrc-run: mark signal bools as volatile --- src/openrc-run/openrc-run.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/openrc-run/openrc-run.c b/src/openrc-run/openrc-run.c index 9a7f55f3a..3bdd70a58 100644 --- a/src/openrc-run/openrc-run.c +++ b/src/openrc-run/openrc-run.c @@ -93,7 +93,8 @@ static RC_STRINGLIST *use_services; static RC_STRINGLIST *want_services; static RC_HOOK hook_out; static int exclusive_fd = -1, master_tty = -1; -static bool sighup, skip_mark, in_background, deps, dry_run; +static bool in_background, deps, dry_run; +static volatile bool sighup, skip_mark; static pid_t service_pid; static int signal_pipe[2] = { -1, -1 }; From 76a4e43bb86c090d8ae29a69da9bec1d37dca330 Mon Sep 17 00:00:00 2001 From: "Anna (navi) Figueiredo Gomes" Date: Sun, 6 Apr 2025 16:35:31 +0200 Subject: [PATCH 4/7] openrc-run: properly wait for services without busy polling and use alarm.2 for timing out, this means that instead of waking up every 20ms and doing a few syscalls to check the lockfile, we wake up only every 10s for warning about the timeout --- src/openrc-run/openrc-run.c | 72 +++++++++++++++++-------------------- 1 file changed, 33 insertions(+), 39 deletions(-) diff --git a/src/openrc-run/openrc-run.c b/src/openrc-run/openrc-run.c index 3bdd70a58..db861946c 100644 --- a/src/openrc-run/openrc-run.c +++ b/src/openrc-run/openrc-run.c @@ -94,7 +94,7 @@ static RC_STRINGLIST *want_services; static RC_HOOK hook_out; static int exclusive_fd = -1, master_tty = -1; static bool in_background, deps, dry_run; -static volatile bool sighup, skip_mark; +static volatile bool sighup, skip_mark, timedout; static pid_t service_pid; static int signal_pipe[2] = { -1, -1 }; @@ -157,6 +157,10 @@ handle_signal(int sig) exit(EXIT_FAILURE); /* NOTREACHED */ + case SIGALRM: + timedout = true; + break; + default: eerror("%s: caught unknown signal %d", applet, sig); } @@ -500,55 +504,44 @@ svc_wait(const char *svc) int fd; bool forever = false; RC_STRINGLIST *keywords; - struct timespec timeout, warn; const char *base = basename_c(svc); + int timeout = WAIT_TIMEOUT; + bool retval = false; /* Some services don't have a timeout, like fsck */ keywords = rc_deptree_depend(deptree, svc, "keyword"); - if (rc_stringlist_find(keywords, "-timeout") || - rc_stringlist_find(keywords, "notimeout")) + if (rc_stringlist_find(keywords, "-timeout") || rc_stringlist_find(keywords, "notimeout")) forever = true; rc_stringlist_free(keywords); - timeout.tv_sec = WAIT_TIMEOUT; - timeout.tv_nsec = 0; - warn.tv_sec = WARN_TIMEOUT; - warn.tv_nsec = 0; + if ((fd = openat(rc_dirfd(RC_DIR_EXCLUSIVE), base, O_RDONLY | O_NONBLOCK)) == -1) + return errno == ENOENT; + + timedout = false; for (;;) { - fd = openat(rc_dirfd(RC_DIR_EXCLUSIVE), base, O_RDONLY | O_NONBLOCK); - if (fd != -1) { - if (flock(fd, LOCK_SH | LOCK_NB) == 0) { - close(fd); - return true; - } - close(fd); - } - if (errno == ENOENT) { - return true; - } - if (errno != EWOULDBLOCK) { - eerror("%s: open '%s/exclusive/%s': %s", applet, rc_svcdir(), base, strerror(errno)); - exit(EXIT_FAILURE); - } - if (nanosleep(&interval, NULL) == -1) { - if (errno != EINTR) - goto finish; + if (!forever) + alarm(WARN_TIMEOUT); + + if (flock(fd, LOCK_SH) == 0) { + retval = true; + break; } - if (!forever) { - timespecsub(&timeout, &interval, &timeout); - if (timeout.tv_sec <= 0) - goto finish; - timespecsub(&warn, &interval, &warn); - if (warn.tv_sec <= 0) { - ewarn("%s: waiting for %s (%d seconds)", - applet, svc, (int)timeout.tv_sec); - warn.tv_sec = WARN_TIMEOUT; - warn.tv_nsec = 0; - } + + if (errno != EINTR) + break; + + if (!forever && timedout) { + timedout = false; + if ((timeout -= WARN_TIMEOUT) > 0) + ewarn("%s: waiting for %s (%d seconds)", applet, base, timeout); + else + break; } } -finish: - return false; + + alarm(0); + close(fd); + return retval; } static void @@ -1237,6 +1230,7 @@ int main(int argc, char **argv) /* Setup a signal handler */ signal_setup(SIGHUP, handle_signal); + signal_setup(SIGALRM, handle_signal); signal_setup(SIGUSR1, handle_signal); signal_setup(SIGINT, handle_signal); signal_setup(SIGQUIT, handle_signal); From cf03ce4b8990cba088bc78d39894d72e7443591d Mon Sep 17 00:00:00 2001 From: "Anna (navi) Figueiredo Gomes" Date: Sun, 6 Apr 2025 16:52:33 +0200 Subject: [PATCH 5/7] openrc-run: avoid busy polling in svc_exec --- src/openrc-run/meson.build | 2 +- src/openrc-run/openrc-run.c | 75 +++++++++++++++++++++---------------- 2 files changed, 43 insertions(+), 34 deletions(-) diff --git a/src/openrc-run/meson.build b/src/openrc-run/meson.build index 29ab69ec0..2acdf7f9a 100644 --- a/src/openrc-run/meson.build +++ b/src/openrc-run/meson.build @@ -1,5 +1,5 @@ executable('openrc-run', - ['openrc-run.c', misc_c, plugin_c, rc_exec_c, selinux_c, usage_c, version_h], + ['openrc-run.c', misc_c, timeutils_c, plugin_c, rc_exec_c, selinux_c, usage_c, version_h], c_args : [cc_audit_flags, cc_branding_flags, cc_pam_flags, cc_selinux_flags], link_with: [libeinfo, librc], dependencies: [audit_dep, dl_dep, pam_dep, pam_misc_dep, selinux_dep, util_dep, crypt_dep], diff --git a/src/openrc-run/openrc-run.c b/src/openrc-run/openrc-run.c index db861946c..ddf2083ad 100644 --- a/src/openrc-run/openrc-run.c +++ b/src/openrc-run/openrc-run.c @@ -35,6 +35,7 @@ #include #include #include +#include #if defined(__linux__) || (defined(__FreeBSD_kernel__) && defined(__GLIBC__)) \ || defined(__GNU__) @@ -50,17 +51,15 @@ #include "rc.h" #include "rc_exec.h" #include "misc.h" +#include "timeutils.h" #include "plugin.h" #include "selinux.h" #include "_usage.h" #include "helpers.h" -#define WAIT_INTERVAL 20000000 /* usecs to poll the lock file */ #define WAIT_TIMEOUT 60 /* seconds until we timeout */ #define WARN_TIMEOUT 10 /* warn about this every N seconds */ -static const struct timespec interval = { .tv_nsec = WAIT_INTERVAL }; - const char *applet = NULL; const char *extraopts = "stop | start | restart | status | describe | zap"; const char getoptstring[] = "dDsSvl:Z" getoptstring_COMMON; @@ -364,8 +363,7 @@ svc_exec(const char *arg1, const char *arg2) int slave_tty; sigset_t sigchldmask; sigset_t oldmask; - struct timespec timeout = { .tv_sec = WAIT_TIMEOUT }; - struct timespec warn = { .tv_sec = WARN_TIMEOUT }; + int64_t wait_timeout, warn_timeout, now; RC_STRINGLIST *keywords; bool forever; @@ -401,6 +399,9 @@ svc_exec(const char *arg1, const char *arg2) fcntl(slave_tty, F_SETFD, flags | FD_CLOEXEC); } + now = tm_now(); + wait_timeout = now + (WAIT_TIMEOUT * 1000); + warn_timeout = now + (WARN_TIMEOUT * 1000); service_pid = fork(); if (service_pid == -1) eerrorx("%s: fork: %s", applet, strerror(errno)); @@ -432,45 +433,53 @@ svc_exec(const char *arg1, const char *arg2) } for (;;) { - if ((s = poll(fd, master_tty >= 0 ? 2 : 1, WAIT_INTERVAL / 1000000)) == -1) { + int timeout; + if (forever) { + timeout = -1; + } else if ((timeout = warn_timeout - tm_now()) < 0) { + timeout = 0; + } + + if (poll(fd, master_tty >= 0 ? 2 : 1, timeout) == -1) { if (errno != EINTR) { eerror("%s: poll: %s", applet, strerror(errno)); ret = -1; break; } + continue; } - if (s == 0 && !forever) { - timespecsub(&timeout, &interval, &timeout); - if (timeout.tv_sec <= 0) { - kill(service_pid, SIGKILL); + if (fd[1].revents & (POLLIN | POLLHUP)) { + bytes = read(master_tty, buffer, BUFSIZ); + write_prefix(buffer, bytes, &prefixed); + } + + /* 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", applet, s < 0 ? strerror(errno) : "short read"); ret = -1; break; } - timespecsub(&warn, &interval, &warn); - if (warn.tv_sec <= 0) { - ewarn("%s: waiting for service (%d seconds)", applet, (int)timeout.tv_sec); - warn = (struct timespec) { .tv_sec = WARN_TIMEOUT }; - } - } else if (s > 0) { - if (fd[1].revents & (POLLIN | POLLHUP)) { - bytes = read(master_tty, buffer, BUFSIZ); - write_prefix(buffer, bytes, &prefixed); - } + ret = WEXITSTATUS(ret); + if (ret != 0 && errno == ECHILD) + /* killall5 -9 could cause this */ + ret = 0; + break; + } - /* 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", applet, 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; - } + if (forever) + continue; + + if ((now = tm_now()) >= wait_timeout) { + kill(service_pid, SIGKILL); + ret = -1; + break; + } else if (now >= warn_timeout) { + /* the timer might get off by a few ms, add 500ms so we get round numbers matching svc_wait. */ + ewarn("%s: waiting for service (%"PRId64" seconds)", applet, (wait_timeout - now + 500) / 1000); + if ((warn_timeout += (WARN_TIMEOUT * 1000)) > wait_timeout) + warn_timeout = wait_timeout; } } From b07e698f8e618b59210632ae4ce84f4a96546a5c Mon Sep 17 00:00:00 2001 From: "Anna (navi) Figueiredo Gomes" Date: Sun, 6 Apr 2025 18:46:01 +0200 Subject: [PATCH 6/7] openrc-run: remove unnecessary branch. --- src/openrc-run/openrc-run.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/openrc-run/openrc-run.c b/src/openrc-run/openrc-run.c index ddf2083ad..a8ec0dd0f 100644 --- a/src/openrc-run/openrc-run.c +++ b/src/openrc-run/openrc-run.c @@ -424,13 +424,9 @@ svc_exec(const char *arg1, const char *arg2) buffer = xmalloc(sizeof(char) * BUFSIZ); fd[0].fd = signal_pipe[0]; + fd[1].fd = master_tty; fd[0].events = fd[1].events = POLLIN; fd[0].revents = fd[1].revents = 0; - if (master_tty >= 0) { - fd[1].fd = master_tty; - fd[1].events = POLLIN; - fd[1].revents = 0; - } for (;;) { int timeout; From 7069e8ce9f731280753a1d0d910a326344e5716c Mon Sep 17 00:00:00 2001 From: "Anna (navi) Figueiredo Gomes" Date: Sun, 6 Apr 2025 18:47:07 +0200 Subject: [PATCH 7/7] openrc-run: use array instead of malloc(BUFSIZ) --- src/openrc-run/openrc-run.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/openrc-run/openrc-run.c b/src/openrc-run/openrc-run.c index a8ec0dd0f..73fb129a4 100644 --- a/src/openrc-run/openrc-run.c +++ b/src/openrc-run/openrc-run.c @@ -357,7 +357,7 @@ svc_exec(const char *arg1, const char *arg2) int flags = 0; struct pollfd fd[2]; int s; - char *buffer; + char buffer[BUFSIZ]; size_t bytes; bool prefixed = false; int slave_tty; @@ -422,7 +422,6 @@ svc_exec(const char *arg1, const char *arg2) /* UNREACHABLE */ } - buffer = xmalloc(sizeof(char) * BUFSIZ); fd[0].fd = signal_pipe[0]; fd[1].fd = master_tty; fd[0].events = fd[1].events = POLLIN; @@ -479,8 +478,6 @@ svc_exec(const char *arg1, const char *arg2) } } - free(buffer); - sigemptyset (&sigchldmask); sigaddset (&sigchldmask, SIGCHLD); sigprocmask (SIG_BLOCK, &sigchldmask, &oldmask);