[tor-commits] [tor/master] rust: Refactor protover::compute_for_old_tor().

[nickm at torproject.org]

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