From 5fe1a483612f24aaff24acbd6804761f4ea675b3 Mon Sep 17 00:00:00 2001 From: SW van Heerden Date: Fri, 15 Sep 2023 15:32:05 +0200 Subject: [PATCH] fixes potential index out of bounds --- .../src/base_node/sync/block_sync/error.rs | 4 ++++ .../base_node/sync/block_sync/synchronizer.rs | 22 +++++++++++------- .../src/base_node/sync/header_sync/error.rs | 3 +++ .../sync/header_sync/synchronizer.rs | 17 ++++++++++---- .../sync/horizon_state_sync/error.rs | 3 +++ .../sync/horizon_state_sync/synchronizer.rs | 23 +++++++++++-------- 6 files changed, 50 insertions(+), 22 deletions(-) diff --git a/base_layer/core/src/base_node/sync/block_sync/error.rs b/base_layer/core/src/base_node/sync/block_sync/error.rs index 39fe41a793..ad224fdf19 100644 --- a/base_layer/core/src/base_node/sync/block_sync/error.rs +++ b/base_layer/core/src/base_node/sync/block_sync/error.rs @@ -67,6 +67,8 @@ pub enum BlockSyncError { FixedHashSizeError(#[from] FixedHashSizeError), #[error("This sync round failed")] SyncRoundFailed, + #[error("Could not find peer info")] + PeerNotFound, } impl BlockSyncError { @@ -90,6 +92,7 @@ impl BlockSyncError { BlockSyncError::AllSyncPeersExceedLatency => "AllSyncPeersExceedLatency", BlockSyncError::FixedHashSizeError(_) => "FixedHashSizeError", BlockSyncError::SyncRoundFailed => "SyncRoundFailed", + BlockSyncError::PeerNotFound => "PeerNotFound", } } } @@ -106,6 +109,7 @@ impl BlockSyncError { BlockSyncError::NoMoreSyncPeers(_) | BlockSyncError::AllSyncPeersExceedLatency | BlockSyncError::FailedToConstructChainBlock | + BlockSyncError::PeerNotFound | BlockSyncError::SyncRoundFailed => None, // short ban diff --git a/base_layer/core/src/base_node/sync/block_sync/synchronizer.rs b/base_layer/core/src/base_node/sync/block_sync/synchronizer.rs index 30fd8f7465..316d1a211f 100644 --- a/base_layer/core/src/base_node/sync/block_sync/synchronizer.rs +++ b/base_layer/core/src/base_node/sync/block_sync/synchronizer.rs @@ -143,8 +143,9 @@ impl<'a, B: BlockchainBackend + 'static> BlockSynchronizer<'a, B> { sync_peer_node_ids.len() ); let mut latency_counter = 0usize; - for (i, node_id) in sync_peer_node_ids.iter().enumerate() { - let sync_peer = &self.sync_peers[i]; + for node_id in sync_peer_node_ids { + let peer_index = self.get_sync_peer_index(&node_id).ok_or(BlockSyncError::PeerNotFound)?; + let sync_peer = &self.sync_peers[peer_index]; self.hooks.call_on_starting_hook(sync_peer); let mut conn = match self.connect_to_sync_peer(node_id.clone()).await { Ok(val) => val, @@ -153,7 +154,7 @@ impl<'a, B: BlockchainBackend + 'static> BlockSynchronizer<'a, B> { target: LOG_TARGET, "Failed to connect to sync peer `{}`: {}", node_id, e ); - self.remove_sync_peer(node_id); + self.remove_sync_peer(&node_id); continue; }, }; @@ -170,15 +171,15 @@ impl<'a, B: BlockchainBackend + 'static> BlockSynchronizer<'a, B> { target: LOG_TARGET, "Failed to obtain RPC connection from sync peer `{}`: {}", node_id, e ); - self.remove_sync_peer(node_id); + self.remove_sync_peer(&node_id); continue; }, }; let latency = client .get_last_request_latency() .expect("unreachable panic: last request latency must be set after connect"); - self.sync_peers[i].set_latency(latency); - let sync_peer = self.sync_peers[i].clone(); + self.sync_peers[peer_index].set_latency(latency); + let sync_peer = self.sync_peers[peer_index].clone(); info!( target: LOG_TARGET, "Attempting to synchronize blocks with `{}` latency: {:.2?}", node_id, latency @@ -192,13 +193,13 @@ impl<'a, B: BlockchainBackend + 'static> BlockSynchronizer<'a, B> { if let Some(reason) = ban_reason { warn!(target: LOG_TARGET, "{}", err); self.peer_ban_manager - .ban_peer_if_required(node_id, &Some(reason.clone())) + .ban_peer_if_required(&node_id, &Some(reason.clone())) .await; } if let BlockSyncError::MaxLatencyExceeded { .. } = err { latency_counter += 1; } else { - self.remove_sync_peer(node_id); + self.remove_sync_peer(&node_id); } }, } @@ -423,4 +424,9 @@ impl<'a, B: BlockchainBackend + 'static> BlockSynchronizer<'a, B> { self.sync_peers.remove(pos); } } + + // Helper function to get the index to the node_id inside of the vec of peers + fn get_sync_peer_index(&mut self, node_id: &NodeId) -> Option { + self.sync_peers.iter().position(|p| p.node_id() == node_id) + } } diff --git a/base_layer/core/src/base_node/sync/header_sync/error.rs b/base_layer/core/src/base_node/sync/header_sync/error.rs index e95e345838..998944f2c6 100644 --- a/base_layer/core/src/base_node/sync/header_sync/error.rs +++ b/base_layer/core/src/base_node/sync/header_sync/error.rs @@ -34,6 +34,8 @@ use crate::{blocks::BlockError, chain_storage::ChainStorageError, common::BanRea pub enum BlockHeaderSyncError { #[error("No more sync peers available: {0}")] NoMoreSyncPeers(String), + #[error("Could not find peer info")] + PeerNotFound, #[error("RPC error: {0}")] RpcError(#[from] RpcError), #[error("RPC request failed: {0}")] @@ -103,6 +105,7 @@ impl BlockHeaderSyncError { BlockHeaderSyncError::AllSyncPeersExceedLatency | BlockHeaderSyncError::ConnectivityError(_) | BlockHeaderSyncError::NotInSync | + BlockHeaderSyncError::PeerNotFound | BlockHeaderSyncError::ChainStorageError(_) => None, // short ban diff --git a/base_layer/core/src/base_node/sync/header_sync/synchronizer.rs b/base_layer/core/src/base_node/sync/header_sync/synchronizer.rs index 61a5b20fd7..260b0654f5 100644 --- a/base_layer/core/src/base_node/sync/header_sync/synchronizer.rs +++ b/base_layer/core/src/base_node/sync/header_sync/synchronizer.rs @@ -144,8 +144,8 @@ impl<'a, B: BlockchainBackend + 'static> HeaderSynchronizer<'a, B> { sync_peer_node_ids.len() ); let mut latency_counter = 0usize; - for (i, node_id) in sync_peer_node_ids.iter().enumerate() { - match self.connect_and_attempt_sync(i, node_id, max_latency).await { + for node_id in sync_peer_node_ids { + match self.connect_and_attempt_sync(&node_id, max_latency).await { Ok(peer) => return Ok(peer), Err(err) => { let ban_reason = BlockHeaderSyncError::get_ban_reason( @@ -156,13 +156,13 @@ impl<'a, B: BlockchainBackend + 'static> HeaderSynchronizer<'a, B> { if let Some(reason) = ban_reason { warn!(target: LOG_TARGET, "{}", err); self.peer_ban_manager - .ban_peer_if_required(node_id, &Some(reason.clone())) + .ban_peer_if_required(&node_id, &Some(reason.clone())) .await; } if let BlockHeaderSyncError::MaxLatencyExceeded { .. } = err { latency_counter += 1; } else { - self.remove_sync_peer(node_id); + self.remove_sync_peer(&node_id); } }, } @@ -179,10 +179,12 @@ impl<'a, B: BlockchainBackend + 'static> HeaderSynchronizer<'a, B> { async fn connect_and_attempt_sync( &mut self, - peer_index: usize, node_id: &NodeId, max_latency: Duration, ) -> Result { + let peer_index = self + .get_sync_peer_index(node_id) + .ok_or(BlockHeaderSyncError::PeerNotFound)?; let sync_peer = &self.sync_peers[peer_index]; self.hooks.call_on_starting_hook(sync_peer); @@ -774,6 +776,11 @@ impl<'a, B: BlockchainBackend + 'static> HeaderSynchronizer<'a, B> { self.sync_peers.remove(pos); } } + + // Helper function to get the index to the node_id inside of the vec of peers + fn get_sync_peer_index(&mut self, node_id: &NodeId) -> Option { + self.sync_peers.iter().position(|p| p.node_id() == node_id) + } } struct ChainSplitInfo { diff --git a/base_layer/core/src/base_node/sync/horizon_state_sync/error.rs b/base_layer/core/src/base_node/sync/horizon_state_sync/error.rs index 33e661353e..d0fa0dfe7a 100644 --- a/base_layer/core/src/base_node/sync/horizon_state_sync/error.rs +++ b/base_layer/core/src/base_node/sync/horizon_state_sync/error.rs @@ -91,6 +91,8 @@ pub enum HorizonSyncError { FixedHashSizeError(#[from] FixedHashSizeError), #[error("No more sync peers available: {0}")] NoMoreSyncPeers(String), + #[error("Could not find peer info")] + PeerNotFound, } impl From for HorizonSyncError { @@ -117,6 +119,7 @@ impl HorizonSyncError { HorizonSyncError::RpcError(_) | HorizonSyncError::RpcStatus(_) | HorizonSyncError::NoMoreSyncPeers(_) | + HorizonSyncError::PeerNotFound | HorizonSyncError::JoinError(_) => None, // short ban diff --git a/base_layer/core/src/base_node/sync/horizon_state_sync/synchronizer.rs b/base_layer/core/src/base_node/sync/horizon_state_sync/synchronizer.rs index 5694ec3c1a..bd2c79d72f 100644 --- a/base_layer/core/src/base_node/sync/horizon_state_sync/synchronizer.rs +++ b/base_layer/core/src/base_node/sync/horizon_state_sync/synchronizer.rs @@ -193,8 +193,8 @@ impl<'a, B: BlockchainBackend + 'static> HorizonStateSynchronization<'a, B> { sync_peer_node_ids.len() ); let mut latency_counter = 0usize; - for (i, node_id) in sync_peer_node_ids.iter().enumerate() { - match self.connect_and_attempt_sync(i, node_id, header).await { + for node_id in sync_peer_node_ids { + match self.connect_and_attempt_sync(&node_id, header).await { Ok(_) => return Ok(()), // Try another peer Err(err) => { @@ -204,13 +204,13 @@ impl<'a, B: BlockchainBackend + 'static> HorizonStateSynchronization<'a, B> { if let Some(reason) = ban_reason { warn!(target: LOG_TARGET, "{}", err); self.peer_ban_manager - .ban_peer_if_required(node_id, &Some(reason.clone())) + .ban_peer_if_required(&node_id, &Some(reason.clone())) .await; } if let HorizonSyncError::MaxLatencyExceeded { .. } = err { latency_counter += 1; } else { - self.remove_sync_peer(node_id); + self.remove_sync_peer(&node_id); } }, } @@ -227,14 +227,14 @@ impl<'a, B: BlockchainBackend + 'static> HorizonStateSynchronization<'a, B> { async fn connect_and_attempt_sync( &mut self, - peer_index: usize, node_id: &NodeId, header: &BlockHeader, ) -> Result<(), HorizonSyncError> { - { - let sync_peer = &self.sync_peers[peer_index]; - self.hooks.call_on_starting_hook(sync_peer); - } + let peer_index = self + .get_sync_peer_index(node_id) + .ok_or(HorizonSyncError::PeerNotFound)?; + let sync_peer = &self.sync_peers[peer_index]; + self.hooks.call_on_starting_hook(sync_peer); let mut conn = self.dial_sync_peer(node_id).await?; debug!( @@ -985,6 +985,11 @@ impl<'a, B: BlockchainBackend + 'static> HorizonStateSynchronization<'a, B> { } } + // Helper function to get the index to the node_id inside of the vec of peers + fn get_sync_peer_index(&mut self, node_id: &NodeId) -> Option { + self.sync_peers.iter().position(|p| p.node_id() == node_id) + } + #[inline] fn db(&self) -> &AsyncBlockchainDb { &self.db