From 51bbc777e982d956e198babd027273c63fdde90f Mon Sep 17 00:00:00 2001
From: Takahiro Itazuri <itazur@amazon.com>
Date: Thu, 20 Feb 2025 11:52:31 +0000
Subject: [PATCH] fix(jailer): Make "init" in new PID NS a session leader

https://man7.org/linux/man-pages/man7/pid_namespaces.7.html
> A process in an ancestor namespace can send signals to the "init"
> process of a child PID namespace only if the "init" process has
> established a handelr for that signal.

Firecracker (i.e. the "init" process of the new PID namespace) sets up
handlers for some signals including SIGHUP and jailer exits soon after
spawning firecracker into the new PID namespace. If the jailer process
is a session leader and its exit happens after firecracker configures
the signal handlers, SIGHUP will be sent to firecracker and be caught by
the handler unexpectedly.

In order to avoid the above issue, if jailer is a session leader,
creates a new session and makes the child process (i.e. firecracker)
become the leader of the new session to not get SIGHUP on the exit of
jailer.

Note that this is the case only if `--daemonize` is not passed. This is
because we use the double fork method to daemonize, making itself not a
session leader.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
---
 CHANGELOG.md           |  3 +++
 src/jailer/src/env.rs  | 42 +++++++++++++++++++++++++++++++++++++++++-
 src/jailer/src/main.rs |  4 ++++
 3 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 2c0de0f13aa..a7bf7d4c014 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -54,6 +54,9 @@ and this project adheres to
   Firecracker's `--parent-cpu-time-us` values, which caused development builds
   of Firecracker to crash (but production builds were unaffected as underflows
   do not panic in release mode).
+- [#5045](https://github.com/firecracker-microvm/firecracker/pull/5045): Fixed
+  an issue where firecracker intermittently receives SIGHUP when using jailer
+  with `--new-pid-ns` but without `--daemonize`.
 
 ## [1.10.1]
 
diff --git a/src/jailer/src/env.rs b/src/jailer/src/env.rs
index bf75802ebe7..61cd121b0dc 100644
--- a/src/jailer/src/env.rs
+++ b/src/jailer/src/env.rs
@@ -330,12 +330,52 @@ impl Env {
     }
 
     fn exec_into_new_pid_ns(&mut self, chroot_exec_file: PathBuf) -> Result<(), JailerError> {
+        // https://man7.org/linux/man-pages/man7/pid_namespaces.7.html
+        // > a process in an ancestor namespace can send signals to the "init" process of a child
+        // > PID namespace only if the "init" process has established a handler for that signal.
+        //
+        // Firecracker (i.e. the "init" process of the new PID namespace) sets up handlers for some
+        // signals including SIGHUP and jailer exits soon after spawning firecracker into a new PID
+        // namespace. If the jailer process is a session leader and its exit happens after
+        // firecracker configures the signal handlers, SIGHUP will be sent to firecracker and be
+        // caught by the handler unexpectedly.
+        //
+        // In order to avoid the above issue, if jailer is a session leader, creates a new session
+        // and makes the child process (i.e. firecracker) become the leader of the new session to
+        // not get SIGHUP on the exit of jailer.
+
+        // Check whether jailer is a session leader or not before clone().
+        // Note that, if `--daemonize` is passed, jailer is always not a session leader. This is
+        // because we use the double fork method, making itself not a session leader.
+        let is_session_leader = match self.daemonize {
+            true => false,
+            false => {
+                // SAFETY: Safe because it doesn't take any input parameters.
+                let sid = SyscallReturnCode(unsafe { libc::getsid(0) })
+                    .into_result()
+                    .map_err(JailerError::GetSid)?;
+                // SAFETY: Safe because it doesn't take any input parameters.
+                let ppid = SyscallReturnCode(unsafe { libc::getpid() })
+                    .into_result()
+                    .map_err(JailerError::GetPid)?;
+                sid == ppid
+            }
+        };
+
         // Duplicate the current process. The child process will belong to the previously created
         // PID namespace. The current process will not be moved into the newly created namespace,
         // but its first child will assume the role of init(1) in the new namespace.
         let pid = clone(std::ptr::null_mut(), libc::CLONE_NEWPID)?;
         match pid {
-            0 => Err(JailerError::Exec(self.exec_command(chroot_exec_file))),
+            0 => {
+                if is_session_leader {
+                    // SAFETY: Safe bacause it doesn't take any input parameters.
+                    SyscallReturnCode(unsafe { libc::setsid() })
+                        .into_empty_result()
+                        .map_err(JailerError::SetSid)?;
+                }
+                Err(JailerError::Exec(self.exec_command(chroot_exec_file)))
+            }
             child_pid => {
                 // Save the PID of the process running the exec file provided
                 // inside <chroot_exec_file>.pid file.
diff --git a/src/jailer/src/main.rs b/src/jailer/src/main.rs
index 4e00c14762a..fc7ac216599 100644
--- a/src/jailer/src/main.rs
+++ b/src/jailer/src/main.rs
@@ -85,6 +85,10 @@ pub enum JailerError {
     FromBytesWithNul(std::ffi::FromBytesWithNulError),
     #[error("Failed to get flags from fd: {0}")]
     GetOldFdFlags(io::Error),
+    #[error("Failed to get PID (getpid): {0}")]
+    GetPid(io::Error),
+    #[error("Failed to get SID (getsid): {0}")]
+    GetSid(io::Error),
     #[error("Invalid gid: {0}")]
     Gid(String),
     #[error("Invalid instance ID: {0}")]