diff --git a/usr.bin/lockf/lockf.c b/usr.bin/lockf/lockf.c --- a/usr.bin/lockf/lockf.c +++ b/usr.bin/lockf/lockf.c @@ -34,6 +34,7 @@ #include #include #include +#include #include #include #include @@ -50,14 +51,19 @@ 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 @@ 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,31 @@ 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); + (void)sigaction(SIGCHLD, &sa_prev, NULL); close(lockfd); execvp(argv[0], argv); warn("%s", argv[0]); @@ -250,16 +282,24 @@ 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 +363,26 @@ _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 --- a/usr.bin/lockf/tests/lockf_test.sh +++ b/usr.bin/lockf/tests/lockf_test.sh @@ -62,6 +62,13 @@ 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_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