]> git.ipfire.org Git - thirdparty/lxc.git/commitdiff
lxc/attach: Revert "- LXC attach should exit on SIGCHLD" 4517/head
authorAlexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
Fri, 24 Jan 2025 13:07:36 +0000 (14:07 +0100)
committerAlexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
Fri, 24 Jan 2025 13:20:34 +0000 (14:20 +0100)
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 <aleksandr.mikhalitsyn@canonical.com>
src/lxc/attach.c

index 27acd401effa88eb291a8d01fec89a69cc4f466d..8f2f7a37c3a5e27667ed9db12ada1d12c8cb5f85 100644 (file)
@@ -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);