From 5bc57ea0043eca79e16c73e1891eb2fa39dad20e Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Thu, 30 Apr 2020 14:35:18 -0700 Subject: [PATCH] Nits for sanitize trait (bp #9741) (#9809) * thiserror, docs, remove general Failure case (#9741) automerge (cherry picked from commit a0514eb2ae4bff2a0e543b379a1ec2d56fe126d0) # Conflicts: # core/src/crds_value.rs # core/src/epoch_slots.rs # sdk/src/sanitize.rs * rebase Co-authored-by: anatoly yakovenko Co-authored-by: Michael Vines --- core/src/cluster_info.rs | 4 ++-- core/src/contact_info.rs | 10 +++++++++- core/src/crds_value.rs | 22 +++++++++++----------- sdk/src/hash.rs | 3 +++ sdk/src/instruction.rs | 3 +++ sdk/src/message.rs | 3 +++ sdk/src/sanitize.rs | 18 +++++++++++++++--- 7 files changed, 46 insertions(+), 17 deletions(-) diff --git a/core/src/cluster_info.rs b/core/src/cluster_info.rs index 7c387d3815..4305020982 100644 --- a/core/src/cluster_info.rs +++ b/core/src/cluster_info.rs @@ -157,7 +157,7 @@ pub struct PruneData { impl Sanitize for PruneData { fn sanitize(&self) -> std::result::Result<(), SanitizeError> { if self.wallclock >= MAX_WALLCLOCK { - return Err(SanitizeError::ValueOutOfRange); + return Err(SanitizeError::ValueOutOfBounds); } Ok(()) } @@ -2641,7 +2641,7 @@ mod tests { let mut pd = PruneData::default(); pd.wallclock = MAX_WALLCLOCK; let msg = Protocol::PruneMessage(Pubkey::default(), pd); - assert_eq!(msg.sanitize(), Err(SanitizeError::ValueOutOfRange)); + assert_eq!(msg.sanitize(), Err(SanitizeError::ValueOutOfBounds)); } // computes the maximum size for pull request blooms diff --git a/core/src/contact_info.rs b/core/src/contact_info.rs index d77ba52d80..93fe68333a 100644 --- a/core/src/contact_info.rs +++ b/core/src/contact_info.rs @@ -42,7 +42,7 @@ pub struct ContactInfo { impl Sanitize for ContactInfo { fn sanitize(&self) -> std::result::Result<(), SanitizeError> { if self.wallclock >= MAX_WALLCLOCK { - return Err(SanitizeError::Failed); + return Err(SanitizeError::ValueOutOfBounds); } Ok(()) } @@ -325,4 +325,12 @@ mod tests { ci.rpc = socketaddr!("127.0.0.1:234"); assert!(ci.valid_client_facing_addr().is_some()); } + + #[test] + fn test_sanitize() { + let mut ci = ContactInfo::default(); + assert_eq!(ci.sanitize(), Ok(())); + ci.wallclock = MAX_WALLCLOCK; + assert_eq!(ci.sanitize(), Err(SanitizeError::ValueOutOfBounds)); + } } diff --git a/core/src/crds_value.rs b/core/src/crds_value.rs index 83af0fb0b1..1903ed5337 100644 --- a/core/src/crds_value.rs +++ b/core/src/crds_value.rs @@ -96,7 +96,7 @@ pub struct EpochIncompleteSlots { impl Sanitize for EpochIncompleteSlots { fn sanitize(&self) -> Result<(), SanitizeError> { if self.first >= MAX_SLOT { - return Err(SanitizeError::Failed); + return Err(SanitizeError::InvalidValue); } //rest of the data doesn't matter since we no longer decompress //these values @@ -110,7 +110,7 @@ impl Sanitize for CrdsData { CrdsData::ContactInfo(val) => val.sanitize(), CrdsData::Vote(ix, val) => { if *ix >= MAX_VOTES { - return Err(SanitizeError::Failed); + return Err(SanitizeError::ValueOutOfBounds); } val.sanitize() } @@ -118,7 +118,7 @@ impl Sanitize for CrdsData { CrdsData::AccountsHashes(val) => val.sanitize(), CrdsData::EpochSlots(ix, val) => { if *ix as usize >= MAX_EPOCH_SLOTS as usize { - return Err(SanitizeError::Failed); + return Err(SanitizeError::ValueOutOfBounds); } val.sanitize() } @@ -136,11 +136,11 @@ pub struct SnapshotHash { impl Sanitize for SnapshotHash { fn sanitize(&self) -> Result<(), SanitizeError> { if self.wallclock >= MAX_WALLCLOCK { - return Err(SanitizeError::Failed); + return Err(SanitizeError::ValueOutOfBounds); } for (slot, _) in &self.hashes { if *slot >= MAX_SLOT { - return Err(SanitizeError::Failed); + return Err(SanitizeError::ValueOutOfBounds); } } self.from.sanitize() @@ -183,17 +183,17 @@ impl EpochSlots { impl Sanitize for EpochSlots { fn sanitize(&self) -> Result<(), SanitizeError> { if self.wallclock >= MAX_WALLCLOCK { - return Err(SanitizeError::Failed); + return Err(SanitizeError::ValueOutOfBounds); } if self.lowest >= MAX_SLOT { - return Err(SanitizeError::Failed); + return Err(SanitizeError::ValueOutOfBounds); } if self.root >= MAX_SLOT { - return Err(SanitizeError::Failed); + return Err(SanitizeError::ValueOutOfBounds); } for slot in &self.slots { if *slot >= MAX_SLOT { - return Err(SanitizeError::Failed); + return Err(SanitizeError::ValueOutOfBounds); } } self.stash.sanitize()?; @@ -211,7 +211,7 @@ pub struct Vote { impl Sanitize for Vote { fn sanitize(&self) -> Result<(), SanitizeError> { if self.wallclock >= MAX_WALLCLOCK { - return Err(SanitizeError::Failed); + return Err(SanitizeError::ValueOutOfBounds); } self.from.sanitize()?; self.transaction.sanitize() @@ -484,7 +484,7 @@ mod test { ), &keypair, ); - assert!(item.sanitize().is_err()); + assert_eq!(item.sanitize(), Err(SanitizeError::ValueOutOfBounds)); } #[test] fn test_compute_vote_index_empty() { diff --git a/sdk/src/hash.rs b/sdk/src/hash.rs index 45f6f333e8..c255d61768 100644 --- a/sdk/src/hash.rs +++ b/sdk/src/hash.rs @@ -1,5 +1,6 @@ //! The `hash` module provides functions for creating SHA-256 hashes. +use crate::sanitize::Sanitize; use sha2::{Digest, Sha256}; use std::{convert::TryFrom, fmt, mem, str::FromStr}; use thiserror::Error; @@ -30,6 +31,8 @@ impl Hasher { } } +impl Sanitize for Hash {} + impl AsRef<[u8]> for Hash { fn as_ref(&self) -> &[u8] { &self.0[..] diff --git a/sdk/src/instruction.rs b/sdk/src/instruction.rs index 423b3d13fd..9b810ab322 100644 --- a/sdk/src/instruction.rs +++ b/sdk/src/instruction.rs @@ -1,5 +1,6 @@ //! Defines a composable Instruction type and a memory-efficient CompiledInstruction. +use crate::sanitize::Sanitize; use crate::{pubkey::Pubkey, short_vec, system_instruction::SystemError}; use bincode::serialize; use serde::Serialize; @@ -216,6 +217,8 @@ pub struct CompiledInstruction { pub data: Vec, } +impl Sanitize for CompiledInstruction {} + impl CompiledInstruction { pub fn new(program_ids_index: u8, data: &T, accounts: Vec) -> Self { let data = serialize(data).unwrap(); diff --git a/sdk/src/message.rs b/sdk/src/message.rs index ea5b9d422e..4fafad149d 100644 --- a/sdk/src/message.rs +++ b/sdk/src/message.rs @@ -184,6 +184,9 @@ impl Sanitize for Message { } } } + self.account_keys.sanitize()?; + self.recent_blockhash.sanitize()?; + self.instructions.sanitize()?; Ok(()) } } diff --git a/sdk/src/sanitize.rs b/sdk/src/sanitize.rs index 89681d05b5..a564dc9f8f 100644 --- a/sdk/src/sanitize.rs +++ b/sdk/src/sanitize.rs @@ -1,10 +1,22 @@ -#[derive(PartialEq, Debug)] +use thiserror::Error; + +#[derive(PartialEq, Debug, Error, Eq, Clone)] pub enum SanitizeError { - Failed, + #[error("index out of bounds")] IndexOutOfBounds, - ValueOutOfRange, + #[error("value out of bounds")] + ValueOutOfBounds, + #[error("invalid value")] + InvalidValue, } +/// Trait for sanitizing values and members of over the wire messages. +/// Implementation should recursively decent through the data structure +/// and sanitize all struct members and enum clauses. Sanitize excludes +/// signature verification checks, those are handled by another pass. +/// Sanitize checks should include but are not limited too: +/// * All index values are in range +/// * All values are within their static max/min bounds pub trait Sanitize { fn sanitize(&self) -> Result<(), SanitizeError> { Ok(())