Add T: Copy and remove T: Default::default

[burdges]

We need type level numerics before Default ::default(), or any trait based approach, can work for large arrays, because users cannot impl Default for [u8; 256] since neither Default nor [u8; 256] were defined in their crate.

We cannot create a macro that builds everything from clear_on_drop.rs and core/default.rs onto a user version of Default::default() because then nobody can call across crate boundaries. In fact, users can still wrap their types so that they can impl Default for the wrapper, but that results in boilerplate code like this :

fn kdf(..) {
    struct X([u8; 256]);
    impl Default for X { fn default() -> Self { X([0u8;256]) } }
    let x: X = Default::default();
    ...
}

You need T: Copy anyways because otherwise ClearOnDrop<T> works with T: Drop types. I initially tried to support a limited subset of Drop types in https://github.com/burdges/zerodrop-rs/blob/master/src/zdd.rs#L42 but @bluss pointed out irreparable problems burdges/zerodrop-rs#7 in fact, we do not know that *_place = Default::default() does not read *_place out onto the stack to call the destructor if T: Drop.

I believe we can justify zeroing a T: Copy type with a simple memset like operation. We might violate a core::nonzero::NonZero bound applied to a non-pointer type, which afaik do not yet exist. That's okay if P is Box<T>, etc. since T is going away and Copy types cannot be Drop. About the only way to violate that assumption is with something like

fn kdf(..) {
    let _x: SomeNonZeroType;
    let x = CopyOnDrop(&mut x);
    let x: X = Default::default();
    ...
    ::std::mem::drop(x);
    assert_eq!(Some(_x), None);
}

I think the question now becomes: Which memset? You explained the problems with core::intrinsics::volatile_set_memory. We could call libc::memset or write a new one. Anything I'm missing?

[burdges]

I suppose this might work since T: Copy forbids T: Drop

*_place = unsafe { ::std::mem::zeroed::<T>() };

although I'd feel safer with another solution.

[cesarb]

I originally had a Copy + Default bound for T, but removed the Copy as I felt it was unnecessary; I see no problem adding it back. Also, ptr::write should work as well as *place = ..., but the later is simpler.

The problem with mem::zeroed is that all-zeros is not necessarily a valid value, even for Copy types. For integer intrinsics (and its arrays) and pointers it's valid, but for instance immutable borrows (&something) are AFAIK also Copy, and they can never be zero. Also, something like Option<u8> is Copy ("impl<T> Copy for Option<T> where T: Copy"), but there's zero (heh) guarantee that the discriminant for either None or Some is zero; it could change in a future compiler version.

Even if the value is "going away", it's best to be careful to avoid UB, since there's no telling what the compiler is going to do in that case (and it would certainly break if/when someone decides to make an UBSAN for Rust).

One could create a new ZeroSafe trait to weed out these cases, but then you run into the lack of type-level integers again.

In fact, I hadn't even thought of using ClearOnDrop on raw arrays, only on structs wrapping them; my personal use case was something like ClearOnDrop<Blake2bState>, where Blake2bState has a couple of integer arrays.

Given all that, my opinion is that it's better for now to require the wrapper implementing Default, and wait for type-level integers for the general solution (IMHO, the very first thing to be implemented using type-level integers will be Copy, Clone, and Default for arbitrary-sized arrays).

---

If you want ease-of-use, how about giving the user a predefined set of wrappers for power-of-two arrays, since these are the common sizes in cryptography, and implement Copy/Clone/Default/Deref/DerefMut for them?

Or perhaps lobby the compiler to implement the traits not only for every size up to 32, but also for every power-of-two size up to 4096? It should be only seven more implementations, so wouldn't bloat libstd too much, and would catch a lot of common situations.

[burdges]

I've just done a branch that does this https://github.com/burdges/clear_on_drop/tree/copy so it's only Default that causes problems, not Copy or Deref.

I suppose Default lets you do :

let _b2b: Blake2bState = Default::default();
{
    let b2b = ClearOnDrop::new(&mut b),
    ...
} {
    let b2b = ClearOnDrop::new(&mut b),
    ...
} {
    let b2b = ClearOnDrop::new(&mut b),
    ...
}

which resets _b2b only once per usage. I suppose you cannot normally do that however because an object like that tend towards needing a new method that takes a secret key, including Blake2b.

If powers of two were the issue, then we could both lobby for powers of two mid term and use this short term fix

trait HackyDefault {
    fn default() -> Self;
}
macro_rules! impl_HackyDefault {
    ($s) => {
        impl<T> HackyDefault for [T, $s] where T: Default {
            fn default() -> Self { [Default::default(); $s] }
        }
    }
}
impl_HackyDefault!(64);
impl_HackyDefault!(128);
impl_HackyDefault!(256);
impl_HackyDefault!(512);
impl_HackyDefault!(1024);
impl_HackyDefault!(2048);

It would not work for my use case however which involves manipulating the output of SHAKE or ChaCha in key derivation functions and key schedulers, but complex key derivation functions like that should be rare enough that doing an extra impl might be acceptable.

I'd be mildly annoyed if my code appeared as breaking any RFCs for type-level numerics, but a manual impl Default for MyStruct does not, so that's okay. I'm almost sold on Default here. ;)

I suppose ptr::write would be inlined too? It'd suck to actually copy from a static someplace in memory. lol

I'm still nervous about lacking Copy because Default has many potentially dangerous looking impls. If we've say Cow<Box<T>> then deref gives you the Box<T>, right? You can always make a struct CowBox<T>(Cow<Box<T>>) and give it a short cutting Deref. And now T: Copy.

[burdges]

Now there might be reasons to lack Copy to prevent Rust from copying the data around, but I'd think CopyOnDrop suffices to prevent that already, and you still always manipulate Copy data inside T anyways.

[bluss]

To me it seems like the only interesting problem is given Box<T>, (1) drop T in place, (2) zero over T, (3) drop the box without dropping the overwritten contents.

(1) is std::ptr::drop_in_place (2) is write_bytes (memset) but (3) I'm not sure. A wrapper type ("NoDrop" / "ManuallyDrop") inside the box is a solution.

[burdges]

I'd think you could do that with an enum like

enum ClearOnDropHelper<T, P>
  where T: Default, P: Deref<Target = T> + DerefMut 
{
    Alive(P),
    Dropped
}

pub enum ClearOnDrop<T, P>(ClearOnDropHelper<T,P>)
  where T: Default, P: Deref<Target = T> + DerefMut;

impl<T, P> Drop for ClearOnDrop<T, P>
  where T: Default, P: Deref<Target = T> + DerefMut {
    #[inline]
    fn drop(&mut self) {
        if let ClearOnDropHelper::Alive(p) = self.0 {
            let place = p.deref_mut();
            unsafe { ::std::ptr::drop_in_place::<T>(place); }
            *place = Default::default();
            hide_mem::<T>(place);
        }
        self.0 = ClearOnDropHelper::Dropped;
    }
}

It might not even consume additional space if P uses core::nonzero::NonZero somehow.

I'm assuming here that the two assignments here do not trigger calls to drop, but if they did then maybe we'd want unsafe { ::std::ptr::write::<T>(&self.0,ClearOnDropHelper::Dropped); } and maybe we do not need the explicit call to drop_in_place.

Also, I'm wondering if there is any advantage to replacing Default with another trait like say

trait Clearable : Sized {
    fn clear(&mut self) { *self = unsafe { ::std::mem::zeroed::<T>() }; }  // or maybe not this default
}

impl<T> Clearable for T where T: Default {
    fn clear(&mut self) { *self = Default::default(); }
}

Are there cryptographic objects that should not satisfy Default for some reason?

[bluss]

I'd just overwrite with zero bytes. Using default and regular assignments doesn't guarantee overwriting padding in structs and so on.

[burdges]

Right, so data can leak if anyone used a enum, thanks!

[burdges]

I'm going to close this because my pull request for a Clearable trait, and unsized array support, finally settled on keeping Default and contains much more compelling arguments for adding Copy #5