commit b85436c5961d1b2168c3954a7e718e03a121473f Author: Isis Lovecruft isis@torproject.org Date: Tue Feb 6 14:15:33 2018 +0000

protover: Fix memleak in Rust impl of protover_compute_for_old_tor.

* FIXES #25127: https://bugs.torproject.org/25127 * ADDS a new module to the Rust tor_util crate for small utilities for working with static strings between languages. * CHANGES the return type of protover_compute_for_old_tor to point to immutable data. * CHANGES the code from the previous commit to use the new static string utilities. --- src/rust/protover/ffi.rs | 40 ++++++++++++++------- src/rust/protover/lib.rs | 1 + src/rust/protover/protover.rs | 33 ++++++++--------- src/rust/tor_util/lib.rs | 2 +- src/rust/tor_util/strings.rs | 82 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 126 insertions(+), 32 deletions(-)

diff --git a/src/rust/protover/ffi.rs b/src/rust/protover/ffi.rs index 5fefa8f7c..5519b75ea 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 @@ -144,8 +147,7 @@ pub extern "C" fn protover_get_supported_protocols() -> *const c_char { // 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); + 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, @@ -200,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 @@ -217,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 5a5dea440..fe8c0f9bb 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 00fe08408..1680d3394 100644 --- a/src/rust/protover/protover.rs +++ b/src/rust/protover/protover.rs @@ -10,6 +10,7 @@ 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. @@ -724,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" @@ -737,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 42fa9d5ad..12cb3896b 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 000000000..9321ce4f8 --- /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 +}