zeroize: zeroize entire capacity of Vec

[aticu]

Re-implement #180, but without any additional required trait bounds, which is why #180 was reverted in #276.

[tony-iqlusion]

This appears to create a slice of uninitialized memory, to give it to the Zeroize impl on [Z], which iterates over it and performs ptr::write_volatile.

It might make sense instead to perform ptr::write_volatile here directly, followed by atomic_fence(), to avoid ever constructing an uninitialized slice.

[tony-iqlusion]

Alternatively this was recently stabilized: rust-lang/rust#68234

[HeroicKatora]

I don't quite see how constructing a *mut [T] helps. It's main intent is for data structures storing raw pointer or a NonNull<T> without lifetime information but here it would still require somehow manually iterating over the individual element pointers to do a volatile write. In particular, the implementation of Z::zeroize(&mut self) can still not be called without UB.

[tony-iqlusion]

I think this looks ok now, but will run it by a few others before merging

[aticu]

Thanks! By the way, I just noticed that a similar thing might make sense for String. If you want, I could also implement that.

Oh and I just noticed the CONTRIBUTING.md file. Should I add another commit adding myself to the AUTHORS.md?

[tony-iqlusion]

Should I add another commit adding myself to the AUTHORS.md?

Yes, that'd be good.

I just noticed that a similar thing might make sense for String. If you want, I could also implement that.

Sure, but I'd prefer a separate PR so it doesn't block this one.

[tony-iqlusion]

I think there's a simpler option than casting to a *mut u8, which is using mem::zeroed to produce the all-zero byte pattern for Z, and ptr::write_volatile to write it, e.g.

for i in 0..extra_capacity_len {
    unsafe { ptr::write_volatile(extra_capacity_start.add(i), mem::zeroed()); }
}

[aticu]

I considered that, but it would result in undefined behavior, when Z is not a type where an all-zero pattern is a valid value. However now that I think about it again, it wouldn't make much sense to implement Zeroize for such a type. I'll adjust that.

[aticu]

Actually on second thought, if it were implemented that way, it would be possible to invoke undefined behavior from safe code. For example this would then result in undefined behavior:

use core::num::NonZeroU8;
use zeroize::Zeroize;

struct NonZero(NonZeroU8);

impl Zeroize for NonZero {
    fn zeroize(&mut self) {
        self.0 = NonZeroU8::new(1).unwrap();
    }
}

fn main() {
    let mut vec = vec![NonZero(NonZeroU8::new(2).unwrap())];
    vec.clear();
    // undefined behavior: this would create a `NonZeroU8` with a
    // memory-representation of all-zeroes while zeroing the
    // uninitialized memory
    vec.zeroize();
}

This may not be a very useful implementation of Zeroize, but it would be a way to invoke undefined behavior in safe code. The other version does not have this problem.

[tony-iqlusion]

How would the uninitialized memory be exposed to safe code?

Regardless, the net effect is the same: you are writing zeroed bytes to the excess capacity.

[aticu]

How would the uninitialized memory be exposed to safe code?

It doesn't need to. According to the doc of mem::zeroed:

There is no guarantee that an all-zero byte-pattern represents a valid value of some type T. For example, the all-zero byte-pattern is not a valid value for reference types (&T and &mut T). Using zeroed on such types causes immediate undefined behavior because the Rust compiler assumes that there always is a valid value in a variable it considers initialized.

Which links to a section in the docs about undefined behavior:

It is the programmer's responsibility when writing unsafe code to ensure that any safe code interacting with the unsafe code cannot trigger these behaviors. unsafe code that satisfies this property for any safe client is called sound; if unsafe code can be misused by safe code to exhibit undefined behavior, it is unsound.

The same page also states that

Rust code is incorrect if it exhibits any of the behaviors in the following list.

• [...]
• Invalid values for a type with a custom definition of invalid values. In the standard library, this affects NonNull<T> and NonZero*.

The example above would therefore be both unsound and incorrect, which is possible from safe code. The values do not need to be accessible by safe code to create the problem here.

With the method which zeroes the memory using u8 this would not be the case, as u8 is guaranteed to be able to hold a value of 0.

[HeroicKatora]

What is a better solution?

The implementation, as currently written in this PR.

[tony-iqlusion]

It still seems like 6 of one, half dozen of another to me, except looping a byte-at-a-time and doing pointer arithmetic around core::mem::zeroed::<NonNull<T>>() is more complex and potentially slower.

It seems the crux of this is...

Memory behind a raw pointer can hold any value without any problem, because Rust does not consider it to be initialized. The problem only arises when an invalid value of type T exists, which Rust considers initialized, which mem::zeroed() does.

...but we're talking about a buffer which is defined as being:

What does a normal Vec::with_capacity call initialize it to?

Nothing, it does not create a value of type T though and just leaves the memory completely uninitialized.

The contract of Vec is always to initialize this capacity in some way before reading from it.

It seems the claim is it might be considered initialized by the Rust compiler when it is uninitialized, but before its API permits any reads, it will be initialized again.

Can either of you give a concrete example of a tractable problem which is possible given the contract of Vec to always (re)initialize this memory which would not occur with pointer-based zeroing?

[HeroicKatora]

What you are doing to the excess buffer of the vector is and never was the problem. You are indeed free to do with the extra capacity of bytes whatever you want. But not however you want. The code that writes invalid or uninitialized values into it must still be UB-free and executing mem::zeroed::<T> is not.

[aticu]

Can either of you give a concrete example of a tractable problem which is possible given the contract of Vec to always (re)initialize this memory which would not occur with pointer-based zeroing?

The problem is not the Vec or its memory, the problem is simply that the code could call mem::zeroed<NonNull<()>>, which is UB. The compiler might make optimizations that would crash the program if such a value ever exists. This would be the same problem in every program, even without a Vec being involved.

[tony-iqlusion]

Okay, I think I get it now. Thank you for the explanation.