commit 7ea9e080c58f16fecefa5a0a20406635a2034366 Author: Isis Lovecruft <isis@torproject.org> Date: Tue Feb 6 02:31:04 2018 +0000 protover: Fix memleak in Rust implementation. * FIXES #25127: https://bugs.torproject.org/25127. --- changes/bug25127 | 7 +++++++ src/rust/protover/ffi.rs | 29 ++++++++++++++++---------- src/rust/protover/protover.rs | 47 +++++++++++++++++++++++++++---------------- 3 files changed, 55 insertions(+), 28 deletions(-) diff --git a/changes/bug25127 b/changes/bug25127 new file mode 100644 index 000000000..3438ed125 --- /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 3eb22c933..5fefa8f7c 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 cf6fb78e2..00fe08408 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 {