Skip to content
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.

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

Allocate buffers more efficiently #2

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 1 addition & 4 deletions build.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
fn main() {
println!("cargo:rerun-if-changed=src/lib.c");

cc::Build::new()
.file("src/lib.c")
.compile("libvsprintf.a");
cc::Build::new().file("src/lib.c").compile("libvsprintf.a");
}

70 changes: 28 additions & 42 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,73 +3,59 @@
extern crate libc;

use libc::size_t;
use std::iter::repeat;
use std::io::Error;
use std::os::raw::*;

const INITIAL_BUFFER_SIZE: usize = 512;

/// The result of a vsprintf call.
pub type Result<T> = ::std::result::Result<T, Error>;

/// Prints a format string into a Rust string.
pub unsafe fn vsprintf<V>(format: *const c_char,
va_list: *mut V) -> Result<String> {
vsprintf_raw(format, va_list).map(|bytes| {
String::from_utf8(bytes).expect("vsprintf result is not valid utf-8")
})
pub unsafe fn vsprintf<V>(format: *const c_char, va_list: *mut V) -> Result<String> {
vsprintf_raw(format, va_list)
.map(|bytes| String::from_utf8(bytes).expect("vsprintf result is not valid utf-8"))
}

/// Prints a format string into a list of raw bytes that form
/// a null-terminated C string.
pub unsafe fn vsprintf_raw<V>(format: *const c_char,
va_list: *mut V) -> Result<Vec<u8>> {
let list_ptr = va_list as *mut c_void;

let mut buffer = Vec::new();
buffer.extend([0u8; INITIAL_BUFFER_SIZE].iter().cloned());
pub unsafe fn vsprintf_raw<V>(format: *const c_char, va_list: *mut V) -> Result<Vec<u8>> {
let list_ptr = va_list as *mut c_void;

let mut buffer: Vec<u8> = Vec::new();
loop {
let character_count = vsnprintf_wrapper(
buffer.as_mut_ptr(), buffer.len(), format, list_ptr
);
let rv = vsnprintf_wrapper(buffer.as_mut_ptr(), buffer.len(), format, list_ptr);

// Check for errors.
if character_count == -1 {
let character_count = if rv < 0 {
// C does not require vsprintf to set errno, but POSIX does.
//
// Default handling will just generate an 'unknown' IO error
// if no errno is set.
return Err(Error::last_os_error());
} else {
assert!(character_count >= 0);
let character_count = character_count as usize;

let current_max = buffer.len() - 1;

// Check if we had enough room in the buffer to fit everything.
if character_count > current_max {
let extra_space_required = character_count - current_max;

// Reserve enough space and try again.
buffer.extend(repeat(0).take(extra_space_required as usize));
continue;
} else { // We fit everything into the buffer.
// Truncate the buffer up until the null terminator.
buffer = buffer.into_iter()
.take_while(|&b| b != 0)
.collect();
break;
}
rv as usize
};

if character_count >= buffer.len() {
let new_len = character_count + 1;
buffer.reserve_exact(new_len - buffer.len());
// SAFETY: Any bit pattern is a valid u8, and we reserved the space.
Copy link

@couchand couchand Jan 12, 2022

Choose a reason for hiding this comment

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

This is not sound, despite what the comment claims. It is not sufficient for any bit pattern to be a valid u8, as uninitialized memory is not even guaranteed to be a fixed bit pattern. This is UB, which can be verified by trying this example with Miri: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=4fad0134c08d9f8a6272ce3c77d2c3f9

Copy link
Author

Choose a reason for hiding this comment

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

Oops, thanks for pointing this out. I learned this is incorrect later.

Happy to drop this part if I can get in the fix to avoid repeated allocations.

buffer.set_len(new_len);
continue;
}

// Drop NULL byte and any excess capacity.
buffer.truncate(character_count);
break;
}

Ok(buffer)
}

extern {
fn vsnprintf_wrapper(buffer: *mut u8,
size: size_t,
format: *const c_char,
va_list: *mut c_void) -> libc::c_int;
extern "C" {
fn vsnprintf_wrapper(
buffer: *mut u8,
size: size_t,
format: *const c_char,
va_list: *mut c_void,
) -> libc::c_int;
}