From b4248f626b5accf5a61fa97c93d6376c59c8a0f3 Mon Sep 17 00:00:00 2001 From: Alexander Mikhalitsyn Date: Fri, 24 Jan 2025 14:07:36 +0100 Subject: [PATCH] lxc/attach: Revert "- LXC attach should exit on SIGCHLD" This reverts commit f02158439677d0c1d4b2ed2ed1ba9bc43923a05d. Let's revert this change as it introduces 2 regressions: 1. it's not correct to do exit(2) from a signal handler in this case, as we skip a proper cleaning procedures like restoring PTY configuration state (see lxc_terminal_delete()) which leads to a problem with a PTY after lxc-attach exits. [ hint: just try to use lxc-attach on a main branch with this change and you will see it. After lxc-attach exits you won't be able to type anything in your current terminal session as it's messed up. ] 2. this introduces race-condition in the code which leads to a regression on LXD/(and I believe Incus too) which can be seen as random "Failed to retrieve PID of executing child process" errors on "lxc exec"/"incus exec" commands. It's extremely hard to reproduce, but my guess is that we are getting a race condition here, because by the time when we set a new signal handler for SIGCHLD, transient process is still alive and when it exists it generates SIGCHLD which may lead to exit(). 3. This changes a behavior of lxc-attach which was there for *years* and it's quite scary to be honest. I'm not against having this change, but in a different form, for example we can add a new command line parameter for lxc-attach command which will enable this behavior. My first attempt was to fix that change to prevent race, but then I've noticed that we also have a more serious problem described in (1), this requires more work to do. Signed-off-by: Alexander Mikhalitsyn --- src/lxc/attach.c | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/lxc/attach.c b/src/lxc/attach.c index 27acd401e..8f2f7a37c 100644 --- a/src/lxc/attach.c +++ b/src/lxc/attach.c @@ -1430,15 +1430,6 @@ static inline void lxc_attach_terminal_close_log(struct lxc_terminal *terminal) close_prot_errno_disarm(terminal->log_fd); } -void lxc_attach_sig_handler(int signum) { - // exit on SIGCHLD, but not if sender-pid is 0 (main process) - if (signum == SIGCHLD) { - int sender_pid = waitpid(-1, NULL, WNOHANG); - if (sender_pid > 0) - exit(EXIT_SUCCESS); - } -} - int lxc_attach(struct lxc_container *container, lxc_attach_exec_t exec_function, void *exec_payload, lxc_attach_options_t *options, pid_t *attached_process) @@ -1743,7 +1734,6 @@ int lxc_attach(struct lxc_container *container, lxc_attach_exec_t exec_function, signal(SIGINT, SIG_IGN); signal(SIGQUIT, SIG_IGN); } - signal(SIGCHLD, lxc_attach_sig_handler); /* Reap transient process. */ ret = wait_for_pid(pid); -- 2.47.2