Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Zeroize: Discrepency between [Z; N], `[Z], Vec<Z>, Box<[Z]> #899

Closed
elichai opened this issue May 18, 2023 · 10 comments
Closed

Zeroize: Discrepency between [Z; N], `[Z], Vec<Z>, Box<[Z]> #899

elichai opened this issue May 18, 2023 · 10 comments

Comments

@elichai
Copy link
Contributor

elichai commented May 18, 2023

Right now [Z] only implements Zeroize if Z: DefaultIsZeroes[0].
While Vec<Z>[1], Box<[Z]>[2], and even [Z; N][3] all implement Zeroize if Z: Zeroize.

The justification for impl<Z: DefaultIsZeros> Zeroize for [Z] is given here:

utils/zeroize/src/lib.rs

Lines 455 to 457 in 017165f

/// This impl can eventually be optimized using an memset intrinsic,
/// such as [`core::intrinsics::volatile_set_memory`]. For that reason the
/// blanket impl on slices is bounded by [`DefaultIsZeroes`].

To me it feels like they should all behave ~the same with regard to zeroing

[0] https://docs.rs/zeroize/1.6.0/zeroize/trait.Zeroize.html#impl-Zeroize-for-%5BZ%5D
[1] https://docs.rs/zeroize/1.6.0/zeroize/trait.Zeroize.html#impl-Zeroize-for-Vec%3CZ%3E
[2] https://docs.rs/zeroize/1.6.0/zeroize/trait.Zeroize.html#impl-Zeroize-for-Box%3C%5BZ%5D%3E
[3] https://docs.rs/zeroize/1.6.0/zeroize/trait.Zeroize.html#impl-Zeroize-for-%5BZ;+N%5D

@elichai
Copy link
Contributor Author

elichai commented May 18, 2023

Maybe one day we could use specialization for these kinds of stuff

@tarcieri
Copy link
Member

That stated reason for the slice impl is very important. It's what permits the sort of optimizations we're exploring in #841 without specialization.

It's deliberately inconsistent with the owned types in that regard, and also if you wanted to use the optimized implementation to zero the others, you can always obtain a slice.

@elichai
Copy link
Contributor Author

elichai commented May 18, 2023

It's deliberately inconsistent with the owned types in that regard

Could you expand on that?

@tarcieri
Copy link
Member

...to permit vectorized optimizations, as you already identified

@elichai
Copy link
Contributor Author

elichai commented May 18, 2023

...to permit vectorized optimizations, as you already identified

Wouldn't we also want to do vectorized optimizations for the owned variants?

@elichai
Copy link
Contributor Author

elichai commented May 18, 2023

Maybe DefaultIsZeroes should be an associated const bool in Zeroize and then we can "branch" over it? (which will be optimized away post-monomorphization)

@tarcieri
Copy link
Member

As I just mentioned, if you want to take advantage of that optimization for the owned variants in the case the type is DefaultIsZeros, you can call .as_mut().zeroize().

Likewise, if you have a mut slice you want to zero for a type which is Z: Zeroize but !DefaultIsZeros, you can do slice.iter_mut().zeroize(). So it's possible to go either way while still permitting an optimized implementation.

Without specialization I don't think you can do much better than this. Also zeroize has been 1.0 since 2019, so it's a little late for breaking changes.

@elichai
Copy link
Contributor Author

elichai commented May 18, 2023

As I just mentioned, if you want to take advantage of that optimization for the owned variants in the case the type is DefaultIsZeros, you can call .as_mut().zeroize().

My main concern with that is that at least I usually use this crate via either the derive macros or the derive macro combined with zeroize::Zeroizing so if I do #[derive(Zeroize, ZeroizeOnDrop)] struct MySecret(Box<[u8]) then it won't get the optimized version

@tarcieri
Copy link
Member

We can potentially change the derive macro to support calling some sort of mutable slice accessor like as_mut when a given attribute is provided.

Other than that I'm not sure there's a solution other than specialization.

(also note we haven't landed any optimized implementation yet, so you're not missing anything)

@tarcieri
Copy link
Member

I opened #901 to track custom derive support for AsMut.

Otherwise this is working as intended, and probably won't be revisited until specialization is stable.

@tarcieri tarcieri closed this as not planned Won't fix, can't repro, duplicate, stale May 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants