From 8d6084dd36aa3428a4978abf6b8d9499fb68e286 Mon Sep 17 00:00:00 2001 From: Bardi Harborow Date: Tue, 3 Nov 2026 18:14:45 +1100 Subject: [PATCH 1/3] zeroize: don't panic in the proxy allocator in `tests/alloc.rs` `ProxyAllocator::dealloc` asserted that the registered allocation was zeroized, but `GlobalAlloc` methods must not unwind, so a failure aborted the whole test binary with no diagnostic. Record the failure in a flag instead, and check it from the test thread in `assert_checked`, which also checks that the registered allocation was actually freed. Signed-off-by: Bardi Harborow --- zeroize/tests/alloc.rs | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/zeroize/tests/alloc.rs b/zeroize/tests/alloc.rs index 9a575d7d..1a4f773c 100644 --- a/zeroize/tests/alloc.rs +++ b/zeroize/tests/alloc.rs @@ -3,7 +3,7 @@ use core::{ alloc::{GlobalAlloc, Layout}, ptr, - sync::atomic::{AtomicPtr, Ordering::Relaxed}, + sync::atomic::{AtomicBool, AtomicPtr, Ordering::Relaxed}, }; use zeroize::Zeroize; @@ -11,6 +11,22 @@ use std::alloc::System; 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); + +/// 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 +38,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); } @@ -78,16 +94,20 @@ fn proxy_alloc_test() { 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(); } From cd8f537625a4a53c48218af6a78a3e26c6313024 Mon Sep 17 00:00:00 2001 From: Bardi Harborow Date: Tue, 3 Nov 2026 18:18:50 +1100 Subject: [PATCH 2/3] zeroize: serialize tests which use `REG_PTR` in `tests/alloc.rs` `REG_PTR` is a global, so tests which register an allocation with it must not run concurrently. Add a mutex for such tests to hold, in preparation for adding more of them. Signed-off-by: Bardi Harborow --- zeroize/tests/alloc.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/zeroize/tests/alloc.rs b/zeroize/tests/alloc.rs index 1a4f773c..202402aa 100644 --- a/zeroize/tests/alloc.rs +++ b/zeroize/tests/alloc.rs @@ -7,7 +7,10 @@ use core::{ }; use zeroize::Zeroize; -use std::alloc::System; +use std::{ + alloc::System, + sync::{Mutex, MutexGuard, PoisonError}, +}; static REG_PTR: AtomicPtr = AtomicPtr::new(ptr::null_mut()); @@ -16,6 +19,13 @@ static REG_PTR: AtomicPtr = AtomicPtr::new(ptr::null_mut()); /// 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` @@ -91,6 +101,8 @@ 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); From ff4b5c6d2e223d136b4cfa91040704b503c338e8 Mon Sep 17 00:00:00 2001 From: Bardi Harborow Date: Tue, 3 Nov 2026 18:18:50 +1100 Subject: [PATCH 3/3] zeroize: fix ignored `zeroize_c_string` test The test read the CString's original buffer after `zeroize()`, but that buffer may already be freed by then, which is UB. Replace it with `cstring_zeroize_test` in `tests/alloc.rs`, which checks the original buffer with the proxy allocator when it is freed. Signed-off-by: Bardi Harborow --- zeroize/tests/alloc.rs | 18 ++++++++++++++++++ zeroize/tests/zeroize.rs | 26 -------------------------- 2 files changed, 18 insertions(+), 26 deletions(-) diff --git a/zeroize/tests/alloc.rs b/zeroize/tests/alloc.rs index 202402aa..aa09de49 100644 --- a/zeroize/tests/alloc.rs +++ b/zeroize/tests/alloc.rs @@ -123,3 +123,21 @@ fn proxy_alloc_test() { 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() {