Merge remote-tracking branch 'isis/bug25127'

6 files changed, 179 insertions, 58 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..5519b75ea4 100644
--- a/src/rust/protover/ffi.rs
+++ b/src/rust/protover/ffi.rs
@@ -12,6 +12,9 @@ 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;
+
 
 /// Translate C enums to Rust Proto enums, using the integer value of the C
 /// enum to map to its associated Rust enum
@@ -137,18 +140,24 @@ 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!(byte_slice_is_c_like(SUPPORTED_PROTOCOLS));
+
+    // 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
@@ -193,15 +202,15 @@ pub extern "C" fn protover_is_supported_here(
 /// Provide an interface for C to translate arguments and return types for
 /// protover::compute_for_old_tor
 #[no_mangle]
-pub extern "C" fn protover_compute_for_old_tor(
-    version: *const c_char,
-) -> *mut c_char {
-    // Not handling errors when unwrapping as the content is controlled
-    // and is an empty string
-    let empty = String::new();
+pub extern "C" fn protover_compute_for_old_tor(version: *const c_char) -> *const c_char {
+    let supported: &'static CStr;
+    let elder_protocols: &'static [u8];
+    let empty: &'static CStr;
+
+    empty = empty_static_cstr();
 
     if version.is_null() {
-        return allocate_and_copy_string(&empty);
+        return empty.as_ptr();
     }
 
     // Require an unsafe block to read the version from a C string. The pointer
@@ -210,10 +219,24 @@ pub extern "C" fn protover_compute_for_old_tor(
 
     let version = match c_str.to_str() {
         Ok(n) => n,
-        Err(_) => return allocate_and_copy_string(&empty),
+        Err(_) => return empty.as_ptr(),
     };
 
-    let supported = compute_for_old_tor(&version);
+    elder_protocols = compute_for_old_tor(&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
+    // in protover.rs to contain a NUL byte somewhere in the middle would be a
+    // programming error.
+    assert!(byte_slice_is_c_like(elder_protocols));
+
+    // 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(elder_protocols);
+    }
 
-    allocate_and_copy_string(&supported)
+    supported.as_ptr()
 }
diff --git a/src/rust/protover/lib.rs b/src/rust/protover/lib.rs
index 5a5dea4408..fe8c0f9bb2 100644
--- a/src/rust/protover/lib.rs
+++ b/src/rust/protover/lib.rs
@@ -26,6 +26,7 @@ extern crate libc;
 extern crate smartlist;
 extern crate external;
 extern crate tor_allocate;
+extern crate tor_util;
 
 mod protover;
 pub mod ffi;
diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs
index cf6fb78e2f..1680d3394e 100644
--- a/src/rust/protover/protover.rs
+++ b/src/rust/protover/protover.rs
@@ -3,12 +3,15 @@
 
 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;
 
+use tor_util::strings::NUL_BYTE;
+
 /// 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 +25,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 +105,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 +148,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 +639,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 {
@@ -711,11 +725,11 @@ pub fn is_supported_here(proto: Proto, vers: u32) -> bool {
 ///
 /// # Inputs
 ///
-/// * `version`, a string comprised of "[0-9,-]"
+/// * `version`, a string comprised of "[0-9a-z.-]"
 ///
 /// # Returns
 ///
-/// A `String` whose value is series of pairs, comprising of the protocol name
+/// A `&'static [u8]` whose value is series of pairs, comprising of the protocol name
 /// and versions that it supports. The string takes the following format:
 ///
 /// "HSDir=1-1 LinkAuth=1"
@@ -724,33 +738,27 @@ pub fn is_supported_here(proto: Proto, vers: u32) -> bool {
 /// only for tor versions older than FIRST_TOR_VERSION_TO_ADVERTISE_PROTOCOLS.
 ///
 /// C_RUST_COUPLED: src/rust/protover.c `compute_for_old_tor`
-pub fn compute_for_old_tor(version: &str) -> String {
-    if c_tor_version_as_new_as(
-        version,
-        FIRST_TOR_VERSION_TO_ADVERTISE_PROTOCOLS,
-    )
-    {
-        return String::new();
+pub fn compute_for_old_tor(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") {
-        let ret = "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";
-        return String::from(ret);
+        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") {
-        let ret = "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";
-        return String::from(ret);
+        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") {
-        let ret = "Cons=1 Desc=1 DirCache=1 HSDir=1 HSIntro=3 HSRend=1 \
-                   Link=1-4 LinkAuth=1 Microdesc=1 Relay=1-2";
-        return String::from(ret);
+        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";
     }
-    String::new()
+
+    NUL_BYTE
 }
 
 #[cfg(test)]
diff --git a/src/rust/tor_util/lib.rs b/src/rust/tor_util/lib.rs
index 42fa9d5ad0..12cb3896b6 100644
--- a/src/rust/tor_util/lib.rs
+++ b/src/rust/tor_util/lib.rs
@@ -3,9 +3,9 @@
 
 //! Small module to announce Rust support during startup for demonstration
 //! purposes.
-//!
 
 extern crate libc;
 extern crate tor_allocate;
 
 pub mod ffi;
+pub mod strings;
diff --git a/src/rust/tor_util/strings.rs b/src/rust/tor_util/strings.rs
new file mode 100644
index 0000000000..9321ce4f85
--- /dev/null
+++ b/src/rust/tor_util/strings.rs
@@ -0,0 +1,82 @@
+// Copyright (c) 2016-2017, The Tor Project, Inc. */
+// See LICENSE for licensing information */
+
+//! Utilities for working with static strings.
+
+use std::ffi::CStr;
+
+/// A byte-array containing a single NUL byte (`b"\0"`).
+pub const NUL_BYTE: &'static [u8] = b"\0";
+
+/// Determine if a byte slice is a C-like string.
+///
+/// These checks guarantee that:
+///
+/// 1. there are no intermediate NUL bytes
+/// 2. the last byte *is* a NUL byte
+///
+/// # Warning
+///
+/// This function does _not_ guarantee that the bytes represent any valid
+/// encoding such as ASCII or UTF-8.
+///
+/// # Examples
+///
+/// ```
+/// # use tor_util::strings::byte_slice_is_c_like;
+/// #
+/// let bytes: &[u8] = b"foo bar baz";
+///
+/// assert!(byte_slice_is_c_like(&bytes) == false);
+///
+/// let bytes: &[u8] = b"foo\0bar baz";
+///
+/// assert!(byte_slice_is_c_like(&bytes) == false);
+///
+/// let bytes: &[u8] = b"foo bar baz\0";
+///
+/// assert!(byte_slice_is_c_like(&bytes) == true);
+/// ```
+pub fn byte_slice_is_c_like(bytes: &[u8]) -> bool {
+    if !bytes[..bytes.len() - 1].contains(&0x00) && bytes[bytes.len() - 1] == 0x00 {
+        return true;
+    }
+    false
+}
+
+/// Get a static `CStr` containing a single `NUL_BYTE`.
+///
+/// # Examples
+///
+/// When used as follows in a Rust FFI function, which could be called
+/// from C:
+///
+/// ```
+/// # extern crate libc;
+/// # extern crate tor_util;
+/// #
+/// # use tor_util::strings::empty_static_cstr;
+/// use libc::c_char;
+/// use std::ffi::CStr;
+///
+/// pub extern "C" fn give_c_code_an_empty_static_string() -> *const c_char {
+///     let empty: &'static CStr = empty_static_cstr();
+///
+///     empty.as_ptr()
+/// }
+///
+/// # fn main() {
+/// #     give_c_code_an_empty_static_string();
+/// # }
+/// ```
+///
+/// This equates to an "empty" `const char*` static string in C.
+pub fn empty_static_cstr() -> &'static CStr {
+    let empty: &'static CStr;
+
+    unsafe {
+        empty = CStr::from_bytes_with_nul_unchecked(NUL_BYTE);
+    }
+
+    empty
+}