Merge pull request openethereum#65 from niklasad1/checked-systemtime

prevent SystemTime panics

Author: soc1c

ethcore/src/engines/clique/block_state.rs

@@ -28,6 +28,9 @@ use types::BlockNumber;
 use types::header::Header;
 use unexpected::Mismatch;
 
+#[cfg(not(feature = "time_checked_add"))]
+use time_utils::CheckedSystemTime;
+
 /// Type that keeps track of the state for a given vote
 // Votes that go against the proposal aren't counted since it's equivalent to not voting
 #[derive(Copy, Clone, Debug, PartialEq, PartialOrd)]
@@ -259,29 +262,44 @@ impl CliqueBlockState {
 		Ok(())
 	}
 
-	pub fn calc_next_timestamp(&mut self, header: &Header, period: u64) {
-		let base_time = UNIX_EPOCH + Duration::from_secs(header.timestamp());
+	/// Calculate the next timestamp for `inturn` and `noturn` fails if any of them can't be represented as
+	/// `SystemTime`
+	// TODO(niklasad1): refactor this method to be in constructor of `CliqueBlockState` instead.
+	// This is a quite bad API because we must mutate both variables even when already `inturn` fails
+	// That's why we can't return early and must have the `if-else` in the end
+	pub fn calc_next_timestamp(&mut self, timestamp: u64, period: u64) -> Result<(), Error> {
+		let inturn = UNIX_EPOCH.checked_add(Duration::from_secs(timestamp.saturating_add(period)));
 
-		self.next_timestamp_inturn = Some(base_time + Duration::from_secs(period));
+		self.next_timestamp_inturn = inturn;
 
 		let delay = Duration::from_millis(
 			rand::thread_rng().gen_range(0u64, (self.signers.len() as u64 / 2 + 1) * SIGNING_DELAY_NOTURN_MS));
-		self.next_timestamp_noturn = Some(base_time + Duration::from_secs(period) + delay);
+		self.next_timestamp_noturn = inturn.map(|inturn|  {
+			inturn + delay
+		});
+
+		if self.next_timestamp_inturn.is_some() && self.next_timestamp_noturn.is_some() {
+			Ok(())
+		} else {
+			Err(BlockError::TimestampOverflow)?
+		}
 	}
 
+	/// Returns true if the block difficulty should be `inturn`
 	pub fn is_inturn(&self, current_block_number: u64, author: &Address) -> bool {
 		if let Some(pos) = self.signers.iter().position(|x| *author == *x) {
 			return current_block_number % self.signers.len() as u64 == pos as u64;
 		}
 		false
 	}
 
+	/// Returns whether the signer is authorized to sign a block
 	pub fn is_authorized(&self, author: &Address) -> bool {
 		self.signers.contains(author) && !self.recent_signers.contains(author)
 	}
 
-	// Returns whether it makes sense to cast the specified vote in the
-	// current state (e.g. don't try to add an already authorized signer).
+	/// Returns whether it makes sense to cast the specified vote in the
+	/// current state (e.g. don't try to add an already authorized signer).
 	pub fn is_valid_vote(&self, address: &Address, vote_type: VoteType) -> bool {
 		let in_signer = self.signers.contains(address);
 		match vote_type {
@@ -290,11 +308,12 @@ impl CliqueBlockState {
 		}
 	}
 
+	/// Returns the list of current signers
 	pub fn signers(&self) -> &BTreeSet<Address> {
 		&self.signers
 	}
 
-	// Note this method will always return `true` but it is intended for a unifrom `API`
+	// Note this method will always return `true` but it is intended for a uniform `API`
 	fn add_vote(&mut self, pending_vote: PendingVote, kind: VoteType) -> bool {
 
 		self.votes.entry(pending_vote)

ethcore/src/engines/clique/mod.rs

@@ -79,6 +79,9 @@ use unexpected::{Mismatch, OutOfBounds};
 use types::BlockNumber;
 use types::header::{ExtendedHeader, Header};
 
+#[cfg(not(feature = "time_checked_add"))]
+use time_utils::CheckedSystemTime;
+
 use self::block_state::CliqueBlockState;
 use self::params::CliqueParams;
 use self::step_service::StepService;
@@ -241,7 +244,8 @@ impl Clique {
 		let mut state = CliqueBlockState::new(
 			extract_signers(header)?);
 
-		state.calc_next_timestamp(header, self.period);
+		// TODO(niklasad1): refactor to perform this check in the `CliqueBlockState` constructor instead
+		state.calc_next_timestamp(header.timestamp(), self.period)?;
 
 		Ok(state)
 	}
@@ -326,7 +330,7 @@ impl Clique {
 				for item in chain {
 					new_state.apply(item, false)?;
 				}
-				new_state.calc_next_timestamp(header, self.period);
+				new_state.calc_next_timestamp(header.timestamp(), self.period)?;
 				block_state_by_hash.insert(header.hash(), new_state.clone());
 
 				let elapsed = backfill_start.elapsed();
@@ -435,7 +439,7 @@ impl Engine<EthereumMachine> for Clique {
 		// locally sealed block don't go through valid_block_family(), so we have to record state here.
 		let mut new_state = state.clone();
 		new_state.apply(&header, is_checkpoint)?;
-		new_state.calc_next_timestamp(&header, self.period);
+		new_state.calc_next_timestamp(header.timestamp(), self.period)?;
 		self.block_state_by_hash.write().insert(header.hash(), new_state);
 
 		trace!(target: "engine", "on_seal_block: finished, final header: {:?}", header);
@@ -531,15 +535,23 @@ impl Engine<EthereumMachine> for Clique {
 
 		// Don't waste time checking blocks from the future
 		{
-			// TODO(niklasad): checked_add
-			let limit = SystemTime::now().duration_since(UNIX_EPOCH).unwrap_or_default() + Duration::from_secs(self.period);
+			let limit = SystemTime::now().checked_add(Duration::from_secs(self.period))
+				.ok_or(BlockError::TimestampOverflow)?;
+
+			// This should succeed under the contraints that the system clock works
+			let limit_as_dur = limit.duration_since(UNIX_EPOCH).map_err(|e| {
+				Box::new(format!("Converting SystemTime to Duration failed: {}", e))
+			})?;
+
 			let hdr = Duration::from_secs(header.timestamp());
-			if hdr > limit {
-				return Err(BlockError::TemporarilyInvalid(OutOfBounds {
+			if hdr > limit_as_dur {
+				let found = UNIX_EPOCH.checked_add(hdr).ok_or(BlockError::TimestampOverflow)?;
+
+				Err(BlockError::TemporarilyInvalid(OutOfBounds {
 					min: None,
-					max: Some(UNIX_EPOCH + limit),
-					found: UNIX_EPOCH + Duration::from_secs(header.timestamp()),
-				}))?;
+					max: Some(limit),
+					found,
+				}))?
 			}
 		}
 
@@ -642,11 +654,16 @@ impl Engine<EthereumMachine> for Clique {
 		}
 
 		// Ensure that the block's timestamp isn't too close to it's parent
-		if parent.timestamp().saturating_add(self.period) > header.timestamp() {
+		let limit = parent.timestamp().saturating_add(self.period);
+		if limit > header.timestamp() {
+			let max = UNIX_EPOCH.checked_add(Duration::from_secs(header.timestamp()));
+			let found = UNIX_EPOCH.checked_add(Duration::from_secs(limit))
+				.ok_or(BlockError::TimestampOverflow)?;
+
 			Err(BlockError::InvalidTimestamp(OutOfBounds {
-				min: Some(UNIX_EPOCH + Duration::from_secs(parent.timestamp().saturating_add(self.period))),
-				max: None,
-				found: UNIX_EPOCH + Duration::from_secs(header.timestamp())
+				min: None,
+				max,
+				found,
 			}))?
 		}
 
@@ -655,15 +672,15 @@ impl Engine<EthereumMachine> for Clique {
 		// Try to apply current state, apply() will further check signer and recent signer.
 		let mut new_state = parent_state.clone();
 		new_state.apply(header, header.number() % self.epoch_length == 0)?;
-		new_state.calc_next_timestamp(header, self.period);
+		new_state.calc_next_timestamp(header.timestamp(), self.period)?;
 		self.block_state_by_hash.write().insert(header.hash(), new_state);
 
 		Ok(())
 	}
 
 	fn genesis_epoch_data(&self, header: &Header, _call: &Call) -> Result<Vec<u8>, String> {
 		let mut state = self.new_checkpoint_state(header).expect("Unable to parse genesis data.");
-		state.calc_next_timestamp(header, self.period);
+		state.calc_next_timestamp(header.timestamp(), self.period).map_err(|e| format!("{}", e))?;
 		self.block_state_by_hash.write().insert(header.hash(), state);
 
 		// no proof.