April 2018 - tor-commits - lists.torproject.org

[tor/master] Merge remote-tracking branch 'isis-github/bug24031_r5_squashed_033' into maint-0.3.3
by nickm＠torproject.org 03 Apr '18

03 Apr '18

commit 8d6b1da2e6729c0cf8331e663bdfeee5d5660237 Merge: 961d2ad59 b503df277 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Apr 3 15:29:29 2018 -0400 Merge remote-tracking branch 'isis-github/bug24031_r5_squashed_033' into maint-0.3.3 changes/bug24031 | 13 + src/or/protover.c | 77 ++- src/rust/protover/errors.rs | 43 ++ src/rust/protover/ffi.rs | 94 ++- src/rust/protover/lib.rs | 4 + src/rust/protover/protoset.rs | 634 ++++++++++++++++++ src/rust/protover/protover.rs | 1265 ++++++++++++++++------------------- src/rust/protover/tests/protover.rs | 421 +++++++----- src/test/test_protover.c | 44 +- 9 files changed, 1690 insertions(+), 905 deletions(-)

1 0

[tor/master] rust: Refactor protover::compute_for_old_tor().
by nickm＠torproject.org 03 Apr '18

03 Apr '18

commit cd28b4c7f5cefd319d6ded635d25911b4323b50b Author: Isis Lovecruft <isis(a)torproject.org> Date: Tue Mar 27 02:41:25 2018 +0000 rust: Refactor protover::compute_for_old_tor(). During code review and discussion with Chelsea Komlo, she pointed out that protover::compute_for_old_tor() was a public function whose return type was `&'static CStr`. We both agree that C-like parts of APIs should: 1. not be exposed publicly (to other Rust crates), 2. only be called in the appropriate FFI code, 3. not expose types which are meant for FFI code (e.g. `*mut char`, `CString`, `*const c_int`, etc.) to the pure-Rust code of other crates. 4. FFI code (e.g. things in `ffi.rs` modules) should _never_ be called from pure-Rust, not even from other modules in its own crate (i.e. do not call `protover::ffi::*` from anywhere in `protover::protoset::*`, etc). With that in mind, this commit makes the following changes: * CHANGE `protover::compute_for_old_tor()` to be visible only at the `pub(crate)` level. * RENAME `protover::compute_for_old_tor()` to `protover::compute_for_old_tor_cstr()` to reflect the last change. * ADD a new `protover::compute_for_old_tor()` function wrapper which is public and intended for other Rust code to use, which returns a `&str`. --- src/rust/protover/ffi.rs | 2 +- src/rust/protover/protover.rs | 58 +++++++++++++++++++++++++++++++++++++------ 2 files changed, 51 insertions(+), 9 deletions(-) diff --git a/src/rust/protover/ffi.rs b/src/rust/protover/ffi.rs index 3632f5de8..a40353eb1 100644 --- a/src/rust/protover/ffi.rs +++ b/src/rust/protover/ffi.rs @@ -246,7 +246,7 @@ pub extern "C" fn protover_compute_for_old_tor(version: *const c_char) -> *const Err(_) => return empty.as_ptr(), }; - elder_protocols = compute_for_old_tor(&version); + elder_protocols = compute_for_old_tor_cstr(&version); // If we're going to pass it to C, there cannot be any intermediate NUL // bytes. An assert is okay here, since changing the const byte slice diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs index 96e9dd582..fc89f70d4 100644 --- a/src/rust/protover/protover.rs +++ b/src/rust/protover/protover.rs @@ -611,8 +611,8 @@ pub fn is_supported_here(proto: &Protocol, vers: &Version) -> bool { supported_versions.contains(vers) } -/// Older versions of Tor cannot infer their own subprotocols -/// Used to determine which subprotocols are supported by older Tor versions. +/// Since older versions of Tor cannot infer their own subprotocols, +/// determine which subprotocols are supported by older Tor versions. /// /// # Inputs /// @@ -626,24 +626,28 @@ pub fn is_supported_here(proto: &Protocol, vers: &Version) -> bool { /// "HSDir=1-1 LinkAuth=1" /// /// This function returns the protocols that are supported by the version input, -/// only for tor versions older than FIRST_TOR_VERSION_TO_ADVERTISE_PROTOCOLS. +/// only for tor versions older than `FIRST_TOR_VERSION_TO_ADVERTISE_PROTOCOLS` +/// (but not older than 0.2.4.19). For newer tors (or older than 0.2.4.19), it +/// returns an empty string. /// -/// C_RUST_COUPLED: src/rust/protover.c `compute_for_old_tor` -pub fn compute_for_old_tor(version: &str) -> &'static [u8] { +/// # Note +/// +/// This function is meant to be called for/within FFI code. If you'd +/// like to use this code in Rust, please see `compute_for_old_tor()`. +// +// C_RUST_COUPLED: src/rust/protover.c `compute_for_old_tor` +pub(crate) fn compute_for_old_tor_cstr(version: &str) -> &'static [u8] { if c_tor_version_as_new_as(version, FIRST_TOR_VERSION_TO_ADVERTISE_PROTOCOLS) { return NUL_BYTE; } - if c_tor_version_as_new_as(version, "0.2.9.1-alpha") { return b"Cons=1-2 Desc=1-2 DirCache=1 HSDir=1 HSIntro=3 HSRend=1-2 \ Link=1-4 LinkAuth=1 Microdesc=1-2 Relay=1-2\0"; } - if c_tor_version_as_new_as(version, "0.2.7.5") { return b"Cons=1-2 Desc=1-2 DirCache=1 HSDir=1 HSIntro=3 HSRend=1 \ Link=1-4 LinkAuth=1 Microdesc=1-2 Relay=1-2\0"; } - if c_tor_version_as_new_as(version, "0.2.4.19") { return b"Cons=1 Desc=1 DirCache=1 HSDir=1 HSIntro=3 HSRend=1 \ Link=1-4 LinkAuth=1 Microdesc=1 Relay=1-2\0"; @@ -652,6 +656,44 @@ pub fn compute_for_old_tor(version: &str) -> &'static [u8] { NUL_BYTE } +/// Since older versions of Tor cannot infer their own subprotocols, +/// determine which subprotocols are supported by older Tor versions. +/// +/// # Inputs +/// +/// * `version`, a string comprised of "[0-9a-z.-]" +/// +/// # Returns +/// +/// A `Result` whose `Ok` value is an `&'static str` encoding a list of protocol +/// names and supported versions. The string takes the following format: +/// +/// "HSDir=1-1 LinkAuth=1" +/// +/// This function returns the protocols that are supported by the version input, +/// only for tor versions older than `FIRST_TOR_VERSION_TO_ADVERTISE_PROTOCOLS`. +/// (but not older than 0.2.4.19). For newer tors (or older than 0.2.4.19), its +/// `Ok` `Result` contains an empty string. +/// +/// Otherwise, its `Err` contains a `ProtoverError::Unparseable` if the +/// `version` string was invalid utf-8. +/// +/// # Note +/// +/// This function is meant to be called for/within non-FFI Rust code. +// +// C_RUST_COUPLED: src/rust/protover.c `compute_for_old_tor` +pub fn compute_for_old_tor(version: &str) -> Result<&'static str, ProtoverError> { + let mut computed: &'static [u8] = compute_for_old_tor_cstr(version); + + // Remove the NULL byte at the end. + computed = &computed[..computed.len() - 1]; + + // .from_utf8() fails with a Utf8Error if it couldn't validate the + // utf-8, so convert that here into an Unparseable ProtoverError. + str::from_utf8(computed).or(Err(ProtoverError::Unparseable)) +} + #[cfg(test)] mod test { use std::str::FromStr;

1 0

[tor/master] rust: Refactor Rust implementation of protover_is_supported_here().
by nickm＠torproject.org 03 Apr '18

03 Apr '18

commit fd127bfbfa13d407e5fb5d22a567f51a30af4c2e Author: Isis Lovecruft <isis(a)torproject.org> Date: Wed Mar 21 03:08:35 2018 +0000 rust: Refactor Rust implementation of protover_is_supported_here(). It was changed to take borrows instead of taking ownership. * REFACTOR `protover::ffi::protover_is_supported_here()` to use changed method signature on `protover::is_supported_here()`. --- src/rust/protover/ffi.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rust/protover/ffi.rs b/src/rust/protover/ffi.rs index 13d64821f..3632f5de8 100644 --- a/src/rust/protover/ffi.rs +++ b/src/rust/protover/ffi.rs @@ -218,7 +218,7 @@ pub extern "C" fn protover_is_supported_here( Err(_) => return 0, }; - let is_supported = is_supported_here(protocol, version); + let is_supported = is_supported_here(&protocol, &version); return if is_supported { 1 } else { 0 }; }

1 0

[tor/master] rust: Fix ProtoSet and ProtoEntry to use the same DoS limits as C.
by nickm＠torproject.org 03 Apr '18

03 Apr '18

commit c65088cb1943748412e1a390de655e20bdb28692 Author: Isis Lovecruft <isis(a)torproject.org> Date: Tue Mar 27 22:46:14 2018 +0000 rust: Fix ProtoSet and ProtoEntry to use the same DoS limits as C. Previously, the limit for MAX_PROTOCOLS_TO_EXPAND was actually being applied in Rust to the maximum number of version (total, for all subprotocols). Whereas in C, it was being applied to the number of subprotocols that were allowed. This changes the Rust to match C's behaviour. --- src/rust/protover/protoset.rs | 14 +++++++------- src/rust/protover/protover.rs | 18 +++++++++++++++--- src/rust/protover/tests/protover.rs | 14 -------------- 3 files changed, 22 insertions(+), 24 deletions(-) diff --git a/src/rust/protover/protoset.rs b/src/rust/protover/protoset.rs index f94e6299c..4afc50edf 100644 --- a/src/rust/protover/protoset.rs +++ b/src/rust/protover/protoset.rs @@ -8,7 +8,6 @@ use std::slice; use std::str::FromStr; use std::u32; -use protover::MAX_PROTOCOLS_TO_EXPAND; use errors::ProtoverError; /// A single version number. @@ -183,10 +182,6 @@ impl ProtoSet { last_high = high; } - if self.len() > MAX_PROTOCOLS_TO_EXPAND { - return Err(ProtoverError::ExceedsMax); - } - Ok(self) } @@ -317,9 +312,15 @@ impl FromStr for ProtoSet { /// assert!(protoset.contains(&5)); /// assert!(!protoset.contains(&10)); /// - /// // We can also equivalently call `ProtoSet::from_str` by doing: + /// // We can also equivalently call `ProtoSet::from_str` by doing (all + /// // implementations of `FromStr` can be called this way, this one isn't + /// // special): /// let protoset: ProtoSet = "4-6,12".parse()?; /// + /// // Calling it (either way) can take really large ranges (up to `u32::MAX`): + /// let protoset: ProtoSet = "1-70000".parse()?; + /// let protoset: ProtoSet = "1-4294967296".parse()?; + /// /// // There are lots of ways to get an `Err` from this function. Here are /// // a few: /// assert_eq!(Err(ProtoverError::Unparseable), ProtoSet::from_str("=")); @@ -327,7 +328,6 @@ impl FromStr for ProtoSet { /// assert_eq!(Err(ProtoverError::Unparseable), ProtoSet::from_str("not_an_int")); /// assert_eq!(Err(ProtoverError::Unparseable), ProtoSet::from_str("3-")); /// assert_eq!(Err(ProtoverError::Unparseable), ProtoSet::from_str("1-,4")); - /// assert_eq!(Err(ProtoverError::ExceedsMax), ProtoSet::from_str("1-70000")); /// /// // Things which would get parsed into an _empty_ `ProtoSet` are, /// // however, legal, and result in an empty `ProtoSet`: diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs index fc89f70d4..5e5a31cd3 100644 --- a/src/rust/protover/protover.rs +++ b/src/rust/protover/protover.rs @@ -26,7 +26,7 @@ const FIRST_TOR_VERSION_TO_ADVERTISE_PROTOCOLS: &'static str = "0.2.9.3-alpha"; /// before concluding that someone is trying to DoS us /// /// C_RUST_COUPLED: src/or/protover.c `MAX_PROTOCOLS_TO_EXPAND` -pub(crate) const MAX_PROTOCOLS_TO_EXPAND: usize = (1<<16); +const MAX_PROTOCOLS_TO_EXPAND: usize = (1<<16); /// Currently supported protocols and their versions, as a byte-slice. /// @@ -166,6 +166,10 @@ impl ProtoEntry { supported.parse() } + pub fn len(&self) -> usize { + self.0.len() + } + pub fn get(&self, protocol: &Protocol) -> Option<&ProtoSet> { self.0.get(protocol) } @@ -220,8 +224,11 @@ impl FromStr for ProtoEntry { let proto_name: Protocol = proto.parse()?; proto_entry.insert(proto_name, versions); - } + if proto_entry.len() > MAX_PROTOCOLS_TO_EXPAND { + return Err(ProtoverError::ExceedsMax); + } + } Ok(proto_entry) } } @@ -738,8 +745,13 @@ mod test { } #[test] + fn test_protoentry_from_str_allowed_number_of_versions() { + assert_protoentry_is_parseable!("Desc=1-4294967294"); + } + + #[test] fn test_protoentry_from_str_too_many_versions() { - assert_protoentry_is_unparseable!("Desc=1-65537"); + assert_protoentry_is_unparseable!("Desc=1-4294967295"); } #[test] diff --git a/src/rust/protover/tests/protover.rs b/src/rust/protover/tests/protover.rs index 11015f35b..2db01a163 100644 --- a/src/rust/protover/tests/protover.rs +++ b/src/rust/protover/tests/protover.rs @@ -353,13 +353,6 @@ fn protover_all_supported_should_exclude_some_versions_and_entire_protocols() { } #[test] -#[should_panic(expected = "called `Result::unwrap()` on an `Err` value: ExceedsMax")] -// C_RUST_DIFFERS: This test fails in Rust (but not in C) because an -// UnvalidatedProtoEntry is defined as a Hashmap<UnknownProtocol, ProtoSet>. -// Because it contains the ProtoSet part, there's still *some* validation -// happening, so in this case the DoS protections in the Rust code are kicking -// in because the range here is exceeds the maximum, so it returns an -// Err(ProtoverError::ExceedsMax). The C, however seems to parse it anyway. fn protover_all_supported_should_not_dos_anyones_computer() { let proto: UnvalidatedProtoEntry = "Sleen=0-2147483648".parse().unwrap(); let result: String = proto.all_supported().unwrap().to_string(); @@ -368,13 +361,6 @@ fn protover_all_supported_should_not_dos_anyones_computer() { } #[test] -#[should_panic(expected = "called `Result::unwrap()` on an `Err` value: ExceedsMax")] -// C_RUST_DIFFERS: This test fails in Rust (but not in C) because an -// UnvalidatedProtoEntry is defined as a Hashmap<UnknownProtocol, ProtoSet>. -// Because it contains the ProtoSet part, there's still *some* validation -// happening, so in this case the DoS protections in the Rust code are kicking -// in because the range here is exceeds the maximum, so it returns an -// Err(ProtoverError::ExceedsMax). The C, however seems to parse it anyway. fn protover_all_supported_should_not_dos_anyones_computer_max_versions() { let proto: UnvalidatedProtoEntry = "Sleen=0-4294967294".parse().unwrap(); let result: String = proto.all_supported().unwrap().to_string();

1 0

[tor/release-0.3.3] rust: Add new protover::UnknownProtocol type.
by nickm＠torproject.org 03 Apr '18

03 Apr '18

commit 60daaa68b153cdca6d48b09f1951d6ba580609e5 Author: Isis Lovecruft <isis(a)torproject.org> Date: Wed Mar 21 01:07:18 2018 +0000 rust: Add new protover::UnknownProtocol type. * ADD new type, protover::UnknownProtocol, so that we have greater type safety and our protover functionality which works with unsanitised protocol names is more clearly demarcated. * REFACTOR protover::Proto, renaming it protover::Protocol to mirror the new protover::UnknownProtocol type name. * ADD a utility conversion of `impl From<Protocol> for UnknownProtocol` so that we can easily with known protocols and unknown protocols simultaneously (e.g. doing comparisons, checking their version numbers), while not allowing UnknownProtocols to be accidentally used in functions which should only take Protocols. * FIXES part of #24031: https://bugs.torproject.org/24031 --- src/rust/protover/ffi.rs | 30 ++++++++++++------------ src/rust/protover/protover.rs | 53 +++++++++++++++++++++++++++++++------------ 2 files changed, 55 insertions(+), 28 deletions(-) diff --git a/src/rust/protover/ffi.rs b/src/rust/protover/ffi.rs index 2ee0286ec..ce2837832 100644 --- a/src/rust/protover/ffi.rs +++ b/src/rust/protover/ffi.rs @@ -9,30 +9,32 @@ use libc::{c_char, c_int, uint32_t}; use std::ffi::CStr; use std::ffi::CString; -use protover::*; use smartlist::*; use tor_allocate::allocate_and_copy_string; use tor_util::strings::byte_slice_is_c_like; use tor_util::strings::empty_static_cstr; +use errors::ProtoverError; +use protover::*; + /// Translate C enums to Rust Proto enums, using the integer value of the C -/// enum to map to its associated Rust enum +/// enum to map to its associated Rust enum. /// /// C_RUST_COUPLED: src/or/protover.h `protocol_type_t` -fn translate_to_rust(c_proto: uint32_t) -> Result<Proto, &'static str> { +fn translate_to_rust(c_proto: uint32_t) -> Result<Protocol, ProtoverError> { match c_proto { - 0 => Ok(Proto::Link), - 1 => Ok(Proto::LinkAuth), - 2 => Ok(Proto::Relay), - 3 => Ok(Proto::DirCache), - 4 => Ok(Proto::HSDir), - 5 => Ok(Proto::HSIntro), - 6 => Ok(Proto::HSRend), - 7 => Ok(Proto::Desc), - 8 => Ok(Proto::Microdesc), - 9 => Ok(Proto::Cons), - _ => Err("Invalid protocol type"), + 0 => Ok(Protocol::Link), + 1 => Ok(Protocol::LinkAuth), + 2 => Ok(Protocol::Relay), + 3 => Ok(Protocol::DirCache), + 4 => Ok(Protocol::HSDir), + 5 => Ok(Protocol::HSIntro), + 6 => Ok(Protocol::HSRend), + 7 => Ok(Protocol::Desc), + 8 => Ok(Protocol::Microdesc), + 9 => Ok(Protocol::Cons), + _ => Err(ProtoverError::UnknownProtocol), } } diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs index d74ca00c1..1ccad4af4 100644 --- a/src/rust/protover/protover.rs +++ b/src/rust/protover/protover.rs @@ -56,8 +56,8 @@ pub(crate) const SUPPORTED_PROTOCOLS: &'static [u8] = /// Known subprotocols in Tor. Indicates which subprotocol a relay supports. /// /// C_RUST_COUPLED: src/or/protover.h `protocol_type_t` -#[derive(Hash, Eq, PartialEq, Debug)] -pub enum Proto { +#[derive(Clone, Hash, Eq, PartialEq, Debug)] +pub enum Protocol { Cons, Desc, DirCache, @@ -70,7 +70,7 @@ pub enum Proto { Relay, } -impl fmt::Display for Proto { +impl fmt::Display for Protocol { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!(f, "{:?}", self) } @@ -80,26 +80,51 @@ impl fmt::Display for Proto { /// Error if the string is an unrecognized protocol name. /// /// C_RUST_COUPLED: src/or/protover.c `PROTOCOL_NAMES` -impl FromStr for Proto { +impl FromStr for Protocol { type Err = ProtoverError; fn from_str(s: &str) -> Result<Self, Self::Err> { match s { - "Cons" => Ok(Proto::Cons), - "Desc" => Ok(Proto::Desc), - "DirCache" => Ok(Proto::DirCache), - "HSDir" => Ok(Proto::HSDir), - "HSIntro" => Ok(Proto::HSIntro), - "HSRend" => Ok(Proto::HSRend), - "Link" => Ok(Proto::Link), - "LinkAuth" => Ok(Proto::LinkAuth), - "Microdesc" => Ok(Proto::Microdesc), - "Relay" => Ok(Proto::Relay), + "Cons" => Ok(Protocol::Cons), + "Desc" => Ok(Protocol::Desc), + "DirCache" => Ok(Protocol::DirCache), + "HSDir" => Ok(Protocol::HSDir), + "HSIntro" => Ok(Protocol::HSIntro), + "HSRend" => Ok(Protocol::HSRend), + "Link" => Ok(Protocol::Link), + "LinkAuth" => Ok(Protocol::LinkAuth), + "Microdesc" => Ok(Protocol::Microdesc), + "Relay" => Ok(Protocol::Relay), _ => Err(ProtoverError::UnknownProtocol), } } } +/// A protocol string which is not one of the `Protocols` we currently know +/// about. +#[derive(Clone, Debug, Hash, Eq, PartialEq)] +pub struct UnknownProtocol(String); + +impl fmt::Display for UnknownProtocol { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "{}", self.0) + } +} + +impl FromStr for UnknownProtocol { + type Err = ProtoverError; + + fn from_str(s: &str) -> Result<Self, Self::Err> { + Ok(UnknownProtocol(s.to_string())) + } +} + +impl From<Protocol> for UnknownProtocol { + fn from(p: Protocol) -> UnknownProtocol { + UnknownProtocol(p.to_string()) + } +} + /// Get the string representation of current supported protocols /// /// # Returns

1 0

[tor/master] tests: Run all existing protover tests in both languages.
by nickm＠torproject.org 03 Apr '18

03 Apr '18

commit 6739a69c590a12af0f1cb2af62972f4305803670 Author: Isis Lovecruft <isis(a)torproject.org> Date: Tue Mar 27 21:36:10 2018 +0000 tests: Run all existing protover tests in both languages. There's now no difference in these tests w.r.t. the C or Rust: both fail miserably (well, Rust fails with nice descriptive errors, and C gives you a traceback, because, well, C). --- src/test/test_protover.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/test/test_protover.c b/src/test/test_protover.c index e7e17efe3..7bf1471eb 100644 --- a/src/test/test_protover.c +++ b/src/test/test_protover.c @@ -283,23 +283,22 @@ test_protover_all_supported(void *arg) tt_str_op(msg, OP_EQ, "Sleen=0-4294967294"); tor_free(msg); - /* If we get an unparseable list, we say "yes, that's supported." */ -#ifndef HAVE_RUST - // XXXX let's make this section unconditional: rust should behave the - // XXXX same as C here! + /* If we get a (barely) valid (but unsupported list, we say "yes, that's + * supported." */ + tt_assert(protover_all_supported("Fribble=", &msg)); + tt_ptr_op(msg, OP_EQ, NULL); + + /* If we get a completely unparseable list, protover_all_supported should + * hit a fatal assertion for BUG(entries == NULL). */ tor_capture_bugs_(1); tt_assert(protover_all_supported("Fribble", &msg)); - tt_ptr_op(msg, OP_EQ, NULL); tor_end_capture_bugs_(); - /* This case is forbidden. Since it came from a protover_all_supported, - * it can trigger a bug message. */ + /* If we get a completely unparseable list, protover_all_supported should + * hit a fatal assertion for BUG(entries == NULL). */ tor_capture_bugs_(1); tt_assert(protover_all_supported("Sleen=0-4294967295", &msg)); - tt_ptr_op(msg, OP_EQ, NULL); - tor_free(msg); tor_end_capture_bugs_(); -#endif done: tor_end_capture_bugs_();

1 0

[tor/master] Fix bug24031 changes file
by nickm＠torproject.org 03 Apr '18

03 Apr '18

commit 21c81348a39dd235c40656c34abb76daf88e81f3 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Apr 3 19:03:33 2018 -0400 Fix bug24031 changes file --- changes/bug24031 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changes/bug24031 b/changes/bug24031 index adffa46d8..2bb0e8309 100644 --- a/changes/bug24031 +++ b/changes/bug24031 @@ -10,4 +10,4 @@ in outside crates and avoid mistakenly passing an internal error string to C over the FFI boundary. Many tests were added, and some previous differences between the C and Rust implementations have been - remedied. Fixes 24031; bugfix on 0.3.3.1-alpha. + remedied. Fixes bug 24031; bugfix on 0.3.3.1-alpha.

1 0

[tor/release-0.3.3] rust: Implement more memory-efficient protover datatype.
by nickm＠torproject.org 03 Apr '18

03 Apr '18

commit e6625113c98c281b0a649598d7daa347c28915e9 Author: Isis Lovecruft <isis(a)torproject.org> Date: Wed Mar 21 00:43:55 2018 +0000 rust: Implement more memory-efficient protover datatype. * ADD new protover::protoset module. * ADD new protover::protoset::ProtoSet class for holding protover versions. * REMOVE protover::Versions type implementation and its method `from_version_string()`, and instead implement this behaviour in a more rust-like manner as `impl FromStr for ProtoSet`. * MOVE the `find_range()` utility function from protover::protover to protover::protoset since it's only used internally in the implementation of ProtoSet. * REMOVE the `contract_protocol_list()` function from protover::protover and instead refactor it (reusing nearly the entire thing, with minor superficial, i.e. non-behavioural, changes) into a more rusty `impl ToString for ProtoSet`. * REMOVE the `expand_version_range()` function from protover::protover and instead refactor it into a more rusty implementation of `impl Into<Vec<Version>> for ProtoSet` using the new error types in protover::errors. * FIXES part of #24031: https://bugs.torproject.org/24031. --- src/rust/protover/lib.rs | 1 + src/rust/protover/protoset.rs | 634 ++++++++++++++++++++++++++++++++++++++++++ src/rust/protover/protover.rs | 215 +------------- 3 files changed, 641 insertions(+), 209 deletions(-) diff --git a/src/rust/protover/lib.rs b/src/rust/protover/lib.rs index 371d1ae58..483260bca 100644 --- a/src/rust/protover/lib.rs +++ b/src/rust/protover/lib.rs @@ -31,6 +31,7 @@ extern crate tor_allocate; extern crate tor_util; pub mod errors; +pub mod protoset; mod protover; pub mod ffi; diff --git a/src/rust/protover/protoset.rs b/src/rust/protover/protoset.rs new file mode 100644 index 000000000..f94e6299c --- /dev/null +++ b/src/rust/protover/protoset.rs @@ -0,0 +1,634 @@ +// Copyright (c) 2018, The Tor Project, Inc. +// Copyright (c) 2018, isis agora lovecruft +// See LICENSE for licensing information + +//! Sets for lazily storing ordered, non-overlapping ranges of integers. + +use std::slice; +use std::str::FromStr; +use std::u32; + +use protover::MAX_PROTOCOLS_TO_EXPAND; +use errors::ProtoverError; + +/// A single version number. +pub type Version = u32; + +/// A `ProtoSet` stores an ordered `Vec<T>` of `(low, high)` pairs of ranges of +/// non-overlapping protocol versions. +/// +/// # Examples +/// +/// ``` +/// use std::str::FromStr; +/// +/// use protover::errors::ProtoverError; +/// use protover::protoset::ProtoSet; +/// use protover::protoset::Version; +/// +/// # fn do_test() -> Result<ProtoSet, ProtoverError> { +/// let protoset: ProtoSet = ProtoSet::from_str("3-5,8")?; +/// +/// // We could also equivalently call: +/// let protoset: ProtoSet = "3-5,8".parse()?; +/// +/// assert!(protoset.contains(&4)); +/// assert!(!protoset.contains(&7)); +/// +/// let expanded: Vec<Version> = protoset.clone().into(); +/// +/// assert_eq!(&expanded[..], &[3, 4, 5, 8]); +/// +/// let contracted: String = protoset.clone().to_string(); +/// +/// assert_eq!(contracted, "3-5,8".to_string()); +/// # Ok(protoset) +/// # } +/// # fn main() { do_test(); } // wrap the test so we can use the ? operator +#[derive(Clone, Debug, Eq, PartialEq, Hash)] +pub struct ProtoSet { + pub(crate) pairs: Vec<(Version, Version)>, +} + +impl Default for ProtoSet { + fn default() -> Self { + let pairs: Vec<(Version, Version)> = Vec::new(); + + ProtoSet{ pairs } + } +} + +impl<'a> ProtoSet { + /// Create a new `ProtoSet` from a slice of `(low, high)` pairs. + /// + /// # Inputs + /// + /// We do not assume the input pairs are deduplicated or ordered. + pub fn from_slice(low_high_pairs: &'a [(Version, Version)]) -> Result<Self, ProtoverError> { + let mut pairs: Vec<(Version, Version)> = Vec::with_capacity(low_high_pairs.len()); + + for &(low, high) in low_high_pairs { + pairs.push((low, high)); + } + // Sort the pairs without reallocation and remove all duplicate pairs. + pairs.sort_unstable(); + pairs.dedup(); + + ProtoSet{ pairs }.is_ok() + } +} + +/// Expand this `ProtoSet` to a `Vec` of all its `Version`s. +/// +/// # Examples +/// +/// ``` +/// use std::str::FromStr; +/// use protover::protoset::ProtoSet; +/// use protover::protoset::Version; +/// # use protover::errors::ProtoverError; +/// +/// # fn do_test() -> Result<Vec<Version>, ProtoverError> { +/// let protoset: ProtoSet = ProtoSet::from_str("3-5,21")?; +/// let versions: Vec<Version> = protoset.into(); +/// +/// assert_eq!(&versions[..], &[3, 4, 5, 21]); +/// # +/// # Ok(versions) +/// # } +/// # fn main() { do_test(); } // wrap the test so we can use the ? operator +/// ``` +impl Into<Vec<Version>> for ProtoSet { + fn into(self) -> Vec<Version> { + let mut versions: Vec<Version> = Vec::new(); + + for &(low, high) in self.iter() { + versions.extend(low..high + 1); + } + versions + } +} + +impl ProtoSet { + /// Get an iterator over the `(low, high)` `pairs` in this `ProtoSet`. + pub fn iter(&self) -> slice::Iter<(Version, Version)> { + self.pairs.iter() + } + + /// Expand this `ProtoSet` into a `Vec` of all its `Version`s. + /// + /// # Examples + /// + /// ``` + /// # use protover::errors::ProtoverError; + /// use protover::protoset::ProtoSet; + /// + /// # fn do_test() -> Result<bool, ProtoverError> { + /// let protoset: ProtoSet = "3-5,9".parse()?; + /// + /// assert_eq!(protoset.expand(), vec![3, 4, 5, 9]); + /// + /// let protoset: ProtoSet = "1,3,5-7".parse()?; + /// + /// assert_eq!(protoset.expand(), vec![1, 3, 5, 6, 7]); + /// # + /// # Ok(true) + /// # } + /// # fn main() { do_test(); } // wrap the test so we can use the ? operator + /// ``` + pub fn expand(self) -> Vec<Version> { + self.into() + } + + pub fn len(&self) -> usize { + let mut length: usize = 0; + + for &(low, high) in self.iter() { + length += (high as usize - low as usize) + 1; + } + + length + } + + /// Check that this `ProtoSet` is well-formed. + /// + /// This is automatically called in `ProtoSet::from_str()`. + /// + /// # Errors + /// + /// * `ProtoverError::LowGreaterThanHigh`: if its `pairs` were not + /// well-formed, i.e. a `low` in a `(low, high)` was higher than the + /// previous `high`, + /// * `ProtoverError::Overlap`: if one or more of the `pairs` are + /// overlapping, + /// * `ProtoverError::ExceedsMax`: if the number of versions when expanded + /// would exceed `MAX_PROTOCOLS_TO_EXPAND`, and + /// + /// # Returns + /// + /// A `Result` whose `Ok` is this `Protoset`, and whose `Err` is one of the + /// errors enumerated in the Errors section above. + fn is_ok(self) -> Result<ProtoSet, ProtoverError> { + let mut last_high: Version = 0; + + for &(low, high) in self.iter() { + if low == u32::MAX || high == u32::MAX { + return Err(ProtoverError::ExceedsMax); + } + if low < last_high { + return Err(ProtoverError::Overlap); + } else if low > high { + return Err(ProtoverError::LowGreaterThanHigh); + } + last_high = high; + } + + if self.len() > MAX_PROTOCOLS_TO_EXPAND { + return Err(ProtoverError::ExceedsMax); + } + + Ok(self) + } + + /// Determine if this `ProtoSet` contains no `Version`s. + /// + /// # Returns + /// + /// * `true` if this `ProtoSet`'s length is zero, and + /// * `false` otherwise. + /// + /// # Examples + /// + /// ``` + /// use protover::protoset::ProtoSet; + /// + /// let protoset: ProtoSet = ProtoSet::default(); + /// + /// assert!(protoset.is_empty()); + /// ``` + pub fn is_empty(&self) -> bool { + self.pairs.len() == 0 + } + + /// Determine if `version` is included within this `ProtoSet`. + /// + /// # Inputs + /// + /// * `version`: a `Version`. + /// + /// # Returns + /// + /// `true` if the `version` is contained within this set; `false` otherwise. + /// + /// # Examples + /// + /// ``` + /// # use protover::errors::ProtoverError; + /// use protover::protoset::ProtoSet; + /// + /// # fn do_test() -> Result<ProtoSet, ProtoverError> { + /// let protoset: ProtoSet = ProtoSet::from_slice(&[(0, 5), (7, 9), (13, 14)])?; + /// + /// assert!(protoset.contains(&5)); + /// assert!(!protoset.contains(&10)); + /// # + /// # Ok(protoset) + /// # } + /// # fn main() { do_test(); } // wrap the test so we can use the ? operator + /// ``` + pub fn contains(&self, version: &Version) -> bool { + for &(low, high) in self.iter() { + if low <= *version && *version <= high { + return true; + } + } + false + } + + /// Retain only the `Version`s in this `ProtoSet` for which the predicate + /// `F` returns `true`. + /// + /// # Examples + /// + /// ``` + /// # use protover::errors::ProtoverError; + /// use protover::protoset::ProtoSet; + /// + /// # fn do_test() -> Result<bool, ProtoverError> { + /// let mut protoset: ProtoSet = "1,3-5,9".parse()?; + /// + /// // Keep only versions less than or equal to 8: + /// protoset.retain(|x| x <= &8); + /// + /// assert_eq!(protoset.expand(), vec![1, 3, 4, 5]); + /// # + /// # Ok(true) + /// # } + /// # fn main() { do_test(); } // wrap the test so we can use the ? operator + /// ``` + // XXX we could probably do something more efficient here. —isis + pub fn retain<F>(&mut self, f: F) + where F: FnMut(&Version) -> bool + { + let mut expanded: Vec<Version> = self.clone().expand(); + expanded.retain(f); + *self = expanded.into(); + } +} + +impl FromStr for ProtoSet { + type Err = ProtoverError; + + /// Parse the unique version numbers supported by a subprotocol from a string. + /// + /// # Inputs + /// + /// * `version_string`, a string comprised of "[0-9,-]" + /// + /// # Returns + /// + /// A `Result` whose `Ok` value is a `ProtoSet` holding all of the unique + /// version numbers. + /// + /// The returned `Result`'s `Err` value is an `ProtoverError` appropriate to + /// the error. + /// + /// # Errors + /// + /// This function will error if: + /// + /// * the `version_string` is an equals (`"="`) sign, + /// * the expansion of a version range produces an error (see + /// `expand_version_range`), + /// * any single version number is not parseable as an `u32` in radix 10, or + /// * there are greater than 2^16 version numbers to expand. + /// + /// # Examples + /// + /// ``` + /// use std::str::FromStr; + /// + /// use protover::errors::ProtoverError; + /// use protover::protoset::ProtoSet; + /// + /// # fn do_test() -> Result<ProtoSet, ProtoverError> { + /// let protoset: ProtoSet = ProtoSet::from_str("2-5,8")?; + /// + /// assert!(protoset.contains(&5)); + /// assert!(!protoset.contains(&10)); + /// + /// // We can also equivalently call `ProtoSet::from_str` by doing: + /// let protoset: ProtoSet = "4-6,12".parse()?; + /// + /// // There are lots of ways to get an `Err` from this function. Here are + /// // a few: + /// assert_eq!(Err(ProtoverError::Unparseable), ProtoSet::from_str("=")); + /// assert_eq!(Err(ProtoverError::Unparseable), ProtoSet::from_str("-")); + /// assert_eq!(Err(ProtoverError::Unparseable), ProtoSet::from_str("not_an_int")); + /// assert_eq!(Err(ProtoverError::Unparseable), ProtoSet::from_str("3-")); + /// assert_eq!(Err(ProtoverError::Unparseable), ProtoSet::from_str("1-,4")); + /// assert_eq!(Err(ProtoverError::ExceedsMax), ProtoSet::from_str("1-70000")); + /// + /// // Things which would get parsed into an _empty_ `ProtoSet` are, + /// // however, legal, and result in an empty `ProtoSet`: + /// assert_eq!(Ok(ProtoSet::default()), ProtoSet::from_str("")); + /// assert_eq!(Ok(ProtoSet::default()), ProtoSet::from_str(",,,")); + /// # + /// # Ok(protoset) + /// # } + /// # fn main() { do_test(); } // wrap the test so we can use the ? operator + /// ``` + fn from_str(version_string: &str) -> Result<Self, Self::Err> { + let mut pairs: Vec<(Version, Version)> = Vec::new(); + let pieces: ::std::str::Split<char> = version_string.trim().split(','); + + for piece in pieces { + let p: &str = piece.trim(); + + if p.is_empty() { + continue; + } else if p.contains('-') { + let mut pair = p.split('-'); + + let low = pair.next().ok_or(ProtoverError::Unparseable)?; + let high = pair.next().ok_or(ProtoverError::Unparseable)?; + + let lo: Version = low.parse().or(Err(ProtoverError::Unparseable))?; + let hi: Version = high.parse().or(Err(ProtoverError::Unparseable))?; + + if lo == u32::MAX || hi == u32::MAX { + return Err(ProtoverError::ExceedsMax); + } + pairs.push((lo, hi)); + } else { + let v: u32 = p.parse().or(Err(ProtoverError::Unparseable))?; + + if v == u32::MAX { + return Err(ProtoverError::ExceedsMax); + } + pairs.push((v, v)); + } + } + // If we were passed in an empty string, or a bunch of whitespace, or + // simply a comma, or a pile of commas, then return an empty ProtoSet. + if pairs.len() == 0 { + return Ok(ProtoSet::default()); + } + ProtoSet::from_slice(&pairs[..]) + } +} + +impl ToString for ProtoSet { + /// Contracts a `ProtoSet` of versions into a string. + /// + /// # Returns + /// + /// A `String` representation of this `ProtoSet` in ascending order. + fn to_string(&self) -> String { + let mut final_output: Vec<String> = Vec::new(); + + for &(lo, hi) in self.iter() { + if lo != hi { + debug_assert!(lo < hi); + final_output.push(format!("{}-{}", lo, hi)); + } else { + final_output.push(format!("{}", lo)); + } + } + final_output.join(",") + } +} + +/// Checks to see if there is a continuous range of integers, starting at the +/// first in the list. Returns the last integer in the range if a range exists. +/// +/// # Inputs +/// +/// `list`, an ordered vector of `u32` integers of "[0-9,-]" representing the +/// supported versions for a single protocol. +/// +/// # Returns +/// +/// A `bool` indicating whether the list contains a range, starting at the first +/// in the list, a`Version` of the last integer in the range, and a `usize` of +/// the index of that version. +/// +/// For example, if given vec![1, 2, 3, 5], find_range will return true, +/// as there is a continuous range, and 3, which is the last number in the +/// continuous range, and 2 which is the index of 3. +fn find_range(list: &Vec<Version>) -> (bool, Version, usize) { + if list.len() == 0 { + return (false, 0, 0); + } + + let mut index: usize = 0; + let mut iterable = list.iter().peekable(); + let mut range_end = match iterable.next() { + Some(n) => *n, + None => return (false, 0, 0), + }; + + let mut has_range = false; + + while iterable.peek().is_some() { + let n = *iterable.next().unwrap(); + if n != range_end + 1 { + break; + } + + has_range = true; + range_end = n; + index += 1; + } + + (has_range, range_end, index) +} + +impl From<Vec<Version>> for ProtoSet { + fn from(mut v: Vec<Version>) -> ProtoSet { + let mut version_pairs: Vec<(Version, Version)> = Vec::new(); + + v.sort_unstable(); + v.dedup(); + + 'vector: while !v.is_empty() { + let (has_range, end, index): (bool, Version, usize) = find_range(&v); + + if has_range { + let first: Version = match v.first() { + Some(x) => *x, + None => continue, + }; + let last: Version = match v.get(index) { + Some(x) => *x, + None => continue, + }; + debug_assert!(last == end, format!("last = {}, end = {}", last, end)); + + version_pairs.push((first, last)); + v = v.split_off(index + 1); + + if v.len() == 0 { + break 'vector; + } + } else { + let last: Version = match v.get(index) { + Some(x) => *x, + None => continue, + }; + version_pairs.push((last, last)); + v.remove(index); + } + } + ProtoSet::from_slice(&version_pairs[..]).unwrap_or(ProtoSet::default()) + } +} + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn test_find_range() { + assert_eq!((false, 0, 0), find_range(&vec![])); + assert_eq!((false, 1, 0), find_range(&vec![1])); + assert_eq!((true, 2, 1), find_range(&vec![1, 2])); + assert_eq!((true, 3, 2), find_range(&vec![1, 2, 3])); + assert_eq!((true, 3, 2), find_range(&vec![1, 2, 3, 5])); + } + + macro_rules! assert_contains_each { + ($protoset:expr, $versions:expr) => ( + for version in $versions { + assert!($protoset.contains(version)); + } + ) + } + + macro_rules! test_protoset_contains_versions { + ($list:expr, $str:expr) => ( + let versions: &[Version] = $list; + let protoset: Result<ProtoSet, ProtoverError> = ProtoSet::from_str($str); + + assert!(protoset.is_ok()); + let p = protoset.unwrap(); + assert_contains_each!(p, versions); + ) + } + + #[test] + fn test_versions_from_str() { + test_protoset_contains_versions!(&[], ""); + test_protoset_contains_versions!(&[1], "1"); + test_protoset_contains_versions!(&[1, 2], "1,2"); + test_protoset_contains_versions!(&[1, 2, 3], "1-3"); + test_protoset_contains_versions!(&[0, 1], "0-1"); + test_protoset_contains_versions!(&[1, 2, 5], "1-2,5"); + test_protoset_contains_versions!(&[1, 3, 4, 5], "1,3-5"); + test_protoset_contains_versions!(&[42, 55, 56, 57, 58], "42,55-58"); + } + + #[test] + fn test_versions_from_str_ab() { + assert_eq!(Err(ProtoverError::Unparseable), ProtoSet::from_str("a,b")); + } + + #[test] + fn test_versions_from_str_negative_1() { + assert_eq!(Err(ProtoverError::Unparseable), ProtoSet::from_str("-1")); + } + + #[test] + fn test_versions_from_str_1exclam() { + assert_eq!(Err(ProtoverError::Unparseable), ProtoSet::from_str("1,!")); + } + + #[test] + fn test_versions_from_str_percent_equal() { + assert_eq!(Err(ProtoverError::Unparseable), ProtoSet::from_str("%=")); + } + + #[test] + fn test_versions_from_str_overlap() { + assert_eq!(Err(ProtoverError::Overlap), ProtoSet::from_str("1-3,2-4")); + } + + #[test] + fn test_versions_from_slice_overlap() { + assert_eq!(Err(ProtoverError::Overlap), ProtoSet::from_slice(&[(1, 3), (2, 4)])); + } + + #[test] + fn test_versions_from_str_max() { + assert_eq!(Err(ProtoverError::ExceedsMax), ProtoSet::from_str("4294967295")); + } + + #[test] + fn test_versions_from_slice_max() { + assert_eq!(Err(ProtoverError::ExceedsMax), ProtoSet::from_slice(&[(4294967295, 4294967295)])); + } + + #[test] + fn test_protoset_contains() { + let protoset: ProtoSet = ProtoSet::from_slice(&[(0, 5), (7, 9), (13, 14)]).unwrap(); + + for x in 0..6 { assert!(protoset.contains(&x), format!("should contain {}", x)); } + for x in 7..10 { assert!(protoset.contains(&x), format!("should contain {}", x)); } + for x in 13..15 { assert!(protoset.contains(&x), format!("should contain {}", x)); } + + for x in [6, 10, 11, 12, 15, 42, 43, 44, 45, 1234584].iter() { + assert!(!protoset.contains(&x), format!("should not contain {}", x)); + } + } + + #[test] + fn test_protoset_contains_0_3() { + let protoset: ProtoSet = ProtoSet::from_slice(&[(0, 3)]).unwrap(); + + for x in 0..4 { assert!(protoset.contains(&x), format!("should contain {}", x)); } + } + + macro_rules! assert_protoset_from_vec_contains_all { + ($($x:expr),*) => ( + let vec: Vec<Version> = vec!($($x),*); + let protoset: ProtoSet = vec.clone().into(); + + for x in vec.iter() { + assert!(protoset.contains(&x)); + } + ) + } + + #[test] + fn test_protoset_from_vec_123() { + assert_protoset_from_vec_contains_all!(1, 2, 3); + } + + #[test] + fn test_protoset_from_vec_0_315() { + assert_protoset_from_vec_contains_all!(0, 1, 2, 3, 15); + } + + #[test] + fn test_protoset_from_vec_unordered() { + let v: Vec<Version> = vec!(2, 3, 8, 4, 3, 9, 7, 2); + let ps: ProtoSet = v.into(); + + assert_eq!(ps.to_string(), "2-4,7-9"); + } + + #[test] + fn test_protoset_into_vec() { + let ps: ProtoSet = "1-13,42,9001,4294967294".parse().unwrap(); + let v: Vec<Version> = ps.into(); + + assert!(v.contains(&7)); + assert!(v.contains(&9001)); + assert!(v.contains(&4294967294)); + } +} + +#[cfg(all(test, feature = "bench"))] +mod bench { + use super::*; +} diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs index 8ce182cf1..d74ca00c1 100644 --- a/src/rust/protover/protover.rs +++ b/src/rust/protover/protover.rs @@ -14,6 +14,7 @@ use std::u32; use tor_util::strings::NUL_BYTE; use errors::ProtoverError; +use protoset::Version; /// The first version of Tor that included "proto" entries in its descriptors. /// Authorities should use this to decide whether to guess proto lines. @@ -26,7 +27,7 @@ const FIRST_TOR_VERSION_TO_ADVERTISE_PROTOCOLS: &'static str = "0.2.9.3-alpha"; /// before concluding that someone is trying to DoS us /// /// C_RUST_COUPLED: src/or/protover.c `MAX_PROTOCOLS_TO_EXPAND` -const MAX_PROTOCOLS_TO_EXPAND: usize = (1<<16); +pub(crate) const MAX_PROTOCOLS_TO_EXPAND: usize = (1<<16); /// Currently supported protocols and their versions, as a byte-slice. /// @@ -159,70 +160,6 @@ impl SupportedProtocols { } } -type Version = u32; - -/// Set of versions for a protocol. -#[derive(Debug, PartialEq, Eq)] -pub struct Versions(HashSet<Version>); - -impl Versions { - /// Get the unique version numbers supported by a subprotocol. - /// - /// # Inputs - /// - /// * `version_string`, a string comprised of "[0-9,-]" - /// - /// # Returns - /// - /// A `Result` whose `Ok` value is a `HashSet<u32>` holding all of the unique - /// version numbers. If there were ranges in the `version_string`, then these - /// are expanded, i.e. `"1-3"` would expand to `HashSet<u32>::new([1, 2, 3])`. - /// The returned HashSet is *unordered*. - /// - /// The returned `Result`'s `Err` value is an `&'static str` with a description - /// of the error. - /// - /// # Errors - /// - /// This function will error if: - /// - /// * the `version_string` is empty or contains an equals (`"="`) sign, - /// * the expansion of a version range produces an error (see - /// `expand_version_range`), - /// * any single version number is not parseable as an `u32` in radix 10, or - /// * there are greater than 2^16 version numbers to expand. - /// - fn from_version_string( - version_string: &str, - ) -> Result<Self, &'static str> { - let mut versions = HashSet::<Version>::new(); - - for piece in version_string.split(",") { - if piece.contains("-") { - for p in expand_version_range(piece)? { - versions.insert(p); - } - } else if piece == "" { - continue; - } else { - let v = u32::from_str(piece).or( - Err("invalid protocol entry"), - )?; - if v == u32::MAX { - return Err("invalid protocol entry"); - } - versions.insert(v); - } - - if versions.len() > MAX_PROTOCOLS_TO_EXPAND { - return Err("Too many versions to expand"); - } - } - Ok(Versions(versions)) - } -} - - /// Parse the subprotocol type and its version numbers. /// /// # Inputs @@ -409,149 +346,6 @@ pub fn protover_string_supports_protocol_or_later( supported_versions.0.iter().any(|v| v >= &vers) } -/// Fully expand a version range. For example, 1-3 expands to 1,2,3 -/// Helper for Versions::from_version_string -/// -/// # Inputs -/// -/// `range`, a string comprised of "[0-9,-]" -/// -/// # Returns -/// -/// A `Result` whose `Ok` value a vector of unsigned integers representing the -/// expanded range of supported versions by a single protocol. -/// Otherwise, the `Err` value of this `Result` is a description of the error -/// -/// # Errors -/// -/// This function will error if: -/// -/// * the specified range is empty -/// * the version range does not contain both a valid lower and upper bound. -/// -fn expand_version_range(range: &str) -> Result<Range<u32>, &'static str> { - if range.is_empty() { - return Err("version string empty"); - } - - let mut parts = range.split("-"); - - let lower_string = parts.next().ok_or( - "cannot parse protocol range lower bound", - )?; - - let lower = u32::from_str_radix(lower_string, 10).or(Err( - "cannot parse protocol range lower bound", - ))?; - - let higher_string = parts.next().ok_or( - "cannot parse protocol range upper bound", - )?; - - let higher = u32::from_str_radix(higher_string, 10).or(Err( - "cannot parse protocol range upper bound", - ))?; - - if lower == u32::MAX || higher == u32::MAX { - return Err("protocol range value out of range"); - } - - if lower > higher { - return Err("protocol range is badly formed"); - } - - // We can use inclusive range syntax when it becomes stable. - let result = lower..higher + 1; - - if result.len() > MAX_PROTOCOLS_TO_EXPAND { - Err("Too many protocols in expanded range") - } else { - Ok(result) - } -} - -/// Checks to see if there is a continuous range of integers, starting at the -/// first in the list. Returns the last integer in the range if a range exists. -/// Helper for compute_vote -/// -/// # Inputs -/// -/// `list`, an ordered vector of `u32` integers of "[0-9,-]" representing the -/// supported versions for a single protocol. -/// -/// # Returns -/// -/// A `bool` indicating whether the list contains a range, starting at the -/// first in the list, and an `u32` of the last integer in the range. -/// -/// For example, if given vec![1, 2, 3, 5], find_range will return true, -/// as there is a continuous range, and 3, which is the last number in the -/// continuous range. -/// -fn find_range(list: &Vec<u32>) -> (bool, u32) { - if list.len() == 0 { - return (false, 0); - } - - let mut iterable = list.iter().peekable(); - let mut range_end = match iterable.next() { - Some(n) => *n, - None => return (false, 0), - }; - - let mut has_range = false; - - while iterable.peek().is_some() { - let n = *iterable.next().unwrap(); - if n != range_end + 1 { - break; - } - - has_range = true; - range_end = n; - } - - (has_range, range_end) -} - -/// Contracts a HashSet representation of supported versions into a string. -/// Helper for compute_vote -/// -/// # Inputs -/// -/// `supported_set`, a set of integers of "[0-9,-]" representing the -/// supported versions for a single protocol. -/// -/// # Returns -/// -/// A `String` representation of this set in ascending order. -/// -fn contract_protocol_list<'a>(supported_set: &'a HashSet<Version>) -> String { - let mut supported: Vec<Version> = - supported_set.iter().map(|x| *x).collect(); - supported.sort(); - - let mut final_output: Vec<String> = Vec::new(); - - while supported.len() != 0 { - let (has_range, end) = find_range(&supported); - let current = supported.remove(0); - - if has_range { - final_output.push(format!( - "{}-{}", - current.to_string(), - &end.to_string(), - )); - supported.retain(|&x| x > end); - } else { - final_output.push(current.to_string()); - } - } - - final_output.join(",") -} - /// Parses a protocol list without validating the protocol names /// /// # Inputs @@ -790,7 +584,10 @@ pub fn compute_for_old_tor(version: &str) -> &'static [u8] { #[cfg(test)] mod test { - use super::Version; + use std::str::FromStr; + use std::string::ToString; + + use super::*; #[test] fn test_versions_from_version_string() {

1 0

[tor/release-0.3.3] rust: Add macro for `impl ToString for {Unvalidated}ProtoEntry`.
by nickm＠torproject.org 03 Apr '18

03 Apr '18

commit fa15ea104d5b9f05192afa3a023d0e2405c3842a Author: Isis Lovecruft <isis(a)torproject.org> Date: Wed Mar 21 01:44:59 2018 +0000 rust: Add macro for `impl ToString for {Unvalidated}ProtoEntry`. This implements conversions from either a ProtoEntry or an UnvalidatedProtoEntry into a String, for use in replacing such functions as `protover::write_vote_to_string()`. * ADD macro for implementing ToString trait for ProtoEntry and UnvalidatedProtoEntry. * FIXES part of #24031: https://bugs.torproject.org/24031 --- src/rust/protover/protover.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs index 847406ca2..2a1a5df9e 100644 --- a/src/rust/protover/protover.rs +++ b/src/rust/protover/protover.rs @@ -226,6 +226,27 @@ impl FromStr for ProtoEntry { } } +/// Generate an implementation of `ToString` for either a `ProtoEntry` or an +/// `UnvalidatedProtoEntry`. +macro_rules! impl_to_string_for_proto_entry { + ($t:ty) => ( + impl ToString for $t { + fn to_string(&self) -> String { + let mut parts: Vec<String> = Vec::new(); + + for (protocol, versions) in self.iter() { + parts.push(format!("{}={}", protocol.to_string(), versions.to_string())); + } + parts.sort_unstable(); + parts.join(" ") + } + } + ) +} + +impl_to_string_for_proto_entry!(ProtoEntry); +impl_to_string_for_proto_entry!(UnvalidatedProtoEntry); + /// A `ProtoEntry`, but whose `Protocols` can be any `UnknownProtocol`, not just /// the supported ones enumerated in `Protocols`. The protocol versions are /// validated, however.

1 0

[tor/release-0.3.3] rust: Implement error types for Rust protover implementation.
by nickm＠torproject.org 03 Apr '18

03 Apr '18

commit 88204f91df3e01b69e79b89ba029b42a4025d11f Author: Isis Lovecruft <isis(a)torproject.org> Date: Wed Mar 21 00:24:46 2018 +0000 rust: Implement error types for Rust protover implementation. This will allow us to do actual error handling intra-crate in a more rusty manner, e.g. propogating errors in match statements, conversion between error types, logging messages, etc. * FIXES part of #24031: https://bugs.torproject.org/24031. --- src/rust/protover/errors.rs | 43 +++++++++++++++++++++++++++++++++++++++++++ src/rust/protover/lib.rs | 3 +++ src/rust/protover/protover.rs | 6 ++++-- 3 files changed, 50 insertions(+), 2 deletions(-) diff --git a/src/rust/protover/errors.rs b/src/rust/protover/errors.rs new file mode 100644 index 000000000..56473d12e --- /dev/null +++ b/src/rust/protover/errors.rs @@ -0,0 +1,43 @@ +// Copyright (c) 2018, The Tor Project, Inc. +// Copyright (c) 2018, isis agora lovecruft +// See LICENSE for licensing information + +//! Various errors which may occur during protocol version parsing. + +use std::fmt; +use std::fmt::Display; + +/// All errors which may occur during protover parsing routines. +#[derive(Clone, Copy, Debug, Eq, PartialEq, Hash)] +#[allow(missing_docs)] // See Display impl for error descriptions +pub enum ProtoverError { + Overlap, + LowGreaterThanHigh, + Unparseable, + ExceedsMax, + ExceedsExpansionLimit, + UnknownProtocol, + ExceedsNameLimit, +} + +/// Descriptive error messages for `ProtoverError` variants. +impl Display for ProtoverError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match *self { + ProtoverError::Overlap + => write!(f, "Two or more (low, high) protover ranges would overlap once expanded."), + ProtoverError::LowGreaterThanHigh + => write!(f, "The low in a (low, high) protover range was greater than high."), + ProtoverError::Unparseable + => write!(f, "The protover string was unparseable."), + ProtoverError::ExceedsMax + => write!(f, "The high in a (low, high) protover range exceeds u32::MAX."), + ProtoverError::ExceedsExpansionLimit + => write!(f, "The protover string would exceed the maximum expansion limit."), + ProtoverError::UnknownProtocol + => write!(f, "A protocol in the protover string we attempted to parse is unknown."), + ProtoverError::ExceedsNameLimit + => write!(f, "An unrecognised protocol name was too long."), + } + } +} diff --git a/src/rust/protover/lib.rs b/src/rust/protover/lib.rs index fe8c0f9bb..371d1ae58 100644 --- a/src/rust/protover/lib.rs +++ b/src/rust/protover/lib.rs @@ -22,12 +22,15 @@ //! protocols to develop independently, without having to claim compatibility //! with specific versions of Tor. +#[deny(missing_docs)] + extern crate libc; extern crate smartlist; extern crate external; extern crate tor_allocate; extern crate tor_util; +pub mod errors; mod protover; pub mod ffi; diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs index e5dc69b9a..8ce182cf1 100644 --- a/src/rust/protover/protover.rs +++ b/src/rust/protover/protover.rs @@ -13,6 +13,8 @@ use std::u32; use tor_util::strings::NUL_BYTE; +use errors::ProtoverError; + /// The first version of Tor that included "proto" entries in its descriptors. /// Authorities should use this to decide whether to guess proto lines. /// @@ -78,7 +80,7 @@ impl fmt::Display for Proto { /// /// C_RUST_COUPLED: src/or/protover.c `PROTOCOL_NAMES` impl FromStr for Proto { - type Err = &'static str; + type Err = ProtoverError; fn from_str(s: &str) -> Result<Self, Self::Err> { match s { @@ -92,7 +94,7 @@ impl FromStr for Proto { "LinkAuth" => Ok(Proto::LinkAuth), "Microdesc" => Ok(Proto::Microdesc), "Relay" => Ok(Proto::Relay), - _ => Err("Not a valid protocol type"), + _ => Err(ProtoverError::UnknownProtocol), } } }

1 0