From 07667771ef79b31167d834b12bd14fd73abff96e Mon Sep 17 00:00:00 2001
From: Sagar Dhawan <sagar@solana.com>
Date: Sat, 17 Nov 2018 19:57:28 -0800
Subject: [PATCH] Fix Gossip Pushes going to invalid addresses (#1858)

---
 src/cluster_info.rs     | 43 ++++++++++++++++++++++++++---------------
 src/crds_gossip_push.rs |  6 ++++++
 tests/multinode.rs      |  3 ++-
 3 files changed, 35 insertions(+), 17 deletions(-)

diff --git a/src/cluster_info.rs b/src/cluster_info.rs
index e3f64875ca..a1bc1049ae 100644
--- a/src/cluster_info.rs
+++ b/src/cluster_info.rs
@@ -189,7 +189,7 @@ impl ClusterInfo {
             .values()
             .filter_map(|x| x.value.contact_info())
             .filter(|x| x.id != me)
-            .filter(|x| ClusterInfo::is_valid_address(&x.rpc))
+            .filter(|x| ContactInfo::is_valid_address(&x.rpc))
             .cloned()
             .collect()
     }
@@ -202,7 +202,7 @@ impl ClusterInfo {
             .values()
             .filter_map(|x| x.value.contact_info())
             .filter(|x| x.id != me)
-            .filter(|x| ClusterInfo::is_valid_address(&x.ncp))
+            .filter(|x| ContactInfo::is_valid_address(&x.ncp))
             .cloned()
             .collect()
     }
@@ -216,7 +216,7 @@ impl ClusterInfo {
             .values()
             .filter_map(|x| x.value.contact_info())
             .filter(|x| x.id != me)
-            .filter(|x| ClusterInfo::is_valid_address(&x.tvu))
+            .filter(|x| ContactInfo::is_valid_address(&x.tvu))
             .cloned()
             .collect()
     }
@@ -230,7 +230,7 @@ impl ClusterInfo {
             .values()
             .filter_map(|x| x.value.contact_info())
             .filter(|x| x.id != me)
-            .filter(|x| ClusterInfo::is_valid_address(&x.tpu))
+            .filter(|x| ContactInfo::is_valid_address(&x.tpu))
             .cloned()
             .collect()
     }
@@ -898,18 +898,6 @@ impl ClusterInfo {
             }).unwrap()
     }
 
-    fn is_valid_ip(addr: IpAddr) -> bool {
-        !(addr.is_unspecified() || addr.is_multicast())
-        // || (addr.is_loopback() && !cfg_test))
-        // TODO: boot loopback in production networks
-    }
-    /// port must not be 0
-    /// ip must be specified and not mulitcast
-    /// loopback ip is only allowed in tests
-    pub fn is_valid_address(addr: &SocketAddr) -> bool {
-        (addr.port() != 0) && Self::is_valid_ip(addr.ip())
-    }
-
     pub fn spy_node() -> (NodeInfo, UdpSocket) {
         let (_, gossip_socket) = bind_in_range(FULLNODE_PORT_RANGE).unwrap();
         let pubkey = Keypair::new().pubkey();
@@ -1057,6 +1045,29 @@ mod tests {
     use std::sync::{Arc, RwLock};
     use window::default_window;
 
+    #[test]
+    fn test_cluster_spy_gossip() {
+        //check that gossip doesn't try to push to invalid addresses
+        let node = Node::new_localhost();
+        let (spy, _) = ClusterInfo::spy_node();
+        let cluster_info = Arc::new(RwLock::new(
+            ClusterInfo::new(node.info).expect("ClusterInfo::new"),
+        ));
+        cluster_info.write().unwrap().insert_info(spy);
+        cluster_info
+            .write()
+            .unwrap()
+            .gossip
+            .refresh_push_active_set();
+        let reqs = cluster_info.write().unwrap().gossip_request();
+        //assert none of the addrs are invalid.
+        reqs.iter().all(|(addr, _)| {
+            let res = ContactInfo::is_valid_address(addr);
+            assert!(res);
+            res
+        });
+    }
+
     #[test]
     fn test_cluster_info_new() {
         let d = NodeInfo::new_localhost(Keypair::new().pubkey(), timestamp());
diff --git a/src/crds_gossip_push.rs b/src/crds_gossip_push.rs
index afdd872737..00f50ba630 100644
--- a/src/crds_gossip_push.rs
+++ b/src/crds_gossip_push.rs
@@ -10,6 +10,7 @@
 
 use bincode::serialized_size;
 use bloom::Bloom;
+use contact_info::ContactInfo;
 use crds::{Crds, VersionedCrdsValue};
 use crds_gossip_error::CrdsGossipError;
 use crds_value::{CrdsValue, CrdsValueLabel};
@@ -177,6 +178,11 @@ impl CrdsGossipPush {
             if new_items.get(&val.0.pubkey()).is_some() {
                 continue;
             }
+            if let Some(contact) = val.1.value.contact_info() {
+                if !ContactInfo::is_valid_address(&contact.ncp) {
+                    continue;
+                }
+            }
             let bloom = Bloom::random(network_size, 0.1, 1024 * 8 * 4);
             new_items.insert(val.0.pubkey(), bloom);
             if new_items.len() == need {
diff --git a/tests/multinode.rs b/tests/multinode.rs
index d138ef613c..ea7c454552 100644
--- a/tests/multinode.rs
+++ b/tests/multinode.rs
@@ -8,6 +8,7 @@ extern crate solana_sdk;
 
 use solana::blob_fetch_stage::BlobFetchStage;
 use solana::cluster_info::{ClusterInfo, Node, NodeInfo};
+use solana::contact_info::ContactInfo;
 use solana::entry::{reconstruct_entries_from_blobs, Entry};
 use solana::fullnode::{Fullnode, FullnodeReturnType};
 use solana::leader_scheduler::{make_active_set_entries, LeaderScheduler, LeaderSchedulerConfig};
@@ -1621,7 +1622,7 @@ fn test_broadcast_last_tick() {
 
 fn mk_client(leader: &NodeInfo) -> ThinClient {
     let transactions_socket = UdpSocket::bind("0.0.0.0:0").unwrap();
-    assert!(ClusterInfo::is_valid_address(&leader.tpu));
+    assert!(ContactInfo::is_valid_address(&leader.tpu));
     ThinClient::new(leader.rpc, leader.tpu, transactions_socket)
 }