Unwind main() for clean shutdown (vs. libc::_exit())

The design of the existing main() function was such that Firecracker
never unwound the stack to the top level in order to end the program.
Instead, it would run some Firecracker-specific shutdown code and
then call `unsafe { libc::_exit() }`.

That skips some Rust-specific shutdown done by std::process::exit().
But even using that does not call destructors for the stack objects,
so it is not a "clean shutdown":

  https://doc.rust-lang.org/std/process/fn.exit.html

While a "dirty shutdown" can be a bit faster, the benefit of having
non-panic shutdowns run their finalization is that it helps with the
accounting of Valgrind and sanitizers.  It's a good sanity check to
make sure the program could exit cleanly if it wanted to.

This commit attempts a somewhat minimally-invasive means of bubbling
up an "ExitCode".  Exiting was managed uniquely by the Vmm event
subscriber during the process() callback, and requires running the
vmm.stop() method.  So this extends the Subscriber trait with an
extended form of callback named process_exitable() that returns
an optional exit code.

The event loop doesn't have a clear (to me) way to get at the VMM
to call stop() if a non-Vmm subscriber requests an exit.  So the
interface means any other subscriber that overrode process_exitable()
would miss the required stop code.

So it suggests the need for a redesign by someone more familiar with
the architecture.  However, it does accomplish a proof of concept by
getting closer to a clean bill of health from a Valgrind analysis.

Author: AE1020

src/firecracker/src/api_server_adapter.rs

@@ -12,7 +12,7 @@ use std::{
 use api_server::{ApiRequest, ApiResponse, ApiServer};
 use logger::{error, warn, ProcessTimeReporter};
 use mmds::MMDS;
-use polly::event_manager::{EventManager, Subscriber};
+use polly::event_manager::{EventManager, Subscriber, ExitCode};
 use seccomp::BpfProgram;
 use utils::{
     epoll::{EpollEvent, EventSet},
@@ -43,7 +43,7 @@ impl ApiServerAdapter {
         vm_resources: VmResources,
         vmm: Arc<Mutex<Vmm>>,
         event_manager: &mut EventManager,
-    ) {
+    ) -> ExitCode {
         let api_adapter = Arc::new(Mutex::new(Self {
             api_event_fd,
             from_api,
@@ -53,10 +53,13 @@ impl ApiServerAdapter {
         event_manager
             .add_subscriber(api_adapter)
             .expect("Cannot register the api event to the event manager.");
+
         loop {
-            event_manager
-                .run()
+            let opt_exit_code = event_manager
+                .run_maybe_exiting()
                 .expect("EventManager events driver fatal error");
+
+            if let Some(exit_code) = opt_exit_code { return exit_code; }
         }
     }
 
@@ -127,7 +130,7 @@ pub(crate) fn run_with_api(
     instance_info: InstanceInfo,
     process_time_reporter: ProcessTimeReporter,
     boot_timer_enabled: bool,
-) {
+) -> ExitCode {
     // FD to notify of API events. This is a blocking eventfd by design.
     // It is used in the config/pre-boot loop which is a simple blocking loop
     // which only consumes API events.
@@ -242,5 +245,5 @@ pub(crate) fn run_with_api(
         vm_resources,
         vmm,
         &mut event_manager,
-    );
+    )
 }

src/firecracker/src/main.rs

@@ -11,7 +11,7 @@ use std::process;
 use std::sync::{Arc, Mutex};
 
 use logger::{error, info, IncMetric, ProcessTimeReporter, LOGGER, METRICS};
-use polly::event_manager::EventManager;
+use polly::event_manager::{EventManager, ExitCode};
 use seccomp::{BpfProgram, SeccompLevel};
 use utils::arg_parser::{ArgParser, Argument};
 use utils::terminal::Terminal;
@@ -30,7 +30,7 @@ const DEFAULT_API_SOCK_PATH: &str = "/run/firecracker.socket";
 const DEFAULT_INSTANCE_ID: &str = "anonymous-instance";
 const FIRECRACKER_VERSION: &str = env!("FIRECRACKER_VERSION");
 
-fn main() {
+fn main_exitable() -> ExitCode {
     LOGGER
         .configure(Some(DEFAULT_INSTANCE_ID.to_string()))
         .expect("Failed to register logger");
@@ -254,17 +254,32 @@ fn main() {
             instance_info,
             process_time_reporter,
             boot_timer_enabled,
-        );
+        )
     } else {
         run_without_api(
             seccomp_filter,
             vmm_config_json,
             &instance_info,
             boot_timer_enabled,
-        );
+        )
     }
 }
 
+fn main () {
+    // This idiom is the prescribed way to get a clean shutdown of Rust (that will report
+    // no leaks in Valgrind or sanitizers).  Calling `unsafe { libc::exit() }` does no
+    // cleanup, and std::process::exit() does more--but does not run destructors.  So the
+    // best thing to do is to is bubble up the exit code through the whole stack, and
+    // only exit when everything potentially destructible has cleaned itself up.
+    //
+    // https://doc.rust-lang.org/std/process/fn.exit.html
+    //
+    // See process_exitable() method of Subscriber trait for what triggers the exit_code.
+    //
+    let exit_code = main_exitable();
+    std::process::exit(i32::from(exit_code));
+}
+
 // Print supported snapshot data format versions.
 fn print_supported_snapshot_versions() {
     let mut snapshot_versions_str = "Supported snapshot data format versions:".to_string();
@@ -316,7 +331,7 @@ fn run_without_api(
     config_json: Option<String>,
     instance_info: &InstanceInfo,
     bool_timer_enabled: bool,
-) {
+) -> ExitCode {
     let mut event_manager = EventManager::new().expect("Unable to create EventManager");
 
     // Right before creating the signalfd,
@@ -357,8 +372,10 @@ fn run_without_api(
 
     // Run the EventManager that drives everything in the microVM.
     loop {
-        event_manager
-            .run()
+        let opt_exit_code = event_manager
+            .run_maybe_exiting()
             .expect("Failed to start the event manager");
+
+        if let Some(exit_code) = opt_exit_code { return exit_code; }
     }
 }

src/polly/src/event_manager.rs

@@ -12,6 +12,15 @@ use utils::epoll::{self, Epoll, EpollEvent};
 pub type Result<T> = std::result::Result<T, Error>;
 pub type Pollable = RawFd;
 
+/// Process Exit Code, historically an 8 bit number on Unix; use a type alias
+/// since `fn something(...) -> u8` is non-obvious.
+///
+///   https://tldp.org/LDP/abs/html/exitcodes.html
+///
+/// !!! Should i32 be used instead, since `std::process::exit` allows it?
+///
+pub type ExitCode = u8;
+
 /// Errors associated with epoll events handling.
 pub enum Error {
     /// Cannot create epoll fd.
@@ -57,7 +66,25 @@ pub trait Subscriber {
     ///                   The only functions safe to call on this `EventManager` reference
     ///                   are `register`, `unregister` and `modify` which correspond to
     ///                   the `libc::epoll_ctl` operations.
-    fn process(&mut self, event: &EpollEvent, event_manager: &mut EventManager);
+    fn process(&mut self, _event: &EpollEvent, _event_manager: &mut EventManager) {
+        //
+        // !!! Can it be compile-time checked that one-and-only-one of process() or
+        // process_exitable() are overloaded?
+        //
+        panic!("Either process() or process_exitable() must be implemented for Subscriber");
+    }
+
+    /// Variant of the process() callback that lets the subscriber signal a desire to exit
+    /// by returning a non-None ExitCode.
+    ///
+    /// This is what's actually called when dispatching subscriptions, but it delegates
+    /// to the plain form.  That way most subscribers implement process() and can avoid
+    /// needing to `return None` to signal not exiting.
+    ///
+    fn process_exitable(&mut self, event: &EpollEvent, event_manager: &mut EventManager) -> Option<ExitCode> {
+        self.process(event, event_manager);
+        None
+    }
 
     /// Returns a list of `EpollEvent` that this subscriber is interested in.
     fn interest_list(&self) -> Vec<EpollEvent>;
@@ -170,41 +197,74 @@ impl EventManager {
         Ok(())
     }
 
-    /// Wait for events, then dispatch to the registered event handlers.
-    pub fn run(&mut self) -> Result<usize> {
-        self.run_with_timeout(-1)
-    }
-
     /// Wait for events for a maximum timeout of `miliseconds`. Dispatch the events to the
     /// registered signal handlers.
-    pub fn run_with_timeout(&mut self, milliseconds: i32) -> Result<usize> {
+    ///
+    /// First return is None if the events were processed and no exit events were encountered.
+    /// If an exit event happens and `--cleanup-level` is 2, the exit code will be returned.
+    /// Lower cleanup levels that get exit events will call libc::exit(), thus not return
+    ///
+    /// Second return is the number of dispatched events, which is only used by the test code.
+    pub fn run_core(&mut self, milliseconds: i32) -> Result<(Option<ExitCode>, usize)> {
         let event_count = match self.epoll.wait(milliseconds, &mut self.ready_events[..]) {
             Ok(event_count) => event_count,
             Err(e) if e.raw_os_error() == Some(libc::EINTR) => 0,
             Err(e) => return Err(Error::Poll(e)),
         };
-        self.dispatch_events(event_count);
+        let opt_exit_code = self.dispatch_events(event_count);
+
+        Ok((opt_exit_code, event_count))  // event count is checked by the tests
+    }
 
+    /// Version of run used by Firecracker, which doesn't need the count of dispatched events
+    /// or a timeout.  It wants to be able to receive an exit code if `--cleanup-level` is 2.
+    pub fn run_maybe_exiting(&mut self) -> Result<Option<ExitCode>> {
+        let (opt_exit_code, _event_count) = self.run_core(-1)?;
+        Ok(opt_exit_code)
+    }
+
+    /// Variation of run used by tests which want a count of dispatched events, and also
+    /// a timeout.  The tests were written before the concept of clean shutdown, so they
+    /// assume any exit events would have terminated Firecracker abuptly.
+    pub fn run_with_timeout(&mut self, milliseconds: i32) -> Result<usize> {
+        let (opt_exit_code, event_count) = self.run_core(milliseconds)?;
+        assert!(opt_exit_code.is_none());
         Ok(event_count)
     }
 
-    fn dispatch_events(&mut self, event_count: usize) {
+    /// Variation of run used by tests which don't want to use a timeout.
+    pub fn run(&mut self) -> Result<usize> {
+        self.run_with_timeout(-1)
+    }
+
+    fn dispatch_events(&mut self, event_count: usize) -> Option<ExitCode> {
         // Use the temporary, pre-allocated buffer to check ready events.
         for ev_index in 0..event_count {
             let event = &self.ready_events[ev_index].clone();
             let pollable = event.fd();
 
             if self.subscribers.contains_key(&pollable) {
-                self.subscribers
+                let opt_exit_code = self.subscribers
                     .get_mut(&pollable)
                     .unwrap() // Safe because we have already checked existence
                     .clone()
                     .lock()
                     .expect("Poisoned lock")
-                    .process(&event, self);
+                    .process_exitable(&event, self);
+
+                if opt_exit_code != None {
+                    //
+                    // Note: allowing other handlers to run could create an
+                    // ambiguity of which exit code to use if more than one asked
+                    // to exit.  Thus we signal exit on the first handler that asks.
+                    //
+                    return opt_exit_code;
+                }
             }
             // TODO: Should we log an error in case the subscriber does not exist?
         }
+
+        None
     }
 }
 

src/vmm/src/lib.rs

@@ -58,7 +58,7 @@ use devices::virtio::{
 };
 use devices::BusDevice;
 use logger::{error, info, warn, LoggerError, MetricsError, METRICS};
-use polly::event_manager::{EventManager, Subscriber};
+use polly::event_manager::{EventManager, Subscriber, ExitCode};
 use rate_limiter::BucketUpdate;
 use seccomp::BpfProgramRef;
 use snapshot::Persist;
@@ -339,8 +339,9 @@ impl Vmm {
             .map_err(Error::I8042Error)
     }
 
-    /// Waits for all vCPUs to exit and terminates the Firecracker process.
-    pub fn stop(&mut self, exit_code: i32) {
+    /// Waits for all vCPUs to exit.  Does not terminate the Firecracker process.
+    /// (See notes in main() about why ExitCode is bubbled up for clean shutdown.)
+    pub fn stop(&mut self) {
         info!("Vmm is stopping.");
 
         if let Some(observer) = self.events_observer.as_mut() {
@@ -353,12 +354,6 @@ impl Vmm {
         if let Err(e) = METRICS.write() {
             error!("Failed to write metrics while stopping: {}", e);
         }
-
-        // Exit from Firecracker using the provided exit code. Safe because we're terminating
-        // the process anyway.
-        unsafe {
-            libc::_exit(exit_code);
-        }
     }
 
     /// Saves the state of a paused Microvm.
@@ -717,7 +712,7 @@ impl Drop for Vmm {
 
 impl Subscriber for Vmm {
     /// Handle a read event (EPOLLIN).
-    fn process(&mut self, event: &EpollEvent, _: &mut EventManager) {
+    fn process_exitable(&mut self, event: &EpollEvent, _: &mut EventManager) -> Option<ExitCode> {
         let source = event.fd();
         let event_set = event.event_set();
 
@@ -735,9 +730,11 @@ impl Subscriber for Vmm {
                     _ => None,
                 })
                 .unwrap_or(FC_EXIT_CODE_OK);
-            self.stop(i32::from(exit_code));
+            self.stop();
+            Some(exit_code)
         } else {
             error!("Spurious EventManager event for handler: Vmm");
+            None
         }
     }
 

src/vmm/tests/integration_tests.rs

@@ -73,7 +73,7 @@ fn test_build_microvm() {
             let _ = event_manager.run_with_timeout(500).unwrap();
 
             #[cfg(target_arch = "x86_64")]
-            vmm.lock().unwrap().stop(-1); // If we got here, something went wrong.
+            vmm.lock().unwrap().stop(1); // If we got here, something went wrong.
             #[cfg(target_arch = "aarch64")]
             vmm.lock().unwrap().stop(0);
         }
@@ -104,7 +104,7 @@ fn test_vmm_seccomp() {
             // Give the vCPUs a chance to attempt KVM_RUN.
             thread::sleep(Duration::from_millis(200));
             // Should never get here.
-            vmm.lock().unwrap().stop(-1);
+            vmm.lock().unwrap().stop(1);
         }
         vmm_pid => {
             // Parent process: wait for the vmm to exit.
@@ -160,7 +160,7 @@ fn test_pause_resume_microvm() {
             let _ = event_manager.run_with_timeout(500).unwrap();
 
             #[cfg(target_arch = "x86_64")]
-            vmm.lock().unwrap().stop(-1); // If we got here, something went wrong.
+            vmm.lock().unwrap().stop(1); // If we got here, something went wrong.
             #[cfg(target_arch = "aarch64")]
             vmm.lock().unwrap().stop(0);
         }
@@ -193,7 +193,7 @@ fn test_dirty_bitmap_error() {
             let _ = event_manager.run_with_timeout(500).unwrap();
 
             #[cfg(target_arch = "x86_64")]
-            vmm.lock().unwrap().stop(-1); // If we got here, something went wrong.
+            vmm.lock().unwrap().stop(1); // If we got here, something went wrong.
             #[cfg(target_arch = "aarch64")]
             vmm.lock().unwrap().stop(0);
         }