Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add error handling to ReseedingRng #32

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 70 additions & 24 deletions src/reseeding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,21 @@
//! A wrapper around another RNG that reseeds it after it
//! generates a certain number of random bytes.

use core::fmt::Debug;

use {Rng, SeedableRng, Error};
use {Rng, SeedableRng, Error, ErrorKind};
#[cfg(feature="std")]
use NewSeeded;

/// How many bytes of entropy the underling RNG is allowed to generate
#[cfg(feature="std")]
use std::fmt;

/// How many bytes of entropy the underlying RNG is allowed to generate
/// before it is reseeded
const DEFAULT_GENERATION_THRESHOLD: u64 = 32 * 1024;

/// A wrapper around any RNG which reseeds the underlying RNG after it
/// has generated a certain number of random bytes.
#[derive(Debug)]
pub struct ReseedingRng<R, Rsdr: Debug> {
pub struct ReseedingRng<R, Rsdr> {
rng: R,
generation_threshold: u64,
bytes_generated: u64,
Expand All @@ -38,9 +39,12 @@ impl<R: Rng, Rsdr: Reseeder<R>> ReseedingRng<R, Rsdr> {
/// # Arguments
///
/// * `rng`: the random number generator to use.
/// * `generation_threshold`: the number of bytes of entropy at which to reseed the RNG.
/// * `generation_threshold`: the number of bytes of entropy at which to
/// reseed the RNG.
/// * `reseeder`: the reseeding object to use.
pub fn new(rng: R, generation_threshold: u64, reseeder: Rsdr) -> ReseedingRng<R,Rsdr> {
pub fn new(rng: R, generation_threshold: u64, reseeder: Rsdr)
-> ReseedingRng<R,Rsdr>
{
ReseedingRng {
rng: rng,
generation_threshold: generation_threshold,
Expand All @@ -51,43 +55,85 @@ impl<R: Rng, Rsdr: Reseeder<R>> ReseedingRng<R, Rsdr> {

/// Reseed the internal RNG if the number of bytes that have been
/// generated exceed the threshold.
pub fn reseed_if_necessary(&mut self) {
pub fn reseed_if_necessary(&mut self) -> Result<(), Error> {
if self.bytes_generated >= self.generation_threshold {
self.reseeder.reseed(&mut self.rng);
let _ = self.reseeder.reseed(&mut self.rng).map_err(map_err)?;
// Reset number of generated bytes, even if reseeding failes.
// This means we don't try to reseed again until a new
// `generation_threshold` amount of output is consumed.
self.bytes_generated = 0;
}
};
Ok(())
}
}

/// An error that can occur when reseeding failed.
#[cfg(feature="std")]
#[derive(Debug)]
pub struct ReseedingError {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced we need another error type (see #10).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative is to add a ReseedingFailed variant to the error kinds in rand_core.

It is just that I first explored this route (should have made a better PR message that said so).

cause: Box<::std::error::Error + Send + Sync>,
}

#[cfg(feature="std")]
impl fmt::Display for ReseedingError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "Failed to reseed RNG.")
}
}

#[cfg(feature="std")]
impl ::std::error::Error for ReseedingError {
fn description(&self) -> &str {
"reseeding failed"
}

fn cause(&self) -> Option<&::std::error::Error> {
Some(self.cause.as_ref())
}
}

#[cfg(feature="std")]
fn map_err<E>(_cause: E) -> Error
where E: Into<Box<::std::error::Error + Send + Sync>>
{
let cause = ReseedingError { cause: _cause.into() };
Error::new_err(ErrorKind::Other, cause)
}

#[cfg(not(feature="std"))]
fn map_err<E>(_cause: E) -> Error {
// We can't pass on the cause in `no_std` mode
Error::new_str(ErrorKind::Other, "reseeding failed")
}

impl<R: Rng, Rsdr: Reseeder<R>> Rng for ReseedingRng<R, Rsdr> {
fn next_u32(&mut self) -> u32 {
self.reseed_if_necessary();
let _ = self.reseed_if_necessary(); // continue if reseeding failed
self.bytes_generated += 4;
self.rng.next_u32()
}

fn next_u64(&mut self) -> u64 {
self.reseed_if_necessary();
let _ = self.reseed_if_necessary(); // continue if reseeding failed
self.bytes_generated += 8;
self.rng.next_u64()
}

#[cfg(feature = "i128_support")]
fn next_u128(&mut self) -> u128 {
self.reseed_if_necessary();
let _ = self.reseed_if_necessary(); // continue if reseeding failed
self.bytes_generated += 16;
self.rng.next_u128()
}

fn fill_bytes(&mut self, dest: &mut [u8]) {
self.reseed_if_necessary();
let _ = self.reseed_if_necessary(); // continue if reseeding failed
self.bytes_generated += dest.len() as u64;
self.rng.fill_bytes(dest);
}

fn try_fill(&mut self, dest: &mut [u8]) -> Result<(), Error> {
self.reseed_if_necessary();
self.reseed_if_necessary()?;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not keen on this difference of error handling between functions. Either the ReseedingRng should always allow continuation when reseeding fails (in which case this function should either silently continue or report ErrorKind::Transient and have some extra logic not to try reseeding too soon after a failure), or ReseedingRng should always fail if reseeding fails (with panic if necessary).

I'm not sure which; it could even be configurable but probably unnecessary. Perhaps allowing continuation after failure is the best option (users shouldn't rely on reseeding for security in any case).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, we can return an error here and fill dest. Then it is up to the calling code to decide what to do.

self.bytes_generated += dest.len() as u64;
self.rng.try_fill(dest)
}
Expand All @@ -109,9 +155,9 @@ impl<S, R: SeedableRng<S>, Rsdr: Reseeder<R>> SeedableRng<(Rsdr, S)> for
}

/// Something that can be used to reseed an RNG via `ReseedingRng`.
pub trait Reseeder<R: ?Sized>: Debug {
pub trait Reseeder<R: ?Sized> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why lose Debug?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this API guideline was appropriate. Besides I see no functional need for the Debug bound here.

/// Reseed the given RNG.
fn reseed(&mut self, rng: &mut R);
fn reseed(&mut self, rng: &mut R) -> Result<(), Error>;
}

/// Reseed an RNG using `NewSeeded` to replace the current instance.
Expand All @@ -121,12 +167,10 @@ pub struct ReseedWithNew;

#[cfg(feature="std")]
impl<R: Rng + NewSeeded> Reseeder<R> for ReseedWithNew {
fn reseed(&mut self, rng: &mut R) {
match R::new() {
Ok(result) => *rng = result,
// TODO: should we ignore and continue without reseeding?
Err(e) => panic!("Reseeding failed: {:?}", e),
}
fn reseed(&mut self, rng: &mut R) -> Result<(), Error> {
let result = R::new()?;
*rng = result;
Ok(())
}
}

Expand All @@ -137,12 +181,14 @@ mod test {
use {SeedableRng, Rng, iter};
use distributions::ascii_word_char;
use super::{ReseedingRng, Reseeder};
use Error;

#[derive(Debug)]
struct ReseedMock;
impl Reseeder<MockAddRng<u32>> for ReseedMock {
fn reseed(&mut self, rng: &mut MockAddRng<u32>) {
fn reseed(&mut self, rng: &mut MockAddRng<u32>) -> Result<(), Error> {
*rng = MockAddRng::new(0, 1);
Ok(())
}
}

Expand Down