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

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

03 Apr '18

commit 52c3ea50454bcbcbcf5b18e628a1490441962b33 Author: Isis Lovecruft <isis(a)torproject.org> Date: Wed Mar 21 02:52:04 2018 +0000 rust: Refactor Rust impl of protover_list_supports_protocol(). This includes a subtle difference in behaviour, as in 4258f1e18, where we return (matching the C impl's return behaviour) earlier than before if parsing failed, saving us computation in parsing the versions into a protover::protoset::ProtoSet. * REFACTOR `protover::ffi::protover_list_supports_protocol()` to use new types and methods. --- src/rust/protover/ffi.rs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/rust/protover/ffi.rs b/src/rust/protover/ffi.rs index 1588b2e4d..0d0376e52 100644 --- a/src/rust/protover/ffi.rs +++ b/src/rust/protover/ffi.rs @@ -98,16 +98,18 @@ pub extern "C" fn protocol_list_supports_protocol( Ok(n) => n, Err(_) => return 1, }; - - let protocol = match translate_to_rust(c_protocol) { - Ok(n) => n, + let proto_entry: UnvalidatedProtoEntry = match protocol_list.parse() { + Ok(n) => n, Err(_) => return 0, }; - - let is_supported = - protover_string_supports_protocol(protocol_list, protocol, version); - - return if is_supported { 1 } else { 0 }; + let protocol: UnknownProtocol = match translate_to_rust(c_protocol) { + Ok(n) => n.into(), + Err(_) => return 0, + }; + match proto_entry.supports_protocol(&protocol, &version) { + false => return 0, + true => return 1, + } } /// Provide an interface for C to translate arguments and return types for

1 0

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

03 Apr '18

commit 0a5494b81df684109986354a3d31051b6f8d0bec Author: Isis Lovecruft <isis(a)torproject.org> Date: Wed Mar 21 02:59:25 2018 +0000 rust: Refactor Rust impl of protover_list_supports_protocol_or_later(). This includes a subtle difference in behaviour, as in 4258f1e18, where we return (matching the C impl's return behaviour) earlier than before if parsing failed, saving us computation in parsing the versions into a protover::protoset::ProtoSet. * REFACTOR `protover::ffi::protover_list_supports_protocol_or_later()` to use new types and methods. --- src/rust/protover/ffi.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/rust/protover/ffi.rs b/src/rust/protover/ffi.rs index 0d0376e52..7f8d86d04 100644 --- a/src/rust/protover/ffi.rs +++ b/src/rust/protover/ffi.rs @@ -138,13 +138,15 @@ pub extern "C" fn protocol_list_supports_protocol_or_later( Err(_) => return 0, }; - let is_supported = protover_string_supports_protocol_or_later( - protocol_list, - protocol, - version, - ); + let proto_entry: UnvalidatedProtoEntry = match protocol_list.parse() { + Ok(n) => n, + Err(_) => return 1, + }; - return if is_supported { 1 } else { 0 }; + if proto_entry.supports_protocol_or_later(&protocol.into(), &version) { + return 1; + } + 0 } /// Provide an interface for C to translate arguments and return types for

1 0

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

03 Apr '18

commit 6f252e098637bbac87b4ad99ece7fa4a7e404cdb Author: Isis Lovecruft <isis(a)torproject.org> Date: Wed Mar 21 03:05:56 2018 +0000 rust: Refactor Rust impl of protover_compute_vote(). This includes a subtle difference in behaviour to the previous Rust implementation, where, for each vote that we're computing over, if a single one fails to parse, we skip it. This now matches the current behaviour in the C implementation. * REFACTOR `protover::ffi::protover_compute_vote()` to use new types and methods. --- src/rust/protover/ffi.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/rust/protover/ffi.rs b/src/rust/protover/ffi.rs index 7f8d86d04..780fe6963 100644 --- a/src/rust/protover/ffi.rs +++ b/src/rust/protover/ffi.rs @@ -161,6 +161,8 @@ pub extern "C" fn protover_get_supported_protocols() -> *const c_char { /// Provide an interface for C to translate arguments and return types for /// protover::compute_vote +// +// Why is the threshold a signed integer? —isis #[no_mangle] pub extern "C" fn protover_compute_vote( list: *const Stringlist, @@ -175,10 +177,19 @@ pub extern "C" fn protover_compute_vote( // Dereference of raw pointer requires an unsafe block. The pointer is // checked above to ensure it is not null. let data: Vec<String> = unsafe { (*list).get_list() }; + let hold: usize = threshold as usize; + let mut proto_entries: Vec<UnvalidatedProtoEntry> = Vec::new(); - let vote = compute_vote(data, threshold); + for datum in data { + let entry: UnvalidatedProtoEntry = match datum.parse() { + Ok(x) => x, + Err(_) => continue, + }; + proto_entries.push(entry); + } + let vote: UnvalidatedProtoEntry = ProtoverVote::compute(&proto_entries, &hold); - allocate_and_copy_string(&vote) + allocate_and_copy_string(&vote.to_string()) } /// Provide an interface for C to translate arguments and return types for

1 0

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

03 Apr '18

commit 9766d53cf990432f25fb70916fe9b165f1d4a36d 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 780fe6963..44c65114e 100644 --- a/src/rust/protover/ffi.rs +++ b/src/rust/protover/ffi.rs @@ -204,7 +204,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] protover: Change protover_all_supported() to return only unsupported.
by nickm＠torproject.org 03 Apr '18

03 Apr '18

commit 6e353664dd7c7a362be82af578954797e348d298 Author: Isis Lovecruft <isis(a)torproject.org> Date: Tue Mar 27 16:59:49 2018 +0000 protover: Change protover_all_supported() to return only unsupported. Previously, if "Link=1-5" was supported, and you asked protover_all_supported() (or protover::all_supported() in Rust) if it supported "Link=3-999", the C version would return "Link=3-999" and the Rust would return "Link=6-999". These both behave the same now, i.e. both return "Link=6-999". --- src/or/protover.c | 77 +++++++++++++++++++++++++++++++++++++++++++----- src/test/test_protover.c | 17 ++++++++++- 2 files changed, 86 insertions(+), 8 deletions(-) diff --git a/src/or/protover.c b/src/or/protover.c index cb168085c..6532f09c2 100644 --- a/src/or/protover.c +++ b/src/or/protover.c @@ -671,7 +671,9 @@ int protover_all_supported(const char *s, char **missing_out) { int all_supported = 1; - smartlist_t *missing; + smartlist_t *missing_some; + smartlist_t *missing_completely; + smartlist_t *missing_all; if (!s) { return 1; @@ -684,7 +686,8 @@ protover_all_supported(const char *s, char **missing_out) return 1; } - missing = smartlist_new(); + missing_some = smartlist_new(); + missing_completely = smartlist_new(); SMARTLIST_FOREACH_BEGIN(entries, const proto_entry_t *, ent) { protocol_type_t tp; @@ -696,26 +699,86 @@ protover_all_supported(const char *s, char **missing_out) } SMARTLIST_FOREACH_BEGIN(ent->ranges, const proto_range_t *, range) { + proto_entry_t *unsupported = tor_malloc_zero(sizeof(proto_entry_t)); + proto_range_t *versions = tor_malloc_zero(sizeof(proto_range_t)); uint32_t i; + + unsupported->name = tor_strdup(ent->name); + unsupported->ranges = smartlist_new(); + for (i = range->low; i <= range->high; ++i) { if (!protover_is_supported_here(tp, i)) { - goto unsupported; + if (versions->low == 0 && versions->high == 0) { + versions->low = i; + /* Pre-emptively add the high now, just in case we're in a single + * version range (e.g. "Link=999"). */ + versions->high = i; + } + /* If the last one to be unsupported is one less than the current + * one, we're in a continous range, so set the high field. */ + if ((versions->high && versions->high == i - 1) || + /* Similarly, if the last high wasn't set and we're currently + * one higher than the low, add current index as the highest + * known high. */ + (!versions->high && versions->low == i - 1)) { + versions->high = i; + continue; + } + } else { + /* If we hit a supported version, and we previously had a range, + * we've hit a non-continuity. Copy the previous range and add it to + * the unsupported->ranges list and zero-out the previous range for + * the next iteration. */ + if (versions->low != 0 && versions->high != 0) { + proto_range_t *versions_to_add = tor_malloc(sizeof(proto_range_t)); + + versions_to_add->low = versions->low; + versions_to_add->high = versions->high; + smartlist_add(unsupported->ranges, versions_to_add); + + versions->low = 0; + versions->high = 0; + } } } + /* Once we've run out of versions to check, see if we had any unsupported + * ones and, if so, add them to unsupported->ranges. */ + if (versions->low != 0 && versions->high != 0) { + smartlist_add(unsupported->ranges, versions); + } + /* Finally, if we had something unsupported, add it to the list of + * missing_some things and mark that there was something missing. */ + if (smartlist_len(unsupported->ranges) != 0) { + smartlist_add(missing_some, (void*) unsupported); + all_supported = 0; + } else { + proto_entry_free(unsupported); + tor_free(versions); + } } SMARTLIST_FOREACH_END(range); continue; unsupported: all_supported = 0; - smartlist_add(missing, (void*) ent); + smartlist_add(missing_completely, (void*) ent); } SMARTLIST_FOREACH_END(ent); + /* We keep the two smartlists separate so that we can free the proto_entry_t + * we created and put in missing_some, so here we add them together to build + * the string. */ + missing_all = smartlist_new(); + smartlist_add_all(missing_all, missing_some); + smartlist_add_all(missing_all, missing_completely); + if (missing_out && !all_supported) { - tor_assert(0 != smartlist_len(missing)); - *missing_out = encode_protocol_list(missing); + tor_assert(smartlist_len(missing_all) != 0); + *missing_out = encode_protocol_list(missing_all); } - smartlist_free(missing); + SMARTLIST_FOREACH(missing_some, proto_entry_t *, ent, proto_entry_free(ent)); + smartlist_free(missing_some); + smartlist_free(missing_completely); + smartlist_free(missing_all); SMARTLIST_FOREACH(entries, proto_entry_t *, ent, proto_entry_free(ent)); smartlist_free(entries); diff --git a/src/test/test_protover.c b/src/test/test_protover.c index c343e9957..95cc5f083 100644 --- a/src/test/test_protover.c +++ b/src/test/test_protover.c @@ -254,8 +254,23 @@ test_protover_all_supported(void *arg) tt_assert(! protover_all_supported("Link=3-4 Wombat=9", &msg)); tt_str_op(msg, OP_EQ, "Wombat=9"); tor_free(msg); + + /* Mix of things we support and don't support within a single protocol + * which we do support */ tt_assert(! protover_all_supported("Link=3-999", &msg)); - tt_str_op(msg, OP_EQ, "Link=3-999"); + tt_str_op(msg, OP_EQ, "Link=6-999"); + tor_free(msg); + tt_assert(! protover_all_supported("Link=1-3,345-666", &msg)); + tt_str_op(msg, OP_EQ, "Link=345-666"); + tor_free(msg); + tt_assert(! protover_all_supported("Link=1-3,5-12", &msg)); + tt_str_op(msg, OP_EQ, "Link=6-12"); + tor_free(msg); + + /* Mix of protocols we do support and some we don't, where the protocols + * we do support have some versions we don't support. */ + tt_assert(! protover_all_supported("Link=1-3,5-12 Quokka=9000-9001", &msg)); + tt_str_op(msg, OP_EQ, "Link=6-12 Quokka=9000-9001"); tor_free(msg); /* CPU/RAM DoS loop: Rust only */

1 0

[tor/master] tests: Make inline comments in test_protover.c more accurate.
by nickm＠torproject.org 03 Apr '18

03 Apr '18

commit 22c65a0e4bc7da08e216a769bba1b91f7b2a0ca1 Author: Isis Lovecruft <isis(a)torproject.org> Date: Tue Mar 27 21:34:00 2018 +0000 tests: Make inline comments in test_protover.c more accurate. The DoS potential is slightly higher in C now due to some differences to the Rust code, see the C_RUST_DIFFERS tags in src/rust/protover/tests/protover.rs. Also, the comment about "failing at the splitting stage" in Rust wasn't true, since when we split, we ignore empty chunks (e.g. "1--1" parses into "(1,None),(None,1)" and "None" can't be parsed into an integer). Finally, the comment about "Rust seems to experience an internal error" is only true in debug mode, where u32s are bounds-checked at runtime. In release mode, code expressing the equivalent of this test will error with `Err(ProtoverError::Unparseable)` because 4294967295 is too large. --- src/test/test_protover.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/test/test_protover.c b/src/test/test_protover.c index 95cc5f083..e7e17efe3 100644 --- a/src/test/test_protover.c +++ b/src/test/test_protover.c @@ -273,7 +273,7 @@ test_protover_all_supported(void *arg) tt_str_op(msg, OP_EQ, "Link=6-12 Quokka=9000-9001"); tor_free(msg); - /* CPU/RAM DoS loop: Rust only */ + /* We shouldn't be able to DoS ourselves parsing a large range. */ tt_assert(! protover_all_supported("Sleen=0-2147483648", &msg)); tt_str_op(msg, OP_EQ, "Sleen=0-2147483648"); tor_free(msg); @@ -546,8 +546,6 @@ test_protover_vote_roundtrip(void *args) { "Link=1,9-8,3", NULL }, { "Faux=-0", NULL }, { "Faux=0--0", NULL }, - // "These fail at the splitting stage in Rust, but the number parsing - // stage in C." { "Faux=-1", NULL }, { "Faux=-1-3", NULL }, { "Faux=1--1", NULL }, @@ -556,9 +554,9 @@ test_protover_vote_roundtrip(void *args) /* Large range */ { "Sleen=1-501", "Sleen=1-501" }, { "Sleen=1-65537", NULL }, - /* CPU/RAM DoS Loop: Rust only. */ + /* Both C/Rust implementations should be able to handle this mild DoS. */ { "Sleen=0-2147483648", NULL }, - /* Rust seems to experience an internal error here. */ + /* Rust tests are built in debug mode, so ints are bounds-checked. */ { "Sleen=0-4294967295", NULL }, }; unsigned u;

1 0

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

03 Apr '18

commit 527a2398634ecf1c244422121653f431c8df94fc 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] rust: Refactor protover::compute_for_old_tor().
by nickm＠torproject.org 03 Apr '18

03 Apr '18

commit fc2a42cc49fdcab9bbc0d90212c3c7bebda238b8 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 | 54 +++++++++++++++++++++++++++++++++++-------- 2 files changed, 46 insertions(+), 10 deletions(-) diff --git a/src/rust/protover/ffi.rs b/src/rust/protover/ffi.rs index 44c65114e..2dfeda87b 100644 --- a/src/rust/protover/ffi.rs +++ b/src/rust/protover/ffi.rs @@ -231,6 +231,6 @@ pub extern "C" fn protover_compute_for_old_tor(version: *const c_char) -> *const Err(_) => return empty.as_ptr(), }; - supported = compute_for_old_tor(&version); + supported = compute_for_old_tor_cstr(&version); supported.as_ptr() } diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs index 5492f7458..b4fb2e842 100644 --- a/src/rust/protover/protover.rs +++ b/src/rust/protover/protover.rs @@ -600,8 +600,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 /// @@ -615,34 +615,70 @@ 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 CStr { +/// # 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 CStr { let empty: &'static CStr = cstr!(""); if c_tor_version_as_new_as(version, FIRST_TOR_VERSION_TO_ADVERTISE_PROTOCOLS) { return empty; } - if c_tor_version_as_new_as(version, "0.2.9.1-alpha") { return cstr!("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"); } - if c_tor_version_as_new_as(version, "0.2.7.5") { return cstr!("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"); } - if c_tor_version_as_new_as(version, "0.2.4.19") { return cstr!("Cons=1 Desc=1 DirCache=1 HSDir=1 HSIntro=3 HSRend=1 \ Link=1-4 LinkAuth=1 Microdesc=1 Relay=1-2"); } - empty } +/// 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> { + // .to_str() fails with a Utf8Error if it couldn't validate the + // utf-8, so convert that here into an Unparseable ProtoverError. + compute_for_old_tor_cstr(version).to_str().or(Err(ProtoverError::Unparseable)) +} + #[cfg(test)] mod test { use std::str::FromStr;

1 0

[tor/master] rust: Add new protover::UnvalidatedProtoEntry type.
by nickm＠torproject.org 03 Apr '18

03 Apr '18

commit 3c47d31e1f29a016e2f0f21ca8445430ed7aad0a Author: Isis Lovecruft <isis(a)torproject.org> Date: Wed Mar 21 01:29:36 2018 +0000 rust: Add new protover::UnvalidatedProtoEntry type. This adds a new protover::UnvalidatedProtoEntry type, which is the UnknownProtocol variant of a ProtoEntry, and refactors several functions which should operate on this type into methods. This also fixes what was previously another difference to the C implementation: if you asked the C version of protovet_compute_vote() to compute a single vote containing "Fribble=", it would return NULL. However, the Rust version would return "Fribble=" since it didn't check if the versions were empty before constructing the string of differences. ("Fribble=" is technically a valid protover string.) This is now fixed, and the Rust version in that case will, analogous to (although safer than) C returning a NULL, return None. * REMOVE internal `contains_only_supported_protocols()` function. * REMOVE `all_supported()` function and refactor it into `UnvalidatedProtoEntry::all_supported()`. * REMOVE `parse_protocols_from_string_with_no_validation()` and refactor it into the more rusty implementation of `impl FromStr for UnvalidatedProtoEntry`. * REMOVE `protover_string_supports_protocol()` and refactor it into `UnvalidatedProtoEntry::supports_protocol()`. * REMOVE `protover_string_supports_protocol_or_later()` and refactor it into `UnvalidatedProtoEntry::supports_protocol_or_later()`. * FIXES part of #24031: https://bugs.torproject.org/24031 rust: Fix another C/Rust different in compute_vote(). This fixes the unittest from the prior commit by checking if the versions are empty before adding a protocol to a vote. --- src/rust/protover/protover.rs | 388 ++++++++++++++++++++++-------------------- 1 file changed, 208 insertions(+), 180 deletions(-) diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs index 1f62e70f1..847406ca2 100644 --- a/src/rust/protover/protover.rs +++ b/src/rust/protover/protover.rs @@ -226,207 +226,235 @@ impl FromStr for ProtoEntry { } } -/// Parses a single subprotocol entry string into subprotocol and version -/// parts, and then checks whether any of those versions are unsupported. -/// Helper for protover::all_supported -/// -/// # Inputs -/// -/// Accepted data is in the string format as follows: -/// -/// "HSDir=1-1" -/// -/// # Returns -/// -/// Returns `true` if the protocol entry is well-formatted and only contains -/// versions that are also supported in tor. Otherwise, returns false -/// -fn contains_only_supported_protocols(proto_entry: &str) -> bool { - let (name, mut vers) = match get_proto_and_vers(proto_entry) { - Ok(n) => n, - Err(_) => return false, - }; - - let currently_supported = match SupportedProtocols::tor_supported() { - Ok(n) => n.0, - Err(_) => return false, - }; - - let supported_versions = match currently_supported.get(&name) { - Some(n) => n, - None => return false, - }; +/// A `ProtoEntry`, but whose `Protocols` can be any `UnknownProtocol`, not just +/// the supported ones enumerated in `Protocols`. The protocol versions are +/// validated, however. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct UnvalidatedProtoEntry(HashMap<UnknownProtocol, ProtoSet>); - vers.0.retain(|x| !supported_versions.0.contains(x)); - vers.0.is_empty() +impl Default for UnvalidatedProtoEntry { + fn default() -> UnvalidatedProtoEntry { + UnvalidatedProtoEntry( HashMap::new() ) + } } -/// Determine if we support every protocol a client supports, and if not, -/// determine which protocols we do not have support for. -/// -/// # Inputs -/// -/// Accepted data is in the string format as follows: -/// -/// "HSDir=1-1 LinkAuth=1-2" -/// -/// # Returns -/// -/// Return `true` if every protocol version is one that we support. -/// Otherwise, return `false`. -/// Optionally, return parameters which the client supports but which we do not -/// -/// # Examples -/// ``` -/// use protover::all_supported; -/// -/// let (is_supported, unsupported) = all_supported("Link=1"); -/// assert_eq!(true, is_supported); -/// -/// let (is_supported, unsupported) = all_supported("Link=5-6"); -/// assert_eq!(false, is_supported); -/// assert_eq!("Link=5-6", unsupported); -/// -pub fn all_supported(protocols: &str) -> (bool, String) { - let unsupported = protocols - .split_whitespace() - .filter(|v| !contains_only_supported_protocols(v)) - .collect::<Vec<&str>>(); +impl UnvalidatedProtoEntry { + /// Get an iterator over the `Protocol`s and their `ProtoSet`s in this `ProtoEntry`. + pub fn iter(&self) -> hash_map::Iter<UnknownProtocol, ProtoSet> { + self.0.iter() + } - (unsupported.is_empty(), unsupported.join(" ")) -} + pub fn get(&self, protocol: &UnknownProtocol) -> Option<&ProtoSet> { + self.0.get(protocol) + } -/// Return true iff the provided protocol list includes support for the -/// indicated protocol and version. -/// Otherwise, return false -/// -/// # Inputs -/// -/// * `list`, a string representation of a list of protocol entries. -/// * `proto`, a `Proto` to test support for -/// * `vers`, a `Version` version which we will go on to determine whether the -/// specified protocol supports. -/// -/// # Examples -/// ``` -/// use protover::*; -/// -/// let is_supported = protover_string_supports_protocol("Link=3-4 Cons=1", -/// Proto::Cons,1); -/// assert_eq!(true, is_supported); -/// -/// let is_not_supported = protover_string_supports_protocol("Link=3-4 Cons=1", -/// Proto::Cons,5); -/// assert_eq!(false, is_not_supported) -/// ``` -pub fn protover_string_supports_protocol( - list: &str, - proto: Proto, - vers: Version, -) -> bool { - let supported = match SupportedProtocols::from_proto_entries_string(list) { - Ok(result) => result.0, - Err(_) => return false, - }; + pub fn insert(&mut self, key: UnknownProtocol, value: ProtoSet) { + self.0.insert(key, value); + } - let supported_versions = match supported.get(&proto) { - Some(n) => n, - None => return false, - }; + pub fn remove(&mut self, key: &UnknownProtocol) -> Option<ProtoSet> { + self.0.remove(key) + } - supported_versions.0.contains(&vers) -} + pub fn is_empty(&self) -> bool { + self.0.is_empty() + } -/// As protover_string_supports_protocol(), but also returns True if -/// any later version of the protocol is supported. -/// -/// # Examples -/// ``` -/// use protover::*; -/// -/// let is_supported = protover_string_supports_protocol_or_later( -/// "Link=3-4 Cons=5", Proto::Cons, 5); -/// -/// assert_eq!(true, is_supported); -/// -/// let is_supported = protover_string_supports_protocol_or_later( -/// "Link=3-4 Cons=5", Proto::Cons, 4); -/// -/// assert_eq!(true, is_supported); -/// -/// let is_supported = protover_string_supports_protocol_or_later( -/// "Link=3-4 Cons=5", Proto::Cons, 6); -/// -/// assert_eq!(false, is_supported); -/// ``` -pub fn protover_string_supports_protocol_or_later( - list: &str, - proto: Proto, - vers: u32, -) -> bool { - let supported = match SupportedProtocols::from_proto_entries_string(list) { - Ok(result) => result.0, - Err(_) => return false, - }; + /// Determine if we support every protocol a client supports, and if not, + /// determine which protocols we do not have support for. + /// + /// # Returns + /// + /// Optionally, return parameters which the client supports but which we do not. + /// + /// # Examples + /// ``` + /// use protover::UnvalidatedProtoEntry; + /// + /// let protocols: UnvalidatedProtoEntry = "LinkAuth=1 Microdesc=1-2 Relay=2".parse().unwrap(); + /// let unsupported: Option<UnvalidatedProtoEntry> = protocols.all_supported(); + /// assert_eq!(true, unsupported.is_none()); + /// + /// let protocols: UnvalidatedProtoEntry = "Link=1-2 Wombat=9".parse().unwrap(); + /// let unsupported: Option<UnvalidatedProtoEntry> = protocols.all_supported(); + /// assert_eq!(true, unsupported.is_some()); + /// assert_eq!("Wombat=9", &unsupported.unwrap().to_string()); + /// ``` + pub fn all_supported(&self) -> Option<UnvalidatedProtoEntry> { + let mut unsupported: UnvalidatedProtoEntry = UnvalidatedProtoEntry::default(); + let supported: ProtoEntry = match ProtoEntry::supported() { + Ok(x) => x, + Err(_) => return None, + }; - let supported_versions = match supported.get(&proto) { - Some(n) => n, - None => return false, - }; + for (protocol, versions) in self.iter() { + let is_supported: Result<Protocol, ProtoverError> = protocol.0.parse(); + let supported_protocol: Protocol; + + // If the protocol wasn't even in the enum, then we definitely don't + // know about it and don't support any of its versions. + if is_supported.is_err() { + if !versions.is_empty() { + unsupported.insert(protocol.clone(), versions.clone()); + } + continue; + } else { + supported_protocol = is_supported.unwrap(); + } - supported_versions.0.iter().any(|v| v >= &vers) -} + let maybe_supported_versions: Option<&ProtoSet> = supported.get(&supported_protocol); + let supported_versions: &ProtoSet; + let mut unsupported_versions: ProtoSet; + + // If the protocol wasn't in the map, then we don't know about it + // and don't support any of its versions. Add its versions to the + // map (if it has versions). + if maybe_supported_versions.is_none() { + if !versions.is_empty() { + unsupported.insert(protocol.clone(), versions.clone()); + } + continue; + } else { + supported_versions = maybe_supported_versions.unwrap(); + } + unsupported_versions = versions.clone(); + unsupported_versions.retain(|x| !supported_versions.contains(x)); -/// Parses a protocol list without validating the protocol names -/// -/// # Inputs -/// -/// * `protocol_string`, a string comprised of keys and values, both which are -/// strings. The keys are the protocol names while values are a string -/// representation of the supported versions. -/// -/// The input is _not_ expected to be a subset of the Proto types -/// -/// # Returns -/// -/// A `Result` whose `Ok` value is a `HashSet<Version>` holding all of the -/// unique version numbers. -/// -/// The returned `Result`'s `Err` value is an `&'static str` with a description -/// of the error. -/// -/// # Errors -/// -/// This function will error if: -/// -/// * The protocol string does not follow the "protocol_name=version_list" -/// expected format -/// * If the version string is malformed. See `Versions::from_version_string`. -/// -fn parse_protocols_from_string_with_no_validation<'a>( - protocol_string: &'a str, -) -> Result<HashMap<String, Versions>, &'static str> { - let mut parsed: HashMap<String, Versions> = HashMap::new(); + if !unsupported_versions.is_empty() { + unsupported.insert(protocol.clone(), unsupported_versions); + } + } - for subproto in protocol_string.split(" ") { - let mut parts = subproto.splitn(2, "="); + if unsupported.is_empty() { + return None; + } + Some(unsupported) + } - let name = match parts.next() { - Some("") => return Err("invalid protover entry"), + /// Determine if we have support for some protocol and version. + /// + /// # Inputs + /// + /// * `proto`, an `UnknownProtocol` to test support for + /// * `vers`, a `Version` which we will go on to determine whether the + /// specified protocol supports. + /// + /// # Return + /// + /// Returns `true` iff this `UnvalidatedProtoEntry` includes support for the + /// indicated protocol and version, and `false` otherwise. + /// + /// # Examples + /// + /// ``` + /// # use std::str::FromStr; + /// use protover::*; + /// # use protover::errors::ProtoverError; + /// + /// # fn do_test () -> Result<UnvalidatedProtoEntry, ProtoverError> { + /// let proto: UnvalidatedProtoEntry = "Link=3-4 Cons=1 Doggo=3-5".parse()?; + /// assert_eq!(true, proto.supports_protocol(&Protocol::Cons.into(), &1)); + /// assert_eq!(false, proto.supports_protocol(&Protocol::Cons.into(), &5)); + /// assert_eq!(true, proto.supports_protocol(&UnknownProtocol::from_str("Doggo")?, &4)); + /// # Ok(proto) + /// # } fn main () { do_test(); } + /// ``` + pub fn supports_protocol(&self, proto: &UnknownProtocol, vers: &Version) -> bool { + let supported_versions: &ProtoSet = match self.get(proto) { Some(n) => n, - None => return Err("invalid protover entry"), + None => return false, }; + supported_versions.contains(&vers) + } - let vers = match parts.next() { + /// As `UnvalidatedProtoEntry::supports_protocol()`, but also returns `true` + /// if any later version of the protocol is supported. + /// + /// # Examples + /// ``` + /// use protover::*; + /// # use protover::errors::ProtoverError; + /// + /// # fn do_test () -> Result<UnvalidatedProtoEntry, ProtoverError> { + /// let proto: UnvalidatedProtoEntry = "Link=3-4 Cons=5".parse()?; + /// + /// assert_eq!(true, proto.supports_protocol_or_later(&Protocol::Cons.into(), &5)); + /// assert_eq!(true, proto.supports_protocol_or_later(&Protocol::Cons.into(), &4)); + /// assert_eq!(false, proto.supports_protocol_or_later(&Protocol::Cons.into(), &6)); + /// # Ok(proto) + /// # } fn main () { do_test(); } + /// ``` + pub fn supports_protocol_or_later(&self, proto: &UnknownProtocol, vers: &Version) -> bool { + let supported_versions: &ProtoSet = match self.get(&proto) { Some(n) => n, - None => return Err("invalid protover entry"), + None => return false, }; + supported_versions.iter().any(|v| v.1 >= *vers) + } +} + +impl FromStr for UnvalidatedProtoEntry { + type Err = ProtoverError; + + /// Parses a protocol list without validating the protocol names. + /// + /// # Inputs + /// + /// * `protocol_string`, a string comprised of keys and values, both which are + /// strings. The keys are the protocol names while values are a string + /// representation of the supported versions. + /// + /// The input is _not_ expected to be a subset of the Protocol types + /// + /// # 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` whose `Display` + /// impl has a description of the error. + /// + /// # Errors + /// + /// This function will error if: + /// + /// * The protocol string does not follow the "protocol_name=version_list" + /// expected format, or + /// * If the version string is malformed. See `impl FromStr for ProtoSet`. + fn from_str(protocol_string: &str) -> Result<UnvalidatedProtoEntry, ProtoverError> { + let mut parsed: UnvalidatedProtoEntry = UnvalidatedProtoEntry::default(); - let versions = Versions::from_version_string(vers)?; + for subproto in protocol_string.split(' ') { + let mut parts = subproto.splitn(2, '='); - parsed.insert(String::from(name), versions); + let name = match parts.next() { + Some("") => return Err(ProtoverError::Unparseable), + Some(n) => n, + None => return Err(ProtoverError::Unparseable), + }; + let vers = match parts.next() { + Some(n) => n, + None => return Err(ProtoverError::Unparseable), + }; + let versions = ProtoSet::from_str(vers)?; + let protocol = UnknownProtocol::from_str(name)?; + + parsed.insert(protocol, versions); + } + Ok(parsed) + } +} + +/// Pretend a `ProtoEntry` is actually an `UnvalidatedProtoEntry`. +impl From<ProtoEntry> for UnvalidatedProtoEntry { + fn from(proto_entry: ProtoEntry) -> UnvalidatedProtoEntry { + let mut unvalidated: UnvalidatedProtoEntry = UnvalidatedProtoEntry::default(); + + for (protocol, versions) in proto_entry.iter() { + unvalidated.insert(UnknownProtocol::from(protocol.clone()), versions.clone()); + } + unvalidated } - Ok(parsed) } /// Protocol voting implementation.

1 0

[tor/master] rust: Port all C protover_all_supported tests to Rust.
by nickm＠torproject.org 03 Apr '18

03 Apr '18

commit 6eea0dc5f186429d598edda046156afc2a93120c Author: Isis Lovecruft <isis(a)torproject.org> Date: Tue Mar 27 21:38:29 2018 +0000 rust: Port all C protover_all_supported tests to Rust. The behaviours still do not match, unsurprisingly, but now we know where a primary difference is: the Rust is validating version ranges more than the C, so in the C it's possible to call protover_all_supported on a ridiculous version range like "Sleen=0-4294967294" because the C uses MAX_PROTOCOLS_TO_EXPAND to count the number of *subprotocols* whereas the Rust uses it to count the total number of *versions* of all subprotocols. --- src/rust/protover/tests/protover.rs | 86 +++++++++++++++++++++++++++++++++++-- 1 file changed, 83 insertions(+), 3 deletions(-) diff --git a/src/rust/protover/tests/protover.rs b/src/rust/protover/tests/protover.rs index 9f8b5a443..11015f35b 100644 --- a/src/rust/protover/tests/protover.rs +++ b/src/rust/protover/tests/protover.rs @@ -6,6 +6,7 @@ extern crate protover; use protover::ProtoEntry; use protover::ProtoverVote; use protover::UnvalidatedProtoEntry; +use protover::errors::ProtoverError; #[test] fn parse_protocol_with_single_proto_and_single_version() { @@ -320,9 +321,88 @@ fn protover_compute_vote_may_exceed_limit() { } #[test] -fn protover_all_supported_should_include_version_we_actually_do_support() { +fn protover_all_supported_should_exclude_versions_we_actually_do_support() { let proto: UnvalidatedProtoEntry = "Link=3-999".parse().unwrap(); - let _result: String = proto.all_supported().unwrap().to_string(); + let result: String = proto.all_supported().unwrap().to_string(); - assert_eq!(_result, "Link=3-999".to_string()); + assert_eq!(result, "Link=6-999".to_string()); +} + +#[test] +fn protover_all_supported_should_exclude_versions_we_actually_do_support_complex1() { + let proto: UnvalidatedProtoEntry = "Link=1-3,345-666".parse().unwrap(); + let result: String = proto.all_supported().unwrap().to_string(); + + assert_eq!(result, "Link=345-666".to_string()); +} + +#[test] +fn protover_all_supported_should_exclude_versions_we_actually_do_support_complex2() { + let proto: UnvalidatedProtoEntry = "Link=1-3,5-12".parse().unwrap(); + let result: String = proto.all_supported().unwrap().to_string(); + + assert_eq!(result, "Link=6-12".to_string()); +} + +#[test] +fn protover_all_supported_should_exclude_some_versions_and_entire_protocols() { + let proto: UnvalidatedProtoEntry = "Link=1-3,5-12 Quokka=9000-9001".parse().unwrap(); + let result: String = proto.all_supported().unwrap().to_string(); + + assert_eq!(result, "Link=6-12 Quokka=9000-9001".to_string()); +} + +#[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(); + + assert_eq!(result, "Sleen=0-2147483648".to_string()); +} + +#[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(); + + assert_eq!(result, "Sleen=0-4294967294".to_string()); +} + +#[test] +// C_RUST_DIFFERS: The C will return true (e.g. saying "yes, that's supported") +// but set the msg to NULL (??? seems maybe potentially bad). The Rust will +// simply return a None. +fn protover_all_supported_should_return_empty_string_for_weird_thing() { + let proto: UnvalidatedProtoEntry = "Fribble=".parse().unwrap(); + let result: Option<UnvalidatedProtoEntry> = proto.all_supported(); + + assert!(result.is_none()); +} + +#[test] +fn protover_unvalidatedprotoentry_should_err_entirely_unparseable_things() { + let proto: Result<UnvalidatedProtoEntry, ProtoverError> = "Fribble".parse(); + + assert_eq!(Err(ProtoverError::Unparseable), proto); +} + +#[test] +fn protover_all_supported_over_maximum_limit() { + let proto: Result<UnvalidatedProtoEntry, ProtoverError> = "Sleen=0-4294967295".parse(); + + assert_eq!(Err(ProtoverError::ExceedsMax), proto); }

1 0