Revert "slave: use an event FD to communicate errors after fork before exec"

This reverts commit 027a8de6f5.

I was a bit quick on this one, and only tested error cases. Turns out,
this causes foot to hang in a blocking read(3) on the event FD.

This is because while the FD has FD_CLOEXEC, that flag is per file
descriptor. I.e. the forked copy of the event fd is closed when we
exec(), but since this isn't a pipe, nothing is signaled to the parent
process, which thus remains blocking in read(3) since its copy of the
event fd hasn't been closed.

We _could_ write a dummy value on the event fd just before exec(), but
then we wouldn't be able to catch errors from exec() self - only
errors *before* exec().
This commit is contained in:
Daniel Eklöf 2020-08-01 09:41:31 +02:00
parent 027a8de6f5
commit bae3244ec6
No known key found for this signature in database
GPG key ID: 5BBD4992C116573F

37
slave.c
View file

@ -12,7 +12,6 @@
#include <sys/stat.h>
#include <sys/ioctl.h>
#include <sys/eventfd.h>
#include <fcntl.h>
#define LOG_MODULE "slave"
@ -232,7 +231,7 @@ slave_exec(int ptmx, char *argv[], int err_fd, bool login_shell,
execvp(file, argv);
err:
(void)!write(err_fd, &(uint64_t){errno}, sizeof(uint64_t));
(void)!write(err_fd, &errno, sizeof(errno));
if (pts != -1)
close(pts);
if (ptmx != -1)
@ -246,9 +245,9 @@ slave_spawn(int ptmx, int argc, const char *cwd, char *const *argv,
const char *term_env, const char *conf_shell, bool login_shell,
const user_notifications_t *notifications)
{
int error_fd = eventfd(0, EFD_CLOEXEC);
if (error_fd < 0) {
LOG_ERRNO("failed to create event FD");
int fork_pipe[2];
if (pipe2(fork_pipe, O_CLOEXEC) < 0) {
LOG_ERRNO("failed to create pipe");
return -1;
}
@ -256,16 +255,18 @@ slave_spawn(int ptmx, int argc, const char *cwd, char *const *argv,
switch (pid) {
case -1:
LOG_ERRNO("failed to fork");
close(error_fd);
close(fork_pipe[0]);
close(fork_pipe[1]);
return -1;
case 0:
/* Child */
close(fork_pipe[0]); /* Close read end */
if (chdir(cwd) < 0) {
const int _errno = errno;
LOG_ERRNO("failed to change working directory");
(void)!write(error_fd, &(uint64_t){_errno}, sizeof(uint64_t));
(void)!write(fork_pipe[1], &_errno, sizeof(_errno));
_exit(_errno);
}
@ -280,7 +281,7 @@ slave_spawn(int ptmx, int argc, const char *cwd, char *const *argv,
{
const int _errno = errno;
LOG_ERRNO_P("failed to restore signals", errno);
(void)!write(error_fd, &(uint64_t){_errno}, sizeof(uint64_t));
(void)!write(fork_pipe[1], &_errno, sizeof(_errno));
_exit(_errno);
}
@ -292,10 +293,9 @@ slave_spawn(int ptmx, int argc, const char *cwd, char *const *argv,
if (argc == 0) {
char *shell_copy = strdup(conf_shell);
if (!tokenize_cmdline(shell_copy, &_shell_argv)) {
const int _errno = errno;
free(shell_copy);
(void)!write(error_fd, &(uint64_t){_errno}, sizeof(uint64_t));
_exit(_errno);
(void)!write(fork_pipe[1], &errno, sizeof(errno));
_exit(0);
}
shell_argv = _shell_argv;
} else {
@ -311,23 +311,26 @@ slave_spawn(int ptmx, int argc, const char *cwd, char *const *argv,
if (is_valid_shell(shell_argv[0]))
setenv("SHELL", shell_argv[0], 1);
slave_exec(ptmx, shell_argv, error_fd, login_shell, notifications);
slave_exec(ptmx, shell_argv, fork_pipe[1], login_shell, notifications);
assert(false);
break;
default: {
close(fork_pipe[1]); /* Close write end */
LOG_DBG("slave has PID %d", pid);
uint64_t _errno;
ssize_t ret = read(error_fd, &_errno, sizeof(_errno));
close(error_fd);
int _errno;
static_assert(sizeof(errno) == sizeof(_errno), "errno size mismatch");
ssize_t ret = read(fork_pipe[0], &_errno, sizeof(_errno));
close(fork_pipe[0]);
if (ret < 0) {
LOG_ERRNO("failed to read from error FD");
LOG_ERRNO("failed to read from pipe");
return -1;
} else if (ret == sizeof(_errno)) {
LOG_ERRNO_P(
"%s: failed to execute", (int)_errno, argc == 0 ? conf_shell : argv[0]);
"%s: failed to execute", _errno, argc == 0 ? conf_shell : argv[0]);
return -1;
} else
LOG_DBG("%s: successfully started", conf_shell);