Handle socket address parsing errors

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 = []

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;

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();

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<io::Error>) {
+			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<io::Error>) {
 			description("Failed to resolve network address"),
@@ -157,6 +162,12 @@ impl From<crypto::Error> for Error {
 	}
 }
 
+impl From<net::AddrParseError> for Error {
+	fn from(_err: net::AddrParseError) -> Self {
+		ErrorKind::AddressParse(None).into()
+	}
+}
+
 #[test]
 fn test_errors() {
 	assert_eq!(DisconnectReason::ClientQuit, DisconnectReason::from_u8(8));