From 8e3134ed61277ee5697683c6d318c38b38702260 Mon Sep 17 00:00:00 2001 From: Diggory Hardy Date: Thu, 8 Feb 2018 11:58:54 +0000 Subject: [PATCH 1/2] Error handling revisions New Unexpected error kind Error's kind and msg fields are now public Some error handling simplifications and revisions --- src/error.rs | 68 +++++++++++++++++++++++--------------- src/jitter.rs | 2 ++ src/os.rs | 61 +++++++++++++++++----------------- src/read.rs | 9 ++++- src/reseeding.rs | 85 +++++++++++++++++++++--------------------------- 5 files changed, 120 insertions(+), 105 deletions(-) diff --git a/src/error.rs b/src/error.rs index 266bb0c2946..edaff53119e 100644 --- a/src/error.rs +++ b/src/error.rs @@ -18,15 +18,33 @@ use std::error::Error as stdError; /// Error kind which can be matched over. #[derive(PartialEq, Eq, Debug, Copy, Clone)] pub enum ErrorKind { - /// Permanent failure: likely not recoverable without user action. + /// Feature is not available; not recoverable. + /// + /// This is the most permanent failure type and implies the error cannot be + /// resolved simply by retrying (e.g. the feature may not exist in this + /// build of the application or on the current platform). Unavailable, - /// Temporary failure: recommended to retry a few times, but may also be - /// irrecoverable. + /// General failure; there may be a chance of recovery on retry. + /// + /// This is the catch-all kind for errors from known and unknown sources + /// which do not have a more specific kind / handling method. + /// + /// It is suggested to retry a couple of times or retry later when + /// handling; some error sources may be able to resolve themselves, + /// although this is not likely. + Unexpected, + /// A transient failure which likely can be resolved or worked around. + /// + /// This error kind exists for a few specific cases where it is known that + /// the error likely can be resolved internally, but is reported anyway. Transient, /// Not ready yet: recommended to try again a little later. + /// + /// This error kind implies the generator needs more time or needs some + /// other part of the application to do something else first before it is + /// ready for use; for example this may be used by external generators + /// which require time for initialization. NotReady, - /// Uncategorised error - Other, #[doc(hidden)] __Nonexhaustive, } @@ -36,10 +54,7 @@ impl ErrorKind { /// /// See also `should_wait()`. pub fn should_retry(self) -> bool { - match self { - ErrorKind::Transient | ErrorKind::NotReady => true, - _ => false, - } + self != ErrorKind::Unavailable } /// True if we should retry but wait before retrying @@ -52,24 +67,30 @@ impl ErrorKind { /// A description of this error kind pub fn description(self) -> &'static str { match self { - ErrorKind::Unavailable => "permanent failure", + ErrorKind::Unavailable => "permanently unavailable", + ErrorKind::Unexpected => "unexpected failure", ErrorKind::Transient => "transient failure", ErrorKind::NotReady => "not ready yet", - ErrorKind::Other => "uncategorised", ErrorKind::__Nonexhaustive => unreachable!(), } } } + /// Error type of random number generators /// /// This is a relatively simple error type, designed for compatibility with and /// without the Rust `std` library. It embeds a "kind" code, a message (static -/// string only), and an optional chained cause (`std` only). +/// string only), and an optional chained cause (`std` only). The `kind` and +/// `msg` fields can be accessed directly; cause can be accessed via +/// `std::error::Error::cause` or `Error::take_cause`. Construction can only be +/// done via `Error::new` or `Error::with_cause`. #[derive(Debug)] pub struct Error { - kind: ErrorKind, - msg: &'static str, + /// The error kind + pub kind: ErrorKind, + /// The error message + pub msg: &'static str, #[cfg(feature="std")] cause: Option>, } @@ -110,14 +131,11 @@ impl Error { Error { kind: kind, msg: msg } } - /// Get the error kind - pub fn kind(&self) -> ErrorKind { - self.kind - } - - /// Get the error message - pub fn msg(&self) -> &'static str { - self.msg + /// Take the cause, if any. This allows the embedded cause to be extracted. + /// This uses `Option::take`, leaving `self` with no cause. + #[cfg(feature="std")] + pub fn take_cause(&mut self) -> Option> { + self.cause.take() } } @@ -126,12 +144,10 @@ impl fmt::Display for Error { #[cfg(feature="std")] { if let Some(ref cause) = self.cause { return write!(f, "{} ({}); cause: {}", - self.msg(), - self.kind.description(), - cause); + self.msg, self.kind.description(), cause); } } - write!(f, "{} ({})", self.msg(), self.kind.description()) + write!(f, "{} ({})", self.msg, self.kind.description()) } } diff --git a/src/jitter.rs b/src/jitter.rs index 5fdcf1c6765..2a5e5015c24 100644 --- a/src/jitter.rs +++ b/src/jitter.rs @@ -145,6 +145,8 @@ impl ::std::error::Error for TimerError { impl From for Error { fn from(err: TimerError) -> Error { + // Timer check is already quite permissive of failures so we don't + // expect false-positive failures, i.e. any error is irrecoverable. Error::with_cause(ErrorKind::Unavailable, "timer jitter failed basic quality tests", err) } diff --git a/src/os.rs b/src/os.rs index a7c10509e92..459d531ca26 100644 --- a/src/os.rs +++ b/src/os.rs @@ -82,31 +82,27 @@ impl RngCore for OsRng { panic!("OsRng failed too many times; last error: {}", e); } - match e.kind() { - ErrorKind::Transient => { - if !error_logged { - warn!("OsRng failed; retrying up to {} times. Error: {}", - TRANSIENT_RETRIES, e); - error_logged = true; - } - err_count += (RETRY_LIMIT + TRANSIENT_RETRIES - 1) - / TRANSIENT_RETRIES; // round up - continue; - } - ErrorKind::NotReady => { - if !error_logged { - warn!("OsRng failed; waiting up to {}s and retrying. Error: {}", - MAX_RETRY_PERIOD, e); - error_logged = true; - } - err_count += 1; - thread::sleep(wait_dur); - continue; + if e.kind.should_wait() { + if !error_logged { + warn!("OsRng failed; waiting up to {}s and retrying. Error: {}", + MAX_RETRY_PERIOD, e); + error_logged = true; } - _ => { - error!("OsRng failed: {}", e); - panic!("OsRng fatal error: {}", e); + err_count += 1; + thread::sleep(wait_dur); + continue; + } else if e.kind.should_retry() { + if !error_logged { + warn!("OsRng failed; retrying up to {} times. Error: {}", + TRANSIENT_RETRIES, e); + error_logged = true; } + err_count += (RETRY_LIMIT + TRANSIENT_RETRIES - 1) + / TRANSIENT_RETRIES; // round up + continue; + } else { + error!("OsRng failed: {}", e); + panic!("OsRng fatal error: {}", e); } } @@ -147,11 +143,16 @@ impl ReadRng { let mut guard = mutex.lock().unwrap(); if (*guard).is_none() { info!("OsRng: opening random device {}", path.as_ref().display()); - let file = File::open(path).map_err(|err| Error::with_cause( - ErrorKind::Unavailable, - "error opening random device", - err - ))?; + let file = File::open(path).map_err(|err| { + use std::io::ErrorKind::*; + match err.kind() { + NotFound | PermissionDenied | ConnectionRefused | BrokenPipe => + Error::with_cause(ErrorKind::Unavailable, + "permanent error while opening random device", err), + _ => Error::with_cause(ErrorKind::Unexpected, + "unexpected error while opening random device", err) + } + })?; *guard = Some(file); } @@ -169,7 +170,7 @@ impl ReadRng { let mut file = (*guard).as_mut().unwrap(); // Use `std::io::read_exact`, which retries on `ErrorKind::Interrupted`. file.read_exact(dest).map_err(|err| { - Error::with_cause(ErrorKind::Unavailable, "error reading random device", err) + Error::with_cause(ErrorKind::Unexpected, "error reading random device", err) }) } } @@ -250,7 +251,7 @@ mod imp { )); } else { return Err(Error::with_cause( - ErrorKind::Unavailable, + ErrorKind::Unexpected, "unexpected getrandom error", err, )); diff --git a/src/read.rs b/src/read.rs index 9eec5bec2fc..4b033163394 100644 --- a/src/read.rs +++ b/src/read.rs @@ -62,7 +62,14 @@ impl RngCore for ReadRng { if dest.len() == 0 { return Ok(()); } // Use `std::io::read_exact`, which retries on `ErrorKind::Interrupted`. self.reader.read_exact(dest).map_err(|err| { - Error::with_cause(ErrorKind::Unavailable, "ReadRng: read error", err) + use std::io::ErrorKind::*; + match err.kind() { + NotFound | PermissionDenied | ConnectionRefused | BrokenPipe => + Error::with_cause(ErrorKind::Unavailable, + "ReadRng: resource unavailable", err), + _ => Error::with_cause(ErrorKind::Unexpected, + "ReadRng: unexpected error", err) + } }) } } diff --git a/src/reseeding.rs b/src/reseeding.rs index 38a6cec826a..637a0b4b99b 100644 --- a/src/reseeding.rs +++ b/src/reseeding.rs @@ -83,63 +83,43 @@ impl ReseedingRng { /// Reseed the internal PRNG. /// /// This will try to work around errors in the RNG used for reseeding - /// intelligently. If the error kind indicates retrying might help, it will - /// immediately retry a couple of times. If the error kind indicates the - /// seeding RNG is not ready, it will retry later, after `threshold / 256` - /// generated bytes. On other errors in the source RNG, this will skip - /// reseeding and continue using the internal PRNG, until another - /// `threshold` bytes have been generated (at which point it will try - /// reseeding again). - #[inline(never)] + /// intelligently through some combination of retrying and delaying + /// reseeding until later. So long as the internal PRNG doesn't fail, this + /// method will not fail, i.e. failures from the reseeding source are not + /// fatal. pub fn reseed(&mut self) { - trace!("Reseeding RNG after generating {} bytes", - self.threshold - self.bytes_until_reseed); - self.bytes_until_reseed = self.threshold; - let mut err_count = 0; - loop { - if let Err(e) = R::from_rng(&mut self.reseeder) - .map(|result| self.rng = result) { - let kind = e.kind(); - if kind.should_wait() { - self.bytes_until_reseed = self.threshold >> 8; - warn!("Reseeding RNG delayed for {} bytes", - self.bytes_until_reseed); - } else if kind.should_retry() { - err_count += 1; - // Retry immediately for 5 times (arbitrary limit) - if err_count <= 5 { continue; } - } - warn!("Reseeding RNG failed; continuing without reseeding. Error: {}", e); - } - break; // Successfully reseeded, delayed, or given up. - } + // Behaviour is identical to `try_reseed`; we just squelch the error. + let _res = self.try_reseed(); } /// Reseed the internal RNG if the number of bytes that have been /// generated exceed the threshold. /// /// If reseeding fails, return an error with the original cause. Note that - /// if the cause has a permanent failure, we report a transient error and - /// skip reseeding; this means that only two error kinds can be reported - /// from this method: `ErrorKind::Transient` and `ErrorKind::NotReady`. + /// in case of error we simply delay reseeding, allowing the generator to + /// continue its output of random data and try reseeding again later; + /// because of this we always return kind `ErrorKind::Transient`. #[inline(never)] pub fn try_reseed(&mut self) -> Result<(), Error> { trace!("Reseeding RNG after {} generated bytes", self.threshold - self.bytes_until_reseed); - if let Err(err) = R::from_rng(&mut self.reseeder) - .map(|result| self.rng = result) { - let newkind = match err.kind() { - a @ ErrorKind::NotReady => a, - b @ ErrorKind::Transient => b, - _ => { - self.bytes_until_reseed = self.threshold; // skip reseeding - ErrorKind::Transient - } + if let Err(mut e) = R::from_rng(&mut self.reseeder) + .map(|result| self.rng = result) + { + let delay = match e.kind { + ErrorKind::Transient => 0, + kind @ _ if kind.should_retry() => self.threshold >> 8, + _ => self.threshold, }; - return Err(Error::with_cause(newkind, "reseeding failed", err)); + warn!("Reseeding RNG delayed reseeding by {} bytes due to \ + error from source: {}", delay, e); + self.bytes_until_reseed = delay; + e.kind = ErrorKind::Transient; + Err(e) + } else { + self.bytes_until_reseed = self.threshold; + Ok(()) } - self.bytes_until_reseed = self.threshold; - Ok(()) } } @@ -171,12 +151,21 @@ impl RngCore for ReseedingRng } fn try_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), Error> { - self.rng.try_fill_bytes(dest)?; + let res1 = self.rng.try_fill_bytes(dest); self.bytes_until_reseed -= dest.len() as i64; - if self.bytes_until_reseed <= 0 { - self.try_reseed()?; + let res2 = if self.bytes_until_reseed <= 0 { + self.try_reseed() + } else { Ok(()) }; + + if let Err(e) = res1 { + // In the unlikely event the internal PRNG fails, we don't know + // whether this is resolvable; reseed immediately and return + // original error kind. + self.bytes_until_reseed = 0; + Err(e) + } else { + res2 } - Ok(()) } } From 2b044275e42e4a6356cf0ee9dbfc9db96381d200 Mon Sep 17 00:00:00 2001 From: Diggory Hardy Date: Tue, 27 Feb 2018 09:56:21 +0000 Subject: [PATCH 2/2] Error handling: all IO errors other than Interrupted and WouldBlock are hard errors --- src/os.rs | 20 +++++++++++++------- src/read.rs | 11 +++++------ 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/src/os.rs b/src/os.rs index 459d531ca26..e6c61b71430 100644 --- a/src/os.rs +++ b/src/os.rs @@ -146,11 +146,11 @@ impl ReadRng { let file = File::open(path).map_err(|err| { use std::io::ErrorKind::*; match err.kind() { - NotFound | PermissionDenied | ConnectionRefused | BrokenPipe => - Error::with_cause(ErrorKind::Unavailable, - "permanent error while opening random device", err), - _ => Error::with_cause(ErrorKind::Unexpected, - "unexpected error while opening random device", err) + Interrupted => Error::new(ErrorKind::Transient, "interrupted"), + WouldBlock => Error::with_cause(ErrorKind::NotReady, + "opening random device would block", err), + _ => Error::with_cause(ErrorKind::Unavailable, + "error while opening random device", err) } })?; *guard = Some(file); @@ -170,7 +170,13 @@ impl ReadRng { let mut file = (*guard).as_mut().unwrap(); // Use `std::io::read_exact`, which retries on `ErrorKind::Interrupted`. file.read_exact(dest).map_err(|err| { - Error::with_cause(ErrorKind::Unexpected, "error reading random device", err) + match err.kind() { + ::std::io::ErrorKind::WouldBlock => Error::with_cause( + ErrorKind::NotReady, + "reading from random device would block", err), + _ => Error::with_cause(ErrorKind::Unavailable, + "error reading random device", err) + } }) } } @@ -251,7 +257,7 @@ mod imp { )); } else { return Err(Error::with_cause( - ErrorKind::Unexpected, + ErrorKind::Unavailable, "unexpected getrandom error", err, )); diff --git a/src/read.rs b/src/read.rs index 4b033163394..8bf6858991e 100644 --- a/src/read.rs +++ b/src/read.rs @@ -62,13 +62,12 @@ impl RngCore for ReadRng { if dest.len() == 0 { return Ok(()); } // Use `std::io::read_exact`, which retries on `ErrorKind::Interrupted`. self.reader.read_exact(dest).map_err(|err| { - use std::io::ErrorKind::*; match err.kind() { - NotFound | PermissionDenied | ConnectionRefused | BrokenPipe => - Error::with_cause(ErrorKind::Unavailable, - "ReadRng: resource unavailable", err), - _ => Error::with_cause(ErrorKind::Unexpected, - "ReadRng: unexpected error", err) + ::std::io::ErrorKind::WouldBlock => Error::with_cause( + ErrorKind::NotReady, + "reading from random device would block", err), + _ => Error::with_cause(ErrorKind::Unavailable, + "error reading random device", err) } }) }