handling invalid spec jsons properly, additional tests, closes #1840

ethcore/src/engines/basic_authority.rs

@@ -187,7 +187,10 @@ mod tests {
 	use spec::Spec;
 
 	/// Create a new test chain spec with `BasicAuthority` consensus engine.
-	fn new_test_authority() -> Spec { Spec::load(include_bytes!("../../res/test_authority.json")) }
+	fn new_test_authority() -> Spec {
+		let bytes: &[u8] = include_bytes!("../../res/test_authority.json");
+		Spec::load(bytes).expect("invalid chain spec")
+	}
 
 	#[test]
 	fn has_valid_metadata() {

ethcore/src/engines/instant_seal.rs

@@ -72,7 +72,10 @@ mod tests {
 	use block::*;
 
 	/// Create a new test chain spec with `BasicAuthority` consensus engine.
-	fn new_test_instant() -> Spec { Spec::load(include_bytes!("../../res/instant_seal.json")) }
+	fn new_test_instant() -> Spec {
+		let bytes: &[u8] = include_bytes!("../../res/instant_seal.json");
+		Spec::load(bytes).expect("invalid chain spec")
+	}
 
 	#[test]
 	fn instant_can_seal() {

ethcore/src/ethereum/mod.rs

@@ -29,29 +29,33 @@ pub use self::denominations::*;
 
 use super::spec::*;
 
+fn load(b: &[u8]) -> Spec {
+	Spec::load(b).expect("chain spec is invalid")
+}
+
 /// Create a new Olympic chain spec.
-pub fn new_olympic() -> Spec { Spec::load(include_bytes!("../../res/ethereum/olympic.json")) }
+pub fn new_olympic() -> Spec { load(include_bytes!("../../res/ethereum/olympic.json")) }
 
 /// Create a new Frontier mainnet chain spec.
-pub fn new_frontier() -> Spec { Spec::load(include_bytes!("../../res/ethereum/frontier.json")) }
+pub fn new_frontier() -> Spec { load(include_bytes!("../../res/ethereum/frontier.json")) }
 
 /// Create a new Frontier mainnet chain spec without the DAO hardfork.
-pub fn new_classic() -> Spec { Spec::load(include_bytes!("../../res/ethereum/classic.json")) }
+pub fn new_classic() -> Spec { load(include_bytes!("../../res/ethereum/classic.json")) }
 
 /// Create a new Frontier chain spec as though it never changes to Homestead.
-pub fn new_frontier_test() -> Spec { Spec::load(include_bytes!("../../res/ethereum/frontier_test.json")) }
+pub fn new_frontier_test() -> Spec { load(include_bytes!("../../res/ethereum/frontier_test.json")) }
 
 /// Create a new Homestead chain spec as though it never changed from Frontier.
-pub fn new_homestead_test() -> Spec { Spec::load(include_bytes!("../../res/ethereum/homestead_test.json")) }
+pub fn new_homestead_test() -> Spec { load(include_bytes!("../../res/ethereum/homestead_test.json")) }
 
 /// Create a new Frontier/Homestead/DAO chain spec with transition points at #5 and #8.
-pub fn new_daohardfork_test() -> Spec { Spec::load(include_bytes!("../../res/ethereum/daohardfork_test.json")) }
+pub fn new_daohardfork_test() -> Spec { load(include_bytes!("../../res/ethereum/daohardfork_test.json")) }
 
 /// Create a new Frontier main net chain spec without genesis accounts.
-pub fn new_mainnet_like() -> Spec { Spec::load(include_bytes!("../../res/ethereum/frontier_like_test.json")) }
+pub fn new_mainnet_like() -> Spec { load(include_bytes!("../../res/ethereum/frontier_like_test.json")) }
 
 /// Create a new Morden chain spec.
-pub fn new_morden() -> Spec { Spec::load(include_bytes!("../../res/ethereum/morden.json")) }
+pub fn new_morden() -> Spec { load(include_bytes!("../../res/ethereum/morden.json")) }
 
 #[cfg(test)]
 mod tests {

ethcore/src/spec/spec.rs

@@ -244,18 +244,21 @@ impl Spec {
 	}
 
 	/// Loads spec from json file.
-	pub fn load(reader: &[u8]) -> Self {
-		From::from(ethjson::spec::Spec::load(reader).expect("invalid json file"))
+	pub fn load<R>(reader: R) -> Result<Self, String> where R: Read {
+		match ethjson::spec::Spec::load(reader) {
+			Ok(spec) => Ok(spec.into()),
+			_ => Err("Spec json is invalid".into()),
+		}
 	}
 
 	/// Create a new Spec which conforms to the Frontier-era Morden chain except that it's a NullEngine consensus.
-	pub fn new_test() -> Spec {
-		Spec::load(include_bytes!("../../res/null_morden.json"))
+	pub fn new_test() -> Self {
+		Spec::load(include_bytes!("../../res/null_morden.json") as &[u8]).expect("null_morden.json is invalid")
 	}
 
 	/// Create a new Spec which is a NullEngine consensus with a premine of address whose secret is sha3('').
-	pub fn new_null() -> Spec {
-		Spec::load(include_bytes!("../../res/null.json"))
+	pub fn new_null() -> Self {
+		Spec::load(include_bytes!("../../res/null.json") as &[u8]).expect("null.json is invalid")
 	}
 }
 
@@ -267,6 +270,12 @@ mod tests {
 	use views::*;
 	use super::*;
 
+	// https://github.com/ethcore/parity/issues/1840
+	#[test]
+	fn test_load_empty() {
+		assert!(Spec::load(&vec![] as &[u8]).is_err());
+	}
+
 	#[test]
 	fn test_chain() {
 		let test_spec = Spec::new_test();

parity/params.rs

@@ -17,7 +17,7 @@
 use std::str::FromStr;
 use std::fs;
 use std::time::Duration;
-use util::{contents, H256, Address, U256, version_data};
+use util::{H256, Address, U256, version_data};
 use util::journaldb::Algorithm;
 use ethcore::spec::Spec;
 use ethcore::ethereum;
@@ -61,7 +61,10 @@ impl SpecType {
 			SpecType::Testnet => Ok(ethereum::new_morden()),
 			SpecType::Olympic => Ok(ethereum::new_olympic()),
 			SpecType::Classic => Ok(ethereum::new_classic()),
-			SpecType::Custom(ref file) => Ok(Spec::load(&try!(contents(file).map_err(|_| "Could not load specification file."))))
+			SpecType::Custom(ref filename) => {
+				let file = try!(fs::File::open(filename).map_err(|_| "Could not load specification file."));
+				Spec::load(file)
+			}
 		}
 	}
 }

util/src/misc.rs

@@ -16,7 +16,6 @@
 
 //! Diff misc.
 
-use std::fs::File;
 use common::*;
 use rlp::{Stream, RlpStream};
 use target_info::Target;
@@ -33,14 +32,6 @@ pub enum Filth {
 	Dirty,
 }
 
-/// Read the whole contents of a file `name`.
-pub fn contents(name: &str) -> Result<Bytes, UtilError> {
-	let mut file = try!(File::open(name));
-	let mut ret: Vec<u8> = Vec::new();
-	try!(file.read_to_end(&mut ret));
-	Ok(ret)
-}
-
 /// Get the standard version string for this software.
 pub fn version() -> String {
 	let sha3 = short_sha();
@@ -64,4 +55,4 @@ pub fn version_data() -> Bytes {
 	s.append(&rustc_version());
 	s.append(&&Target::os()[0..2]);
 	s.out()
-}
+}