From ec09f9eef2349bc72cc2fa40d0954c8334a9bf71 Mon Sep 17 00:00:00 2001 From: Niklas Adolfsson Date: Tue, 12 Feb 2019 01:15:15 +0100 Subject: [PATCH 1/3] fix(add helper timestamp overflows) --- Cargo.lock | 1 + ethcore/Cargo.toml | 1 + ethcore/src/error.rs | 3 ++ ethcore/src/verification/verification.rs | 42 ++++++++++++++++++++++-- 4 files changed, 44 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6e1f6bbc9cc..900484531e6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -719,6 +719,7 @@ dependencies = [ "kvdb-rocksdb 0.1.4 (registry+https://github.com/rust-lang/crates.io-index)", "lazy_static 1.2.0 (registry+https://github.com/rust-lang/crates.io-index)", "len-caching-lock 0.1.1", + "libc 0.2.48 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.5 (registry+https://github.com/rust-lang/crates.io-index)", "lru-cache 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)", "macros 0.1.0", diff --git a/ethcore/Cargo.toml b/ethcore/Cargo.toml index b6174984434..866e3406318 100644 --- a/ethcore/Cargo.toml +++ b/ethcore/Cargo.toml @@ -41,6 +41,7 @@ kvdb-memorydb = "0.1" kvdb-rocksdb = { version = "0.1.3", optional = true } lazy_static = "1.0" len-caching-lock = { path = "../util/len-caching-lock" } +libc = "0.2.48" log = "0.4" lru-cache = "0.1" macros = { path = "../util/macros" } diff --git a/ethcore/src/error.rs b/ethcore/src/error.rs index 2984dcdea88..32cfe057a0a 100644 --- a/ethcore/src/error.rs +++ b/ethcore/src/error.rs @@ -89,6 +89,8 @@ pub enum BlockError { InvalidNumber(Mismatch), /// Block number isn't sensible. RidiculousNumber(OutOfBounds), + /// Timestamp header overflowed + TimestampOverflow, /// Too many transactions from a particular address. TooManyTransactions(Address), /// Parent given is unknown. @@ -138,6 +140,7 @@ impl fmt::Display for BlockError { UnknownParent(ref hash) => format!("Unknown parent: {}", hash), UnknownUncleParent(ref hash) => format!("Unknown uncle parent: {}", hash), UnknownEpochTransition(ref num) => format!("Unknown transition to epoch number: {}", num), + TimestampOverflow => format!("Timestamp overflow"), TooManyTransactions(ref address) => format!("Too many transactions from: {}", address), }; diff --git a/ethcore/src/verification/verification.rs b/ethcore/src/verification/verification.rs index 827b7143913..bba6c287f01 100644 --- a/ethcore/src/verification/verification.rs +++ b/ethcore/src/verification/verification.rs @@ -40,6 +40,35 @@ use types::{BlockNumber, header::Header}; use types::transaction::SignedTransaction; use verification::queue::kind::blocks::Unverified; + +/// FIXME: @niklasad1 - remove this when and use `SystemTime::checked_add` +/// when https://github.com/rust-lang/rust/issues/55940 is stabilized +pub fn timestamp_checked_add(sys: SystemTime, d2: Duration) -> Result { + const NSEC_PER_SEC: u32 = 1_000_000_000; + let d1 = sys.duration_since(UNIX_EPOCH).map_err(|_| BlockError::TimestampOverflow)?; + + // dummy thing just to get correct size of the struct fields (to use libc's conditional compilation) + let t = libc::timespec { tv_sec: 0, tv_nsec: 0 }; + + let mut secs: u64 = d1.as_secs().checked_add(d2.as_secs()).ok_or(BlockError::TimestampOverflow)?; + // Nano calculations can't overflow because nanos are <1B which fit in a u32. + let mut nsecs = d1.subsec_nanos() + d2.subsec_nanos() as u32; + if nsecs >= NSEC_PER_SEC { + nsecs -= NSEC_PER_SEC; + secs = secs.checked_add(1).ok_or(BlockError::TimestampOverflow)?; + } + + // calculate max_value for signed value (2**n - 1) where n is the number of bits + let size_in_bits = std::mem::size_of_val(&t.tv_sec) as u32 * 8; + let max = (2_u64 << (size_in_bits - 2)) - 1; + + if secs <= max { + Ok(sys + d2) + } else { + Err(BlockError::TimestampOverflow) + } +} + /// Preprocessed block data gathered in `verify_block_unordered` call pub struct PreverifiedBlock { /// Populated block header @@ -306,7 +335,7 @@ pub fn verify_header_params(header: &Header, engine: &EthEngine, is_full: bool, const ACCEPTABLE_DRIFT: Duration = Duration::from_secs(15); let max_time = SystemTime::now() + ACCEPTABLE_DRIFT; let invalid_threshold = max_time + ACCEPTABLE_DRIFT * 9; - let timestamp = UNIX_EPOCH + Duration::from_secs(header.timestamp()); + let timestamp = timestamp_checked_add(UNIX_EPOCH, Duration::from_secs(header.timestamp()))?; if timestamp > invalid_threshold { return Err(From::from(BlockError::InvalidTimestamp(OutOfBounds { max: Some(max_time), min: None, found: timestamp }))) @@ -328,8 +357,8 @@ fn verify_parent(header: &Header, parent: &Header, engine: &EthEngine) -> Result let gas_limit_divisor = engine.params().gas_limit_bound_divisor; if !engine.is_timestamp_valid(header.timestamp(), parent.timestamp()) { - let min = SystemTime::now() + Duration::from_secs(parent.timestamp() + 1); - let found = SystemTime::now() + Duration::from_secs(header.timestamp()); + let min = timestamp_checked_add(SystemTime::now(), Duration::from_secs(parent.timestamp().saturating_add(1)))?; + let found = timestamp_checked_add(SystemTime::now(), Duration::from_secs(header.timestamp()))?; return Err(From::from(BlockError::InvalidTimestamp(OutOfBounds { max: None, min: Some(min), found }))) } if header.number() != parent.number() + 1 { @@ -815,4 +844,11 @@ mod tests { check_fail(unordered_test(&create_test_block_with_data(&header, &bad_transactions, &[]), &engine), TooManyTransactions(keypair.address())); unordered_test(&create_test_block_with_data(&header, &good_transactions, &[]), &engine).unwrap(); } + + #[test] + fn checked_add_systime_dur() { + assert!(timestamp_checked_add(UNIX_EPOCH, Duration::new(i64::max_value() as u64 + 1, 0)).is_err()); + assert!(timestamp_checked_add(UNIX_EPOCH, Duration::new(i64::max_value() as u64, 0)).is_ok()); + assert!(timestamp_checked_add(UNIX_EPOCH, Duration::new(i64::max_value() as u64 - 1, 1_000_000_000)).is_ok()); + } } From e9fe74c2e49ee9b5411c53890baca005e81a19de Mon Sep 17 00:00:00 2001 From: Niklas Adolfsson Date: Tue, 12 Feb 2019 10:29:38 +0100 Subject: [PATCH 2/3] fix(simplify code) --- Cargo.lock | 1 - ethcore/Cargo.toml | 1 - ethcore/src/verification/verification.rs | 37 +++++++++--------------- 3 files changed, 14 insertions(+), 25 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 900484531e6..6e1f6bbc9cc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -719,7 +719,6 @@ dependencies = [ "kvdb-rocksdb 0.1.4 (registry+https://github.com/rust-lang/crates.io-index)", "lazy_static 1.2.0 (registry+https://github.com/rust-lang/crates.io-index)", "len-caching-lock 0.1.1", - "libc 0.2.48 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.5 (registry+https://github.com/rust-lang/crates.io-index)", "lru-cache 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)", "macros 0.1.0", diff --git a/ethcore/Cargo.toml b/ethcore/Cargo.toml index 866e3406318..b6174984434 100644 --- a/ethcore/Cargo.toml +++ b/ethcore/Cargo.toml @@ -41,7 +41,6 @@ kvdb-memorydb = "0.1" kvdb-rocksdb = { version = "0.1.3", optional = true } lazy_static = "1.0" len-caching-lock = { path = "../util/len-caching-lock" } -libc = "0.2.48" log = "0.4" lru-cache = "0.1" macros = { path = "../util/macros" } diff --git a/ethcore/src/verification/verification.rs b/ethcore/src/verification/verification.rs index bba6c287f01..8f49eb59d2d 100644 --- a/ethcore/src/verification/verification.rs +++ b/ethcore/src/verification/verification.rs @@ -41,28 +41,18 @@ use types::transaction::SignedTransaction; use verification::queue::kind::blocks::Unverified; -/// FIXME: @niklasad1 - remove this when and use `SystemTime::checked_add` -/// when https://github.com/rust-lang/rust/issues/55940 is stabilized +/// Returns `Ok` when the result less or equal to `i32::max_value` to prevent `SystemTime` to panic because +/// it is platform specific, may be i32 or i64. +/// +/// `Err Result { - const NSEC_PER_SEC: u32 = 1_000_000_000; let d1 = sys.duration_since(UNIX_EPOCH).map_err(|_| BlockError::TimestampOverflow)?; + let total_time = d1.checked_add(d2).ok_or(BlockError::TimestampOverflow)?; - // dummy thing just to get correct size of the struct fields (to use libc's conditional compilation) - let t = libc::timespec { tv_sec: 0, tv_nsec: 0 }; - - let mut secs: u64 = d1.as_secs().checked_add(d2.as_secs()).ok_or(BlockError::TimestampOverflow)?; - // Nano calculations can't overflow because nanos are <1B which fit in a u32. - let mut nsecs = d1.subsec_nanos() + d2.subsec_nanos() as u32; - if nsecs >= NSEC_PER_SEC { - nsecs -= NSEC_PER_SEC; - secs = secs.checked_add(1).ok_or(BlockError::TimestampOverflow)?; - } - - // calculate max_value for signed value (2**n - 1) where n is the number of bits - let size_in_bits = std::mem::size_of_val(&t.tv_sec) as u32 * 8; - let max = (2_u64 << (size_in_bits - 2)) - 1; - - if secs <= max { + if total_time.as_secs() <= i32::max_value() as u64 { Ok(sys + d2) } else { Err(BlockError::TimestampOverflow) @@ -772,7 +762,8 @@ mod tests { check_fail_timestamp(family_test(&create_test_block_with_data(&header, &good_transactions, &good_uncles), engine, &bc), false); header = good.clone(); - header.set_timestamp(2450000000); + // will return `BlockError::TimestampOverflow` when timestamp > `i32::max_value()` + header.set_timestamp(i32::max_value() as u64); check_fail_timestamp(basic_test(&create_test_block_with_data(&header, &good_transactions, &good_uncles), engine), false); header = good.clone(); @@ -847,8 +838,8 @@ mod tests { #[test] fn checked_add_systime_dur() { - assert!(timestamp_checked_add(UNIX_EPOCH, Duration::new(i64::max_value() as u64 + 1, 0)).is_err()); - assert!(timestamp_checked_add(UNIX_EPOCH, Duration::new(i64::max_value() as u64, 0)).is_ok()); - assert!(timestamp_checked_add(UNIX_EPOCH, Duration::new(i64::max_value() as u64 - 1, 1_000_000_000)).is_ok()); + assert!(timestamp_checked_add(UNIX_EPOCH, Duration::new(i32::max_value() as u64 + 1, 0)).is_err()); + assert!(timestamp_checked_add(UNIX_EPOCH, Duration::new(i32::max_value() as u64, 0)).is_ok()); + assert!(timestamp_checked_add(UNIX_EPOCH, Duration::new(i32::max_value() as u64 - 1, 1_000_000_000)).is_ok()); } } From dbe14e84e8343aedb559bbeec08dbd5c7a0cb269 Mon Sep 17 00:00:00 2001 From: Niklas Adolfsson Date: Tue, 12 Feb 2019 13:07:04 +0100 Subject: [PATCH 3/3] fix(make helper private) --- ethcore/src/verification/verification.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethcore/src/verification/verification.rs b/ethcore/src/verification/verification.rs index 8f49eb59d2d..3f5008a2b86 100644 --- a/ethcore/src/verification/verification.rs +++ b/ethcore/src/verification/verification.rs @@ -48,7 +48,7 @@ use verification::queue::kind::blocks::Unverified; /// // FIXME: @niklasad1 - remove this when and use `SystemTime::checked_add` // when https://github.com/rust-lang/rust/issues/55940 is stabilized. -pub fn timestamp_checked_add(sys: SystemTime, d2: Duration) -> Result { +fn timestamp_checked_add(sys: SystemTime, d2: Duration) -> Result { let d1 = sys.duration_since(UNIX_EPOCH).map_err(|_| BlockError::TimestampOverflow)?; let total_time = d1.checked_add(d2).ok_or(BlockError::TimestampOverflow)?;