Merge pull request #225 from memorysafety/117-some-better-errors

Some better errors on some well known failure scenarios

ntp-daemon/Cargo.toml

@@ -22,6 +22,7 @@ sentry = { version = "0.27.0", optional = true }
 sentry-tracing = { version = "0.27.0", optional = true }
 rand = "0.8.5"
 libc = "0.2.126"
+exitcode = "1.1.2"
 
 [dev-dependencies]
 ntp-proto = { path = "../ntp-proto", features=["ext-test"]}

ntp-daemon/src/main.rs

@@ -3,6 +3,7 @@
 use clap::Parser;
 use ntp_daemon::config::{CmdArgs, Config};
 use std::error::Error;
+use tracing::debug;
 use tracing_subscriber::EnvFilter;
 
 #[tokio::main]
@@ -17,16 +18,32 @@ async fn main() -> Result<(), Box<dyn Error>> {
     let finish_tracing_init =
         ntp_daemon::tracing::init(log_filter, args.log_format.unwrap_or_default());
 
-    let mut config = Config::from_args(args.config, args.peers).await?;
+    let mut config = match Config::from_args(args.config, args.peers).await {
+        Ok(c) => c,
+        Err(e) => {
+            // print to stderr because tracing is not yet setup
+            eprintln!("There was an error loading the config: {}", e);
+            std::process::exit(exitcode::CONFIG);
+        }
+    };
 
     // Sentry has a guard we need to keep alive, so store it.
     // The compiler will optimize this away when not using sentry.
-    let tracing_state = finish_tracing_init(&mut config, has_log_override, has_format_override)?;
+    let tracing_state =
+        match finish_tracing_init(&mut config, has_log_override, has_format_override) {
+            Ok(s) => s,
+            Err(e) => {
+                // print to stderr because tracing was not correctly initialized
+                eprintln!("Failed to complete logging setup: {}", e);
+                std::process::exit(exitcode::CONFIG);
+            }
+        };
 
     // Warn/error if the config is unreasonable. We do this after finishing
     // tracing setup to ensure logging is fully configured.
     config.check();
 
+    debug!("Configuration loaded, spawning daemon jobs");
     let (main_loop_handle, channels) = ntp_daemon::spawn(config.system, &config.peers).await?;
 
     ntp_daemon::observer::spawn(&config.observe, channels.peers, channels.system).await;

ntp-daemon/src/peer.rs

@@ -6,7 +6,7 @@ use ntp_proto::{
 };
 use ntp_udp::UdpSocket;
 use rand::{thread_rng, Rng};
-use tracing::{debug, instrument, warn};
+use tracing::{debug, error, instrument, warn};
 
 use tokio::{
     net::ToSocketAddrs,
@@ -150,7 +150,10 @@ where
         match self.clock.now() {
             Err(e) => {
                 // we cannot determine the origin_timestamp
-                panic!("`clock.now()` reported an error: {:?}", e)
+                error!(error = ?e, "There was an error retrieving the current time");
+
+                // report as no permissions, since this seems the most likely
+                std::process::exit(exitcode::NOPERM);
             }
             Ok(ts) => {
                 self.last_send_timestamp = Some(ts);
@@ -293,7 +296,10 @@ where
                     }
                 }
             };
+            // Unwrap should be safe because we know the socket was bound to a local addres just before
             let our_id = ReferenceId::from_ip(socket.as_ref().local_addr().unwrap().ip());
+
+            // Unwrap should be safe because we know the socket was connected to a remote peer just before
             let peer_id = ReferenceId::from_ip(socket.as_ref().peer_addr().unwrap().ip());
 
             let local_clock_time = NtpInstant::now();

ntp-daemon/src/system.rs

@@ -8,7 +8,7 @@ use ntp_proto::{
     ClockController, ClockUpdateResult, FilterAndCombine, NtpClock, NtpInstant, PeerSnapshot,
     PollInterval, SystemConfig, SystemSnapshot,
 };
-use tracing::info;
+use tracing::{error, info};
 
 use std::sync::Arc;
 use tokio::{
@@ -154,11 +154,8 @@ impl<C: NtpClock> System<C> {
         );
         match adjust_type {
             ClockUpdateResult::Panic => {
-                panic!(
-                    r"Unusually large clock step suggested,
-                                please manually verify system clock and reference clock
-                                     state and restart if appropriate."
-                )
+                error!("Unusually large clock step suggested, please manually verify system clock and reference clock state and restart if appropriate.");
+                std::process::exit(exitcode::SOFTWARE);
             }
             ClockUpdateResult::Step => {
                 self.reset_peers().await;

ntp-proto/Cargo.toml

@@ -15,3 +15,4 @@ md-5 = "0.10.1"
 rand = "0.8.5"
 tracing = "0.1.35"
 serde = { version = "1.0.140", features = ["derive"] }
+exitcode = "1.1.2"

ntp-proto/src/clock.rs

@@ -60,7 +60,10 @@ pub enum ClockUpdateResult {
 
 impl<C: NtpClock> ClockController<C> {
     pub fn new(clock: C) -> Self {
-        clock.set_freq(0.).expect("Unable to set clock frequency");
+        if let Err(e) = clock.set_freq(0.) {
+            error!(error = %e, "Could not set clock frequency, exiting");
+            std::process::exit(exitcode::NOPERM);
+        }
         Self {
             clock,
             state: ClockState::StartupBlank,
@@ -196,15 +199,17 @@ impl<C: NtpClock> ClockController<C> {
 
         // It is reasonable to panic here, as there is very little we can
         // be expected to do if the clock is not amenable to change
-        self.clock
-            .update_clock(
-                self.offset,
-                jitter,
-                root_delay / 2 + root_dispersion,
-                self.preferred_poll_interval,
-                leap_status,
-            )
-            .expect("Unable to update clock");
+        let result = self.clock.update_clock(
+            self.offset,
+            jitter,
+            root_delay / 2 + root_dispersion,
+            self.preferred_poll_interval,
+            leap_status,
+        );
+        if let Err(e) = result {
+            error!(error = %e, "Failed to update the clock, exiting");
+            std::process::exit(exitcode::NOPERM);
+        }
 
         // Adjust whether we would prefer to have a longer or shorter
         // poll interval depending on the amount of jitter
@@ -294,7 +299,10 @@ impl<C: NtpClock> ClockController<C> {
         self.preferred_poll_interval = PollInterval::MIN;
         // It is reasonable to panic here, as there is very little we can
         // be expected to do if the clock is not amenable to change
-        self.clock.step_clock(offset).expect("Unable to step clock");
+        if let Err(e) = self.clock.step_clock(offset) {
+            error!(error = %e, "Could not step the clock, exiting");
+            std::process::exit(exitcode::NOPERM);
+        }
         self.offset = NtpDuration::ZERO;
         self.last_update_time = last_peer_update;
         self.state = match self.state {
@@ -318,12 +326,14 @@ impl<C: NtpClock> ClockController<C> {
             ),
             "Setting initial frequency"
         );
-        self.clock
-            .set_freq(
-                offset.to_seconds()
-                    / NtpInstant::abs_diff(last_peer_update, self.last_update_time).to_seconds(),
-            )
-            .expect("Unable to adjust clock frequency");
+        let result = self.clock.set_freq(
+            offset.to_seconds()
+                / NtpInstant::abs_diff(last_peer_update, self.last_update_time).to_seconds(),
+        );
+        if let Err(e) = result {
+            error!(error = %e, "Unable to adjust clock frequency, exiting");
+            std::process::exit(exitcode::NOPERM);
+        }
     }
 }
 

ntp-proto/src/time_types.rs

@@ -23,7 +23,10 @@ impl NtpInstant {
 
     pub fn abs_diff(self, rhs: Self) -> NtpDuration {
         // our code should always give the bigger argument first.
-        debug_assert!(self >= rhs);
+        debug_assert!(
+            self >= rhs,
+            "self >= rhs, this could indicate another program adjusted the clock"
+        );
 
         // NOTE: `std::time::Duration` cannot be negative, so a simple `lhs - rhs` could give an
         // empty duration. In our logic, we're always interested in the absolute delta between two