Skip to content

Implement Show for CString and fix warning compiling tests for libcollections #15859

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Implement Show for CString and fix warning compiling tests for libcollections #15859

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
44fcec5
Fix deprecation warning in deque.rs
aochagavia Jul 21, 2014
3bfc852
Implement Show for CString
aochagavia Jul 21, 2014
0613313
Add a null pointer check to CString::new
aochagavia Jul 22, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/libcollections/deque.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ pub mod bench {
// measure
let mut i = 0;
b.iter(|| {
map.find(keys.get(i));
map.find(&keys[i]);
i = (i + 1) % n;
})
}
Expand Down
10 changes: 5 additions & 5 deletions src/libnative/io/addrinfo.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@

use libc::{c_char, c_int};
use libc;
use std::c_str::CString;
use std::mem;
use std::ptr::{null, mut_null};
use std::rt::rtio;
Expand All @@ -27,8 +26,8 @@ impl GetAddrInfoRequest {
{
assert!(host.is_some() || servname.is_some());

let c_host = host.map_or(unsafe { CString::new(null(), true) }, |x| x.to_c_str());
let c_serv = servname.map_or(unsafe { CString::new(null(), true) }, |x| x.to_c_str());
let c_host = host.map(|x| x.to_c_str());
let c_serv = servname.map(|x| x.to_c_str());

let hint = hint.map(|hint| {
libc::addrinfo {
Expand All @@ -50,8 +49,8 @@ impl GetAddrInfoRequest {

// Make the call
let s = unsafe {
let ch = if c_host.is_null() { null() } else { c_host.as_ptr() };
let cs = if c_serv.is_null() { null() } else { c_serv.as_ptr() };
let ch = if c_host.is_none() { null() } else { c_host.unwrap().as_ptr() };
let cs = if c_serv.is_none() { null() } else { c_serv.unwrap().as_ptr() };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a use-after-free because you're unwrapping c_host and c_serv, destroying their contents, and then referencing them right afterwards.

getaddrinfo(ch, cs, hint_ptr, &mut res)
};

Expand Down Expand Up @@ -104,6 +103,7 @@ fn get_error(_: c_int) -> IoError {

#[cfg(not(windows))]
fn get_error(s: c_int) -> IoError {
use std::c_str::CString;

let err_str = unsafe {
CString::new(gai_strerror(s), false).as_str().unwrap().to_string()
Expand Down
128 changes: 20 additions & 108 deletions src/librustrt/c_str.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ use core::prelude::*;
use alloc::libc_heap::malloc_raw;
use collections::string::String;
use collections::hash;
use core::fmt;
use core::kinds::marker;
use core::mem;
use core::ptr;
Expand All @@ -92,23 +93,18 @@ impl Clone for CString {
/// reasons, this is always a deep clone, rather than the usual shallow
/// clone.
fn clone(&self) -> CString {
if self.buf.is_null() {
CString { buf: self.buf, owns_buffer_: self.owns_buffer_ }
} else {
let len = self.len() + 1;
let buf = unsafe { malloc_raw(len) } as *mut libc::c_char;
unsafe { ptr::copy_nonoverlapping_memory(buf, self.buf, len); }
CString { buf: buf as *const libc::c_char, owns_buffer_: true }
}
let len = self.len() + 1;
let buf = unsafe { malloc_raw(len) } as *mut libc::c_char;
unsafe { ptr::copy_nonoverlapping_memory(buf, self.buf, len); }
CString { buf: buf as *const libc::c_char, owns_buffer_: true }
}
}

impl PartialEq for CString {
fn eq(&self, other: &CString) -> bool {
// Check if the two strings share the same buffer
if self.buf as uint == other.buf as uint {
true
} else if self.buf.is_null() || other.buf.is_null() {
false
} else {
unsafe {
libc::strcmp(self.buf, other.buf) == 0
Expand All @@ -135,7 +131,12 @@ impl<S: hash::Writer> hash::Hash<S> for CString {

impl CString {
/// Create a C String from a pointer.
///
///# Failure
///
/// Fails if `buf` is null
pub unsafe fn new(buf: *const libc::c_char, owns_buffer: bool) -> CString {
assert!(!buf.is_null());
CString { buf: buf, owns_buffer_: owns_buffer }
}

Expand All @@ -157,10 +158,6 @@ impl CString {
/// let p = foo.to_c_str().as_ptr();
/// ```
///
/// # Failure
///
/// Fails if the CString is null.
///
/// # Example
///
/// ```rust
Expand All @@ -174,8 +171,6 @@ impl CString {
/// }
/// ```
pub fn as_ptr(&self) -> *const libc::c_char {
if self.buf.is_null() { fail!("CString is null!"); }

self.buf
}

Expand All @@ -196,44 +191,30 @@ impl CString {
/// // wrong (the CString will be freed, invalidating `p`)
/// let p = foo.to_c_str().as_mut_ptr();
/// ```
///
/// # Failure
///
/// Fails if the CString is null.
pub fn as_mut_ptr(&mut self) -> *mut libc::c_char {
if self.buf.is_null() { fail!("CString is null!") }

self.buf as *mut _
}

/// Calls a closure with a reference to the underlying `*libc::c_char`.
///
/// # Failure
///
/// Fails if the CString is null.
#[deprecated="use `.as_ptr()`"]
pub fn with_ref<T>(&self, f: |*const libc::c_char| -> T) -> T {
if self.buf.is_null() { fail!("CString is null!"); }
f(self.buf)
}

/// Calls a closure with a mutable reference to the underlying `*libc::c_char`.
///
/// # Failure
///
/// Fails if the CString is null.
#[deprecated="use `.as_mut_ptr()`"]
pub fn with_mut_ref<T>(&mut self, f: |*mut libc::c_char| -> T) -> T {
if self.buf.is_null() { fail!("CString is null!"); }
f(self.buf as *mut libc::c_char)
}

/// Returns true if the CString is a null.
#[deprecated="a CString cannot be null"]
pub fn is_null(&self) -> bool {
self.buf.is_null()
}

/// Returns true if the CString is not null.
#[deprecated="a CString cannot be null"]
pub fn is_not_null(&self) -> bool {
self.buf.is_not_null()
}
Expand All @@ -245,51 +226,32 @@ impl CString {

/// Converts the CString into a `&[u8]` without copying.
/// Includes the terminating NUL byte.
///
/// # Failure
///
/// Fails if the CString is null.
#[inline]
pub fn as_bytes<'a>(&'a self) -> &'a [u8] {
if self.buf.is_null() { fail!("CString is null!"); }
unsafe {
mem::transmute(Slice { data: self.buf, len: self.len() + 1 })
}
}

/// Converts the CString into a `&[u8]` without copying.
/// Does not include the terminating NUL byte.
///
/// # Failure
///
/// Fails if the CString is null.
#[inline]
pub fn as_bytes_no_nul<'a>(&'a self) -> &'a [u8] {
if self.buf.is_null() { fail!("CString is null!"); }
unsafe {
mem::transmute(Slice { data: self.buf, len: self.len() })
}
}

/// Converts the CString into a `&str` without copying.
/// Returns None if the CString is not UTF-8.
///
/// # Failure
///
/// Fails if the CString is null.
#[inline]
pub fn as_str<'a>(&'a self) -> Option<&'a str> {
let buf = self.as_bytes_no_nul();
str::from_utf8(buf)
}

/// Return a CString iterator.
///
/// # Failure
///
/// Fails if the CString is null.
pub fn iter<'a>(&'a self) -> CChars<'a> {
if self.buf.is_null() { fail!("CString is null!"); }
CChars {
ptr: self.buf,
marker: marker::ContravariantLifetime,
Expand Down Expand Up @@ -325,13 +287,8 @@ impl Drop for CString {

impl Collection for CString {
/// Return the number of bytes in the CString (not including the NUL terminator).
///
/// # Failure
///
/// Fails if the CString is null.
#[inline]
fn len(&self) -> uint {
if self.buf.is_null() { fail!("CString is null!"); }
let mut cur = self.buf;
let mut len = 0;
unsafe {
Expand All @@ -344,6 +301,12 @@ impl Collection for CString {
}
}

impl fmt::Show for CString {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
String::from_utf8_lossy(self.as_bytes_no_nul()).fmt(f)
}
}

/// A generic trait for converting a value to a CString.
pub trait ToCStr {
/// Copy the receiver into a CString.
Expand Down Expand Up @@ -624,13 +587,6 @@ mod tests {
}
}

#[test]
fn test_is_null() {
let c_str = unsafe { CString::new(ptr::null(), false) };
assert!(c_str.is_null());
assert!(!c_str.is_not_null());
}

#[test]
fn test_unwrap() {
let c_str = "hello".to_c_str();
Expand All @@ -641,16 +597,8 @@ mod tests {
fn test_as_ptr() {
let c_str = "hello".to_c_str();
let len = unsafe { libc::strlen(c_str.as_ptr()) };
assert!(!c_str.is_null());
assert!(c_str.is_not_null());
assert_eq!(len, 5);
}
#[test]
#[should_fail]
fn test_as_ptr_empty_fail() {
let c_str = unsafe { CString::new(ptr::null(), false) };
c_str.as_ptr();
}

#[test]
fn test_iterator() {
Expand Down Expand Up @@ -709,20 +657,6 @@ mod tests {
assert_eq!(c_str.as_bytes_no_nul(), b"foo\xFF");
}

#[test]
#[should_fail]
fn test_as_bytes_fail() {
let c_str = unsafe { CString::new(ptr::null(), false) };
c_str.as_bytes();
}

#[test]
#[should_fail]
fn test_as_bytes_no_nul_fail() {
let c_str = unsafe { CString::new(ptr::null(), false) };
c_str.as_bytes_no_nul();
}

#[test]
fn test_as_str() {
let c_str = "hello".to_c_str();
Expand All @@ -735,23 +669,8 @@ mod tests {

#[test]
#[should_fail]
fn test_as_str_fail() {
let c_str = unsafe { CString::new(ptr::null(), false) };
c_str.as_str();
}

#[test]
#[should_fail]
fn test_len_fail() {
fn test_new_fail() {
let c_str = unsafe { CString::new(ptr::null(), false) };
c_str.len();
}

#[test]
#[should_fail]
fn test_iter_fail() {
let c_str = unsafe { CString::new(ptr::null(), false) };
c_str.iter();
}

#[test]
Expand Down Expand Up @@ -784,13 +703,6 @@ mod tests {
// force a copy, reading the memory
c_.as_bytes().to_vec();
}

#[test]
fn test_clone_eq_null() {
let x = unsafe { CString::new(ptr::null(), false) };
let y = x.clone();
assert!(x == y);
}
}

#[cfg(test)]
Expand Down