fix: Only setup serial device if input is a terminal/FIFO

Currently, whenever firecracker starts with a daemonized jailer, it
prints the following warning: "Failed to register serial input fd:
event_manager: failed to manage epoll file descriptor: Operation not
permitted (os error 1)". This is because if the jailer daemonizes the
firecracker process, it will redirect stdin to /dev/null. However, when
trying to register FILENO_STDIN to epoll via epoll_ctl, it will return
EPERM if such a redirection has happened. However, if the jailer is
daemonized, then this will result in the correct behavior, because we do
not _want_ a serial device here.

The changes in this commit make firecracker only try to initialize the
serial device if FILENO_STDIN is a terminal (as reported by isatty(3))
or a FIFO pipe (as reported by fstat). This fixes the incorrect
warnings.

Signed-off-by: Patrick Roy <[email protected]>

Author: roypat

CHANGELOG.md

@@ -12,6 +12,10 @@
 - Better logs during validation of CPU ID in snapshot restoration path. Also
   Firecracker now does not fail if it can't get CPU ID from the host or
   can't find CPU ID in the snapshot.
+- Changed the serial device to only try to initialize itself if stdin is a terminal
+  or a FIFO pipe. This fixes logged warnings about the serial device failing to
+  initialize if the process is daemonized (in which case stdin is /dev/null instead
+  of a terminal).
 
 ### Fixed
 

src/vmm/src/devices/legacy/serial.rs

@@ -265,7 +265,14 @@ impl<I: Read + AsRawFd + Send + Debug> MutEventSubscriber
         if self.input.is_some() && self.serial.events().buffer_ready_event_fd.is_some() {
             let serial_fd = self.serial_input_fd();
             let buf_ready_evt = self.buffer_ready_evt_fd();
-            if serial_fd != -1 {
+
+            // If the jailer is instructed to daemonize before exec-ing into firecracker, we set
+            // stdin, stdout and stderr to be open('/dev/null'). However, if stdin is redirected
+            // from /dev/null then trying to register FILENO_STDIN to epoll will fail with EPERM.
+            // Therefore, only try to register stdin to epoll if it is a terminal or a FIFO pipe.
+            // SAFETY: isatty has no invariants that need to be upheld. If serial_fd is an invalid
+            // argument, it will return 0 and set errno to EBADF.
+            if unsafe { libc::isatty(serial_fd) } == 1 || is_fifo(serial_fd) {
                 if let Err(err) = ops.add(Events::new(&serial_fd, EventSet::IN)) {
                     warn!("Failed to register serial input fd: {}", err);
                 }
@@ -277,6 +284,24 @@ impl<I: Read + AsRawFd + Send + Debug> MutEventSubscriber
     }
 }
 
+/// Checks whether the given file descriptor is a FIFO pipe.
+fn is_fifo(fd: RawFd) -> bool {
+    let mut stat = std::mem::MaybeUninit::<libc::stat>::uninit();
+
+    // SAFETY: No unsafety can be introduced by passing in an invalid file descriptor to fstat,
+    // it will return -1 and set errno to EBADF. The pointer passed to fstat is valid for writing
+    // a libc::stat structure.
+    if unsafe { libc::fstat(fd, stat.as_mut_ptr()) } < 0 {
+        return false;
+    }
+
+    // SAFETY: We can safely assume the libc::stat structure to be initialized, as libc::fstat
+    // returning 0 guarantees that the memory is now initialized with the requested file metadata.
+    let stat = unsafe { stat.assume_init() };
+
+    (stat.st_mode & libc::S_IFIFO) != 0
+}
+
 impl<I: Read + AsRawFd + Send + Debug + 'static>
     SerialWrapper<EventFdTrigger, SerialEventsWrapper, I>
 {
@@ -303,6 +328,8 @@ impl<I: Read + AsRawFd + Send + Debug + 'static>
 
 #[cfg(test)]
 mod tests {
+    #![allow(clippy::undocumented_unsafe_blocks)]
+
     use utils::eventfd::EventFd;
 
     use super::*;
@@ -340,4 +367,23 @@ mod tests {
         // The `invalid_read_count` metric should be the same as before the one-byte reads.
         assert_eq!(invalid_reads_after_2, invalid_reads_after);
     }
+
+    #[test]
+    fn test_is_fifo() {
+        // invalid file descriptors arent fifos
+        let invalid = -1;
+        assert!(!is_fifo(invalid));
+
+        // Fifos are fifos
+        let mut fds: [libc::c_int; 2] = [0; 2];
+        let rc = unsafe { libc::pipe(fds.as_mut_ptr()) };
+        assert!(rc == 0);
+
+        assert!(is_fifo(fds[0]));
+        assert!(is_fifo(fds[1]));
+
+        // Files arent fifos
+        let tmp_file = utils::tempfile::TempFile::new().unwrap();
+        assert!(!is_fifo(tmp_file.as_file().as_raw_fd()));
+    }
 }

tests/integration_tests/functional/test_serial_io.py

@@ -219,3 +219,29 @@ def test_serial_block(test_microvm_with_api):
 
     # Should be significantly more than before the `cat` command.
     assert last_count - init_count > 10000
+
+
+REGISTER_FAILED_WARNING = "Failed to register serial input fd: event_manager: failed to manage epoll file descriptor: Operation not permitted (os error 1)"
+
+
+def test_no_serial_fd_error_when_daemonized(uvm_plain):
+    """
+    Tests that when running firecracker daemonized, the serial device
+    does not try to register stdin to epoll (which would fail due to stdin no
+    longer being pointed at a terminal).
+
+    Regression test for #4037.
+    """
+
+    test_microvm = uvm_plain
+    test_microvm.spawn()
+    test_microvm.add_net_iface()
+    test_microvm.basic_config(
+        vcpu_count=1,
+        mem_size_mib=512,
+    )
+    test_microvm.start()
+
+    test_microvm.ssh.run("true")
+
+    assert REGISTER_FAILED_WARNING not in test_microvm.log_data