commit cd28b4c7f5cefd319d6ded635d25911b4323b50b Author: Isis Lovecruft <isis@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;