protover: Fix memleak in Rust implementation.

 * FIXES #25127: https://bugs.torproject.org/25127.

3 files changed, 55 insertions, 28 deletions

diff --git a/changes/bug25127 b/changes/bug25127
new file mode 100644
index 0000000000..3438ed1256
--- /dev/null
+++ b/changes/bug25127
@@ -0,0 +1,7 @@
+  o Minor bugfixes (Rust FFI):
+    - Fix a minor memory leak which would happen whenever the C code would call
+      the Rust implementation of protover_get_supported_protocols().  This was
+      due to the C version returning a static string, whereas the Rust version
+      newly allocated a CString to pass accross the FFI boundary.  Consequently,
+      the C code was not expecting to need to free() what it was given.  Fixes
+      bug 25127; bugfix on 0.3.2.1-alpha.
diff --git a/src/rust/protover/ffi.rs b/src/rust/protover/ffi.rs
index 3eb22c933e..5fefa8f7c0 100644
--- a/src/rust/protover/ffi.rs
+++ b/src/rust/protover/ffi.rs
@@ -137,18 +137,25 @@ pub extern "C" fn protocol_list_supports_protocol_or_later(
 /// Provide an interface for C to translate arguments and return types for
 /// protover::get_supported_protocols
 #[no_mangle]
-pub extern "C" fn protover_get_supported_protocols() -> *mut c_char {
-    // Not handling errors when unwrapping as the content is controlled
-    // and is an empty string
-    let empty = CString::new("").unwrap();
-
-    let supported = get_supported_protocols();
-    let c_supported = match CString::new(supported) {
-        Ok(n) => n,
-        Err(_) => return empty.into_raw(),
-    };
+pub extern "C" fn protover_get_supported_protocols() -> *const c_char {
+    let supported: &'static CStr;
+
+    // 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
+    // in protover.rs to contain a NUL byte somewhere in the middle would be a
+    // programming error.
+    assert!(!SUPPORTED_PROTOCOLS[..SUPPORTED_PROTOCOLS.len() - 1].contains(&0x00));
+    assert!(SUPPORTED_PROTOCOLS[SUPPORTED_PROTOCOLS.len() - 1] == 0x00);
+
+    // It's okay to call the "unchecked" version of the function because
+    // we can see that the bytes we're passing into it 1) are valid UTF-8,
+    // 2) have no intermediate NUL bytes, and 3) are terminated with a NUL
+    // byte.
+    unsafe {
+        supported = CStr::from_bytes_with_nul_unchecked(SUPPORTED_PROTOCOLS);
+    }
 
-    c_supported.into_raw()
+    supported.as_ptr()
 }
 
 /// Provide an interface for C to translate arguments and return types for
diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs
index cf6fb78e2f..00fe084083 100644
--- a/src/rust/protover/protover.rs
+++ b/src/rust/protover/protover.rs
@@ -3,12 +3,14 @@
 
 use external::c_tor_version_as_new_as;
 
+use std::str;
 use std::str::FromStr;
 use std::fmt;
 use std::collections::{HashMap, HashSet};
 use std::ops::Range;
 use std::string::String;
 
+
 /// The first version of Tor that included "proto" entries in its descriptors.
 /// Authorities should use this to decide whether to guess proto lines.
 ///
@@ -22,21 +24,29 @@ const FIRST_TOR_VERSION_TO_ADVERTISE_PROTOCOLS: &'static str = "0.2.9.3-alpha";
 /// C_RUST_COUPLED: src/or/protover.c `MAX_PROTOCOLS_TO_EXPAND`
 const MAX_PROTOCOLS_TO_EXPAND: u32 = 500;
 
-/// Currently supported protocols and their versions
+/// Currently supported protocols and their versions, as a byte-slice.
+///
+/// # Warning
+///
+/// This byte-slice ends in a NUL byte.  This is so that we can directly convert
+/// it to an `&'static CStr` in the FFI code, in order to hand the static string
+/// to C in a way that is compatible with C static strings.
+///
+/// Rust code which wishes to accesses this string should use
+/// `protover::get_supported_protocols()` instead.
 ///
 /// C_RUST_COUPLED: src/or/protover.c `protover_get_supported_protocols`
-const SUPPORTED_PROTOCOLS: &'static [&'static str] = &[
-    "Cons=1-2",
-    "Desc=1-2",
-    "DirCache=1-2",
-    "HSDir=1-2",
-    "HSIntro=3-4",
-    "HSRend=1-2",
-    "Link=1-5",
-    "LinkAuth=1,3",
-    "Microdesc=1-2",
-    "Relay=1-2",
-];
+pub(crate) const SUPPORTED_PROTOCOLS: &'static [u8] =
+    b"Cons=1-2 \
+    Desc=1-2 \
+    DirCache=1-2 \
+    HSDir=1-2 \
+    HSIntro=3-4 \
+    HSRend=1-2 \
+    Link=1-5 \
+    LinkAuth=1,3 \
+    Microdesc=1-2 \
+    Relay=1-2\0";
 
 /// Known subprotocols in Tor. Indicates which subprotocol a relay supports.
 ///
@@ -94,8 +104,11 @@ impl FromStr for Proto {
 ///
 /// "HSDir=1-1 LinkAuth=1"
 ///
-pub fn get_supported_protocols() -> String {
-    SUPPORTED_PROTOCOLS.join(" ")
+pub fn get_supported_protocols() -> &'static str {
+    unsafe {
+        // The `len() - 1` is to remove the NUL byte.
+        str::from_utf8_unchecked(&SUPPORTED_PROTOCOLS[..SUPPORTED_PROTOCOLS.len() - 1])
+    }
 }
 
 /// Translates a vector representation of a protocol list into a HashMap
@@ -134,7 +147,7 @@ fn parse_protocols_from_string<'a>(
 /// of the error.
 ///
 fn tor_supported() -> Result<HashMap<Proto, HashSet<u32>>, &'static str> {
-    parse_protocols(SUPPORTED_PROTOCOLS.iter())
+    parse_protocols(get_supported_protocols().split(" "))
 }
 
 /// Get the unique version numbers supported by a subprotocol.
@@ -625,7 +638,7 @@ pub fn compute_vote(
     }
 
     let mut final_output: HashMap<String, String> =
-        HashMap::with_capacity(SUPPORTED_PROTOCOLS.len());
+        HashMap::with_capacity(get_supported_protocols().split(" ").count());
 
     // Go through and remove verstions that are less than the threshold
     for (protocol, versions) in all_count {