[tor-commits] [tor/master] protover: Fix memleak in Rust implementation.
nickm at torproject.org
nickm at torproject.org
Thu Feb 8 22:36:17 UTC 2018
commit 7ea9e080c58f16fecefa5a0a20406635a2034366
Author: Isis Lovecruft <isis at 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 {
More information about the tor-commits
mailing list