diff --git a/zeroize/tests/alloc.rs b/zeroize/tests/alloc.rs index 9a575d7d..aa09de49 100644 --- a/zeroize/tests/alloc.rs +++ b/zeroize/tests/alloc.rs @@ -3,14 +3,40 @@ use core::{ alloc::{GlobalAlloc, Layout}, ptr, - sync::atomic::{AtomicPtr, Ordering::Relaxed}, + sync::atomic::{AtomicBool, AtomicPtr, Ordering::Relaxed}, }; use zeroize::Zeroize; -use std::alloc::System; +use std::{ + alloc::System, + sync::{Mutex, MutexGuard, PoisonError}, +}; static REG_PTR: AtomicPtr = AtomicPtr::new(ptr::null_mut()); +/// Set by the allocator if the registered allocation was freed while not fully zeroized. +/// +/// The allocator can't assert itself, since `GlobalAlloc` methods must not unwind. +static NOT_ZEROIZED: AtomicBool = AtomicBool::new(false); + +/// `REG_PTR` is global, so tests which use it must not run concurrently. +static REG_LOCK: Mutex<()> = Mutex::new(()); + +fn lock() -> MutexGuard<'static, ()> { + REG_LOCK.lock().unwrap_or_else(PoisonError::into_inner) +} + +/// Assert the registered allocation has been freed, and that it was zeroized when it was. +fn assert_checked() { + // Reset the global state before asserting, so a failure here can't leave `REG_PTR` + // pointing to memory which may be freed and reused by an unrelated allocation. + let freed = REG_PTR.swap(ptr::null_mut(), Relaxed).is_null(); + let not_zeroized = NOT_ZEROIZED.swap(false, Relaxed); + + assert!(freed, "registered allocation was never freed"); + assert!(!not_zeroized, "registered allocation was not zeroized"); +} + // Allocator that ensures that allocation registered in `REG_PTR` is zeroized. struct ProxyAllocator; @@ -22,9 +48,9 @@ unsafe impl GlobalAlloc for ProxyAllocator { unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { if ptr == REG_PTR.load(Relaxed) { - for i in 0..layout.size() { - let b = unsafe { ptr::read(ptr.add(i)) }; - assert_eq!(b, 0); + let bytes = unsafe { core::slice::from_raw_parts(ptr, layout.size()) }; + if bytes.iter().any(|&b| b != 0) { + NOT_ZEROIZED.store(true, Relaxed); } REG_PTR.store(ptr::null_mut(), Relaxed); } @@ -75,19 +101,43 @@ impl Drop for ObserveSecretBox { #[test] fn proxy_alloc_test() { + let _guard = lock(); + let b1 = SecretBox::new([u128::MAX; 10]); core::hint::black_box(&b1); drop(b1); + assert_checked(); let b2 = SecretBox::new([u8::MAX; 160]); core::hint::black_box(&b2); drop(b2); + assert_checked(); let b3 = ObserveSecretBox::new([u128::MAX; 10]); core::hint::black_box(&b3); drop(b3); + assert_checked(); let b4 = SecretBox::new([u8::MAX; 160]); core::hint::black_box(&b4); drop(b4); + assert_checked(); +} + +#[cfg(feature = "std")] +#[test] +fn cstring_zeroize_test() { + use std::ffi::CString; + + let _guard = lock(); + + let mut cstring = CString::new("Hello, world!").expect("CString::new failed"); + // Register the original heap buffer. `zeroize` shrinks it to fit the lone nul byte; since + // `ProxyAllocator` uses the default `GlobalAlloc::realloc` (alloc, copy, dealloc), that + // frees the original buffer, which must contain only zeroes by then. + REG_PTR.store(cstring.as_ptr().cast_mut().cast(), Relaxed); + + cstring.zeroize(); + assert_checked(); + assert_eq!(cstring.as_bytes_with_nul(), &[0]); } diff --git a/zeroize/tests/zeroize.rs b/zeroize/tests/zeroize.rs index fed17537..b1e7a769 100644 --- a/zeroize/tests/zeroize.rs +++ b/zeroize/tests/zeroize.rs @@ -15,9 +15,6 @@ use core::{ use std::sync::Arc; use zeroize::*; -#[cfg(feature = "std")] -use std::ffi::CString; - #[derive(Clone, Debug, PartialEq)] struct ZeroizedOnDrop(u64); @@ -172,29 +169,6 @@ fn zeroize_string_entire_capacity() { assert!(as_vec.iter().all(|byte| *byte == 0)); } -// TODO(tarcieri): debug flaky test (with potential UB?) See: RustCrypto/utils#774 -#[cfg(feature = "std")] -#[ignore] -#[test] -fn zeroize_c_string() { - let mut cstring = CString::new("Hello, world!").expect("CString::new failed"); - let orig_len = cstring.as_bytes().len(); - let orig_ptr = cstring.as_bytes().as_ptr(); - cstring.zeroize(); - // This doesn't quite test that the original memory has been cleared, but only that - // cstring now owns an empty buffer - assert!(cstring.as_bytes().is_empty()); - for i in 0..orig_len { - unsafe { - // Using a simple deref, only one iteration of the loop is performed - // presumably because after zeroize, the internal buffer has a length of one/ - // `read_volatile` seems to "fix" this - // Note that this is very likely UB - assert_eq!(orig_ptr.add(i).read_volatile(), 0); - } - } -} - #[cfg(feature = "alloc")] #[test] fn zeroize_box() {