zeroize: fix ignored zeroize_c_string test

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<u8> = 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<S: Default> Drop for ObserveSecretBox<S> {
 
 #[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]);
 }

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() {