The branch main has been updated by kevans:
URL:
https://cgit.FreeBSD.org/src/commit/?id=7e8afac0cb2d53595c06717fd4a50d6ec97a3f9d
commit 7e8afac0cb2d53595c06717fd4a50d6ec97a3f9d
Author: Kyle Evans <kev...@freebsd.org>
AuthorDate: 2025-06-24 22:18:27 +0000
Commit: Kyle Evans <kev...@freebsd.org>
CommitDate: 2025-07-10 17:54:19 +0000
lockf: switch to a SIGCHLD model for reaping child
A future change will add a -T flag to forward SIGTERM along to the
child before we cleanup and terminate ourselves. Using a SIGCHLD
handler to do that with SIGTERM blocked only while the child is actively
being collected will enable us to safely do so without having to worry
that our pid is potentially invalid.
Add a test that concisely checks that the child's error is properly
bubbled up to the caller.
Reviewed by: des, kib
Differential Revision: https://reviews.freebsd.org/D51024
---
usr.bin/lockf/lockf.c | 69 ++++++++++++++++++++++++++++++++++++---
usr.bin/lockf/tests/lockf_test.sh | 8 +++++
2 files changed, 72 insertions(+), 5 deletions(-)
diff --git a/usr.bin/lockf/lockf.c b/usr.bin/lockf/lockf.c
index 93164e30762c..19424418ed68 100644
--- a/usr.bin/lockf/lockf.c
+++ b/usr.bin/lockf/lockf.c
@@ -34,6 +34,7 @@
#include <fcntl.h>
#include <limits.h>
#include <signal.h>
+#include <stdatomic.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
@@ -50,14 +51,19 @@ union lock_subject {
static int acquire_lock(union lock_subject *subj, int flags, int silent);
static void cleanup(void);
static void killed(int sig);
+static void sigchld(int sig);
static void timeout(int sig);
static void usage(void) __dead2;
static void wait_for_lock(const char *name);
static const char *lockname;
+_Static_assert(sizeof(sig_atomic_t) >= sizeof(pid_t),
+ "PIDs cannot be managed safely from a signal handler on this platform.");
+static sig_atomic_t child = -1;
static int lockfd = -1;
static int keep;
static int fdlock;
+static int status;
static volatile sig_atomic_t timed_out;
/*
@@ -91,10 +97,14 @@ fdlock_implied(const char *name, long *ofd)
int
main(int argc, char **argv)
{
- int ch, flags, silent, status, writepid;
+ struct sigaction sa_chld = {
+ .sa_handler = sigchld,
+ .sa_flags = SA_NOCLDSTOP,
+ }, sa_prev;
+ sigset_t mask, omask;
long long waitsec;
- pid_t child;
union lock_subject subj;
+ int ch, flags, silent, writepid;
silent = keep = writepid = 0;
flags = O_CREAT | O_RDONLY;
@@ -238,9 +248,30 @@ main(int argc, char **argv)
if (atexit(cleanup) == -1)
err(EX_OSERR, "atexit failed");
+
+ /*
+ * Block SIGTERM while SIGCHLD is being processed, so that we can safely
+ * waitpid(2) for the child without a concurrent termination observing
+ * an invalid pid (i.e., waited-on). If our setup between here and the
+ * sigsuspend loop gets any more complicated, we should rewrite it to
+ * just use a pipe to signal the child onto execvp().
+ *
+ * We're blocking SIGCHLD and SIGTERM here so that we don't do any
+ * cleanup before we're ready to (after the pid is written out).
+ */
+ sigemptyset(&mask);
+ sigaddset(&mask, SIGCHLD);
+ sigaddset(&mask, SIGTERM);
+ (void)sigprocmask(SIG_BLOCK, &mask, &omask);
+
+ memcpy(&sa_chld.sa_mask, &omask, sizeof(omask));
+ sigaddset(&sa_chld.sa_mask, SIGTERM);
+ (void)sigaction(SIGCHLD, &sa_chld, &sa_prev);
+
if ((child = fork()) == -1)
err(EX_OSERR, "cannot fork");
if (child == 0) { /* The child process. */
+ (void)sigprocmask(SIG_SETMASK, &omask, NULL);
close(lockfd);
execvp(argv[0], argv);
warn("%s", argv[0]);
@@ -250,16 +281,24 @@ main(int argc, char **argv)
signal(SIGINT, SIG_IGN);
signal(SIGQUIT, SIG_IGN);
signal(SIGTERM, killed);
+
fclose(stdin);
fclose(stdout);
fclose(stderr);
/* Write out the pid before we sleep on it. */
if (writepid)
- (void)dprintf(lockfd, "%d\n", child);
+ (void)dprintf(lockfd, "%d\n", (int)child);
+
+ /* Just in case they were blocked on entry. */
+ sigdelset(&omask, SIGCHLD);
+ sigdelset(&omask, SIGTERM);
+ while (child >= 0) {
+ (void)sigsuspend(&omask);
+ /* child */
+ atomic_signal_fence(memory_order_acquire);
+ }
- if (waitpid(child, &status, 0) == -1)
- exit(EX_OSERR);
return (WIFEXITED(status) ? WEXITSTATUS(status) : EX_SOFTWARE);
}
@@ -323,6 +362,26 @@ killed(int sig)
_Exit(EX_OSERR);
}
+/*
+ * Signal handler for SIGCHLD. Simply waits for the child and ensures that we
+ * don't end up in a sticky situation if we receive a SIGTERM around the same
+ * time.
+ */
+static void
+sigchld(int sig __unused)
+{
+ int ostatus;
+
+ while (waitpid(child, &ostatus, 0) != child) {
+ if (errno != EINTR)
+ _exit(EX_OSERR);
+ }
+
+ status = ostatus;
+ child = -1;
+ atomic_signal_fence(memory_order_release);
+}
+
/*
* Signal handler for SIGALRM.
*/
diff --git a/usr.bin/lockf/tests/lockf_test.sh
b/usr.bin/lockf/tests/lockf_test.sh
index d73c7590653d..0cdf7dae6f57 100644
--- a/usr.bin/lockf/tests/lockf_test.sh
+++ b/usr.bin/lockf/tests/lockf_test.sh
@@ -62,6 +62,13 @@ basic_body()
atf_check test ! -e "testlock"
}
+atf_test_case bubble_error
+bubble_error_body()
+{
+ # Ensure that lockf bubbles up the error as expected.
+ atf_check -s exit:9 lockf testlock sh -c 'exit 9'
+}
+
atf_test_case fdlock
fdlock_body()
{
@@ -233,6 +240,7 @@ atf_init_test_cases()
{
atf_add_test_case badargs
atf_add_test_case basic
+ atf_add_test_case bubble_error
atf_add_test_case fdlock
atf_add_test_case keep
atf_add_test_case needfile
