From e55d4f6dff7fd873d4300c44a26de38a2b2e5a4e Mon Sep 17 00:00:00 2001 From: David Palm Date: Fri, 4 May 2018 15:24:39 +0200 Subject: [PATCH 1/5] Fix issue 6972 Unpack errors and check for io::ErrorKind::InvalidInput and return our own AddressParse error. Remove the foreign link to std::net::AddrParseError and add an `impl From` for that error. Test parsing properly. --- Cargo.lock | 7 ++ util/network-devp2p/Cargo.toml | 1 + util/network-devp2p/src/lib.rs | 2 + util/network-devp2p/src/node_table.rs | 92 +++++++++++++++++++++++++-- util/network/src/error.rs | 13 +++- 5 files changed, 110 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 741cc3832ce..a8446558f63 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -35,6 +35,11 @@ dependencies = [ "nodrop 0.1.12 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "assert_matches" +version = "1.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" + [[package]] name = "aster" version = "0.41.0" @@ -700,6 +705,7 @@ name = "ethcore-network-devp2p" version = "1.12.0" dependencies = [ "ansi_term 0.10.2 (registry+https://github.com/rust-lang/crates.io-index)", + "assert_matches 1.2.0 (registry+https://github.com/rust-lang/crates.io-index)", "bytes 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)", "error-chain 0.11.0 (registry+https://github.com/rust-lang/crates.io-index)", "ethcore-bytes 0.1.0", @@ -3763,6 +3769,7 @@ dependencies = [ "checksum ansi_term 0.10.2 (registry+https://github.com/rust-lang/crates.io-index)" = "6b3568b48b7cefa6b8ce125f9bb4989e52fbcc29ebea88df04cc7c5f12f70455" "checksum app_dirs 1.2.1 (git+https://github.com/paritytech/app-dirs-rs)" = "" "checksum arrayvec 0.4.7 (registry+https://github.com/rust-lang/crates.io-index)" = "a1e964f9e24d588183fcb43503abda40d288c8657dfc27311516ce2f05675aef" +"checksum assert_matches 1.2.0 (registry+https://github.com/rust-lang/crates.io-index)" = "664470abf00fae0f31c0eb6e1ca12d82961b2a2541ef898bc9dd51a9254d218b" "checksum aster 0.41.0 (registry+https://github.com/rust-lang/crates.io-index)" = "4ccfdf7355d9db158df68f976ed030ab0f6578af811f5a7bb6dcf221ec24e0e0" "checksum atty 0.2.8 (registry+https://github.com/rust-lang/crates.io-index)" = "af80143d6f7608d746df1520709e5d141c96f240b0e62b0aa41bdfb53374d9d4" "checksum backtrace 0.3.5 (registry+https://github.com/rust-lang/crates.io-index)" = "ebbbf59b1c43eefa8c3ede390fcc36820b4999f7914104015be25025e0d62af2" diff --git a/util/network-devp2p/Cargo.toml b/util/network-devp2p/Cargo.toml index a8207ca615b..3ec96843931 100644 --- a/util/network-devp2p/Cargo.toml +++ b/util/network-devp2p/Cargo.toml @@ -38,6 +38,7 @@ error-chain = { version = "0.11", default-features = false } [dev-dependencies] tempdir = "0.3" +assert_matches = "1.2" [features] default = [] diff --git a/util/network-devp2p/src/lib.rs b/util/network-devp2p/src/lib.rs index 8faf21e465c..239763cbe18 100644 --- a/util/network-devp2p/src/lib.rs +++ b/util/network-devp2p/src/lib.rs @@ -95,6 +95,8 @@ extern crate serde_derive; #[cfg(test)] extern crate tempdir; +#[cfg(test)] #[macro_use] +extern crate assert_matches; mod host; mod connection; diff --git a/util/network-devp2p/src/node_table.rs b/util/network-devp2p/src/node_table.rs index fd18c10a12c..fa1ac2a8947 100644 --- a/util/network-devp2p/src/node_table.rs +++ b/util/network-devp2p/src/node_table.rs @@ -27,6 +27,7 @@ use network::{Error, ErrorKind, AllowIP, IpFilter}; use discovery::{TableUpdates, NodeEntry}; use ip_utils::*; use serde_json; +use std::io; /// Node public key pub type NodeId = H512; @@ -122,18 +123,31 @@ impl FromStr for NodeEndpoint { address: a, udp_port: a.port() }), - Ok(_) => Err(ErrorKind::AddressResolve(None).into()), - Err(e) => Err(ErrorKind::AddressResolve(Some(e)).into()) + Ok(None) => { + // REVIEW: how is this case possible? I think we can remove this case. + Err(ErrorKind::AddressResolve(None).into()) + }, + Err(e) => { + match e.kind() { + io::ErrorKind::InvalidInput => { + Err(ErrorKind::AddressParse(Some(e)).into()) + }, + _ => { + Err(e.into()) + } + } + } } } } -#[derive(PartialEq, Eq, Copy, Clone)] +#[derive(Debug, PartialEq, Eq, Copy, Clone)] pub enum PeerType { _Required, Optional } +#[derive(Debug)] pub struct Node { pub id: NodeId, pub endpoint: NodeEndpoint, @@ -442,11 +456,56 @@ mod tests { assert!(endpoint.is_ok()); let v4 = match endpoint.unwrap().address { SocketAddr::V4(v4address) => v4address, - _ => panic!("should ve v4 address") + _ => panic!("should be v4 address") }; assert_eq!(SocketAddrV4::new(Ipv4Addr::new(123, 99, 55, 44), 7770), v4); } + #[test] + fn endpoint_parse_empty_ip_string_returns_error() { + let endpoint = NodeEndpoint::from_str(""); + assert!(endpoint.is_err()); + assert_matches!( + endpoint.unwrap_err().kind(), + // TODO: after 1.27 is stable, remove the `&`s and `ref`s, see https://github.com/rust-lang/rust/pull/49394 + &ErrorKind::AddressParse(ref io_err) => { + if let &Some(ref e) = io_err { + assert_eq!(e.to_string(), "invalid socket address"); + } + } + ); + } + + #[test] + fn endpoint_parse_invalid_ip_string_returns_error() { + let endpoint = NodeEndpoint::from_str("beef"); + assert!(endpoint.is_err()); + assert_matches!( + endpoint.unwrap_err().kind(), + &ErrorKind::AddressParse(ref io_err) => { + if let &Some(ref e) = io_err { + assert_eq!(e.to_string(), "invalid socket address"); + } + } + ); + } + + #[test] + fn endpoint_parse_valid_ip_without_port_returns_error() { + let endpoint = NodeEndpoint::from_str("123.123.123.123"); + assert!(endpoint.is_err()); + assert_matches!( + endpoint.unwrap_err().kind(), + &ErrorKind::AddressParse(ref io_err) => { + if let &Some(ref e) = io_err { + assert_eq!(e.to_string(), "invalid socket address"); + } + } + ); + let endpoint = NodeEndpoint::from_str("123.123.123.123:123"); + assert!(endpoint.is_ok()) + } + #[test] fn node_parse() { assert!(validate_node_url("enode://a979fb575495b8d6db44f750317d0f4622bf4c2aa3365d6af7c284339968eef29b69ad0dce72a4d8db5ebb4968de0e3bec910127f134779fbcb0cb6d3331163c@22.99.55.44:7770").is_none()); @@ -463,6 +522,31 @@ mod tests { node.id); } + #[test] + fn node_parse_fails_for_invalid_urls() { + let node = Node::from_str("foo"); + assert!(node.is_err()); + assert_matches!( + node.unwrap_err().kind(), + &ErrorKind::AddressParse(ref io_err) => { + if let &Some(ref e) = io_err { + assert_eq!(e.to_string(), "invalid socket address"); + } + } + ); + + let node = Node::from_str("enode://foo@bar"); + assert!(node.is_err()); + assert_matches!( + node.unwrap_err().kind(), + &ErrorKind::AddressParse(ref io_err) => { + if let &Some(ref e) = io_err { + assert_eq!(e.to_string(), "invalid port value"); + } + } + ); + } + #[test] fn table_failure_percentage_order() { let node1 = Node::from_str("enode://a979fb575495b8d6db44f750317d0f4622bf4c2aa3365d6af7c284339968eef29b69ad0dce72a4d8db5ebb4968de0e3bec910127f134779fbcb0cb6d3331163c@22.99.55.44:7770").unwrap(); diff --git a/util/network/src/error.rs b/util/network/src/error.rs index 48bcf759656..d1e712a2b35 100644 --- a/util/network/src/error.rs +++ b/util/network/src/error.rs @@ -84,11 +84,16 @@ error_chain! { foreign_links { SocketIo(IoError) #[doc = "Socket IO error."]; Io(io::Error) #[doc = "Error concerning the Rust standard library's IO subsystem."]; - AddressParse(net::AddrParseError) #[doc = "Error concerning the network address parsing subsystem."]; Decompression(snappy::InvalidInput) #[doc = "Decompression error."]; } errors { + #[doc = "Error concerning the network address parsing subsystem."] + AddressParse(err: Option) { + description("Failed to parse network address"), + display("Failed to parse network address {}", err.as_ref().map_or("".to_string(), |e| e.to_string())), + } + #[doc = "Error concerning the network address resolution subsystem."] AddressResolve(err: Option) { description("Failed to resolve network address"), @@ -157,6 +162,12 @@ impl From for Error { } } +impl From for Error { + fn from(_err: net::AddrParseError) -> Self { + ErrorKind::AddressParse(None).into() + } +} + #[test] fn test_errors() { assert_eq!(DisconnectReason::ClientQuit, DisconnectReason::from_u8(8)); From 645752d6c11a738908146aa1493535e7a0ebe95d Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 7 May 2018 10:36:17 +0200 Subject: [PATCH 2/5] Sort `use`s --- util/network-devp2p/src/node_table.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/util/network-devp2p/src/node_table.rs b/util/network-devp2p/src/node_table.rs index fa1ac2a8947..64337312586 100644 --- a/util/network-devp2p/src/node_table.rs +++ b/util/network-devp2p/src/node_table.rs @@ -14,20 +14,20 @@ // You should have received a copy of the GNU General Public License // along with Parity. If not, see . +use discovery::{TableUpdates, NodeEntry}; +use ethereum_types::H512; +use ip_utils::*; +use network::{Error, ErrorKind, AllowIP, IpFilter}; +use rlp::{Rlp, RlpStream, DecoderError}; +use serde_json; use std::collections::{HashMap, HashSet}; use std::fmt::{self, Display, Formatter}; use std::hash::{Hash, Hasher}; +use std::io; use std::net::{SocketAddr, ToSocketAddrs, SocketAddrV4, SocketAddrV6, Ipv4Addr, Ipv6Addr}; use std::path::PathBuf; use std::str::FromStr; use std::{fs, mem, slice}; -use ethereum_types::H512; -use rlp::{Rlp, RlpStream, DecoderError}; -use network::{Error, ErrorKind, AllowIP, IpFilter}; -use discovery::{TableUpdates, NodeEntry}; -use ip_utils::*; -use serde_json; -use std::io; /// Node public key pub type NodeId = H512; From 3618f60399bd49daed3a267dccb9057edea8db69 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 7 May 2018 10:42:15 +0200 Subject: [PATCH 3/5] Additional asserts to ensure we have the expected data --- util/network-devp2p/src/node_table.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/util/network-devp2p/src/node_table.rs b/util/network-devp2p/src/node_table.rs index 64337312586..9c95d859ede 100644 --- a/util/network-devp2p/src/node_table.rs +++ b/util/network-devp2p/src/node_table.rs @@ -469,6 +469,7 @@ mod tests { endpoint.unwrap_err().kind(), // TODO: after 1.27 is stable, remove the `&`s and `ref`s, see https://github.com/rust-lang/rust/pull/49394 &ErrorKind::AddressParse(ref io_err) => { + assert!(io_err.is_some()); if let &Some(ref e) = io_err { assert_eq!(e.to_string(), "invalid socket address"); } @@ -483,6 +484,7 @@ mod tests { assert_matches!( endpoint.unwrap_err().kind(), &ErrorKind::AddressParse(ref io_err) => { + assert!(io_err.is_some()); if let &Some(ref e) = io_err { assert_eq!(e.to_string(), "invalid socket address"); } @@ -497,6 +499,7 @@ mod tests { assert_matches!( endpoint.unwrap_err().kind(), &ErrorKind::AddressParse(ref io_err) => { + assert!(io_err.is_some()); if let &Some(ref e) = io_err { assert_eq!(e.to_string(), "invalid socket address"); } From 6bc801b9cb2fb3c84d435baa45e58f584bbc80d4 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 7 May 2018 10:48:16 +0200 Subject: [PATCH 4/5] Remove REVIEW comment based on feedback --- util/network-devp2p/src/node_table.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/util/network-devp2p/src/node_table.rs b/util/network-devp2p/src/node_table.rs index 9c95d859ede..f8251690df7 100644 --- a/util/network-devp2p/src/node_table.rs +++ b/util/network-devp2p/src/node_table.rs @@ -123,10 +123,7 @@ impl FromStr for NodeEndpoint { address: a, udp_port: a.port() }), - Ok(None) => { - // REVIEW: how is this case possible? I think we can remove this case. - Err(ErrorKind::AddressResolve(None).into()) - }, + Ok(None) => bail!(ErrorKind::AddressResolve(None)), Err(e) => { match e.kind() { io::ErrorKind::InvalidInput => { From 940e4b9e963ddd35c31cb0c2058f1617b8432c1c Mon Sep 17 00:00:00 2001 From: David Palm Date: Wed, 9 May 2018 06:33:09 +0200 Subject: [PATCH 5/5] Don't embed the io error in AddressParse --- util/network-devp2p/src/node_table.rs | 61 +++------------------------ util/network/src/error.rs | 8 ++-- 2 files changed, 9 insertions(+), 60 deletions(-) diff --git a/util/network-devp2p/src/node_table.rs b/util/network-devp2p/src/node_table.rs index f8251690df7..0ab72815f11 100644 --- a/util/network-devp2p/src/node_table.rs +++ b/util/network-devp2p/src/node_table.rs @@ -23,7 +23,6 @@ use serde_json; use std::collections::{HashMap, HashSet}; use std::fmt::{self, Display, Formatter}; use std::hash::{Hash, Hasher}; -use std::io; use std::net::{SocketAddr, ToSocketAddrs, SocketAddrV4, SocketAddrV6, Ipv4Addr, Ipv6Addr}; use std::path::PathBuf; use std::str::FromStr; @@ -124,16 +123,7 @@ impl FromStr for NodeEndpoint { udp_port: a.port() }), Ok(None) => bail!(ErrorKind::AddressResolve(None)), - Err(e) => { - match e.kind() { - io::ErrorKind::InvalidInput => { - Err(ErrorKind::AddressParse(Some(e)).into()) - }, - _ => { - Err(e.into()) - } - } - } + Err(_) => Err(ErrorKind::AddressParse.into()) // always an io::Error of InvalidInput kind } } } @@ -462,46 +452,21 @@ mod tests { fn endpoint_parse_empty_ip_string_returns_error() { let endpoint = NodeEndpoint::from_str(""); assert!(endpoint.is_err()); - assert_matches!( - endpoint.unwrap_err().kind(), - // TODO: after 1.27 is stable, remove the `&`s and `ref`s, see https://github.com/rust-lang/rust/pull/49394 - &ErrorKind::AddressParse(ref io_err) => { - assert!(io_err.is_some()); - if let &Some(ref e) = io_err { - assert_eq!(e.to_string(), "invalid socket address"); - } - } - ); + assert_matches!(endpoint.unwrap_err().kind(), &ErrorKind::AddressParse); } #[test] fn endpoint_parse_invalid_ip_string_returns_error() { let endpoint = NodeEndpoint::from_str("beef"); assert!(endpoint.is_err()); - assert_matches!( - endpoint.unwrap_err().kind(), - &ErrorKind::AddressParse(ref io_err) => { - assert!(io_err.is_some()); - if let &Some(ref e) = io_err { - assert_eq!(e.to_string(), "invalid socket address"); - } - } - ); + assert_matches!(endpoint.unwrap_err().kind(), &ErrorKind::AddressParse); } #[test] fn endpoint_parse_valid_ip_without_port_returns_error() { let endpoint = NodeEndpoint::from_str("123.123.123.123"); assert!(endpoint.is_err()); - assert_matches!( - endpoint.unwrap_err().kind(), - &ErrorKind::AddressParse(ref io_err) => { - assert!(io_err.is_some()); - if let &Some(ref e) = io_err { - assert_eq!(e.to_string(), "invalid socket address"); - } - } - ); + assert_matches!(endpoint.unwrap_err().kind(), &ErrorKind::AddressParse); let endpoint = NodeEndpoint::from_str("123.123.123.123:123"); assert!(endpoint.is_ok()) } @@ -526,25 +491,11 @@ mod tests { fn node_parse_fails_for_invalid_urls() { let node = Node::from_str("foo"); assert!(node.is_err()); - assert_matches!( - node.unwrap_err().kind(), - &ErrorKind::AddressParse(ref io_err) => { - if let &Some(ref e) = io_err { - assert_eq!(e.to_string(), "invalid socket address"); - } - } - ); + assert_matches!(node.unwrap_err().kind(), &ErrorKind::AddressParse); let node = Node::from_str("enode://foo@bar"); assert!(node.is_err()); - assert_matches!( - node.unwrap_err().kind(), - &ErrorKind::AddressParse(ref io_err) => { - if let &Some(ref e) = io_err { - assert_eq!(e.to_string(), "invalid port value"); - } - } - ); + assert_matches!(node.unwrap_err().kind(), &ErrorKind::AddressParse); } #[test] diff --git a/util/network/src/error.rs b/util/network/src/error.rs index 83d47958a1e..61044866966 100644 --- a/util/network/src/error.rs +++ b/util/network/src/error.rs @@ -89,9 +89,9 @@ error_chain! { errors { #[doc = "Error concerning the network address parsing subsystem."] - AddressParse(err: Option) { + AddressParse { description("Failed to parse network address"), - display("Failed to parse network address {}", err.as_ref().map_or("".to_string(), |e| e.to_string())), + display("Failed to parse network address"), } #[doc = "Error concerning the network address resolution subsystem."] @@ -169,9 +169,7 @@ impl From for Error { } impl From for Error { - fn from(_err: net::AddrParseError) -> Self { - ErrorKind::AddressParse(None).into() - } + fn from(_err: net::AddrParseError) -> Self { ErrorKind::AddressParse.into() } } #[test]