From 528980d037800da268c61c16dafb093eaf1784c1 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Mon, 31 Jan 2022 16:37:34 +0000 Subject: [PATCH] changes Shred::parent return type to Option (backport #21370) (#22844) * changes Shred::parent return type to Option (#21370) Shred::parent can return garbage if the struct fields are invalid: https://github.com/solana-labs/solana/blob/8a50b6302/ledger/src/shred.rs#L446-L453 The commit adds more sanity checks and changes the return type to Option. (cherry picked from commit dd338b6c9ff1f5f2975605e1c3a98a5bf5f1d435) # Conflicts: # ledger/src/shred.rs * removes mergify merge conflicts Co-authored-by: behzad nouri --- .../src/broadcast_stage/standard_broadcast_run.rs | 2 +- core/src/window_service.rs | 5 ++++- ledger/src/blockstore.rs | 8 ++++++-- ledger/src/shred.rs | 15 ++++++++------- 4 files changed, 19 insertions(+), 11 deletions(-) diff --git a/core/src/broadcast_stage/standard_broadcast_run.rs b/core/src/broadcast_stage/standard_broadcast_run.rs index 7118a64054..c8de92c7ea 100644 --- a/core/src/broadcast_stage/standard_broadcast_run.rs +++ b/core/src/broadcast_stage/standard_broadcast_run.rs @@ -598,7 +598,7 @@ mod test { .expect("Expected a shred that signals an interrupt"); // Validate the shred - assert_eq!(shred.parent(), parent); + assert_eq!(shred.parent(), Some(parent)); assert_eq!(shred.slot(), slot); assert_eq!(shred.index(), next_shred_index); assert!(shred.is_data()); diff --git a/core/src/window_service.rs b/core/src/window_service.rs index 5308055646..f331949a84 100644 --- a/core/src/window_service.rs +++ b/core/src/window_service.rs @@ -163,7 +163,10 @@ impl ReceiveWindowStats { fn verify_shred_slot(shred: &Shred, root: u64) -> bool { match shred.shred_type() { // Only data shreds have parent information - ShredType::Data => blockstore::verify_shred_slots(shred.slot(), shred.parent(), root), + ShredType::Data => match shred.parent() { + Some(parent) => blockstore::verify_shred_slots(shred.slot(), parent, root), + None => false, + }, // Filter out outdated coding shreds ShredType::Code => shred.slot() >= root, } diff --git a/ledger/src/blockstore.rs b/ledger/src/blockstore.rs index fffbab9330..7574480d6c 100644 --- a/ledger/src/blockstore.rs +++ b/ledger/src/blockstore.rs @@ -1231,8 +1231,12 @@ impl Blockstore { get_index_meta_entry(&self.db, slot, index_working_set, index_meta_time); let index_meta = &mut index_meta_working_set_entry.index; - let slot_meta_entry = - get_slot_meta_entry(&self.db, slot_meta_working_set, slot, shred.parent()); + let slot_meta_entry = get_slot_meta_entry( + &self.db, + slot_meta_working_set, + slot, + shred.parent().ok_or(InsertDataShredError::InvalidShred)?, + ); let slot_meta = &mut slot_meta_entry.new_slot_meta.borrow_mut(); diff --git a/ledger/src/shred.rs b/ledger/src/shred.rs index 1823ac0a02..6a4998f3f5 100644 --- a/ledger/src/shred.rs +++ b/ledger/src/shred.rs @@ -457,12 +457,13 @@ impl Shred { self.common_header.slot } - // TODO: This should return Option - pub fn parent(&self) -> Slot { - if self.is_data() { - self.common_header.slot - u64::from(self.data_header.parent_offset) - } else { - std::u64::MAX + pub fn parent(&self) -> Option { + match self.shred_type() { + ShredType::Data => { + let parent_offset = Slot::try_from(self.data_header.parent_offset); + self.slot().checked_sub(parent_offset.ok()?) + } + ShredType::Code => None, } } @@ -1125,7 +1126,7 @@ pub fn verify_test_data_shred( assert!(shred.is_data()); assert_eq!(shred.index(), index); assert_eq!(shred.slot(), slot); - assert_eq!(shred.parent(), parent); + assert_eq!(shred.parent(), Some(parent)); assert_eq!(verify, shred.verify(pk)); if is_last_in_slot { assert!(shred.last_in_slot());