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

ZeroizeOnDrop auto-deref #700

Merged
merged 1 commit into from Jan 6, 2022

Conversation

daxpedda
Copy link
Contributor

@daxpedda daxpedda commented Jan 4, 2022

This changes nothing from the users perspective, but will enable to implement ZeroizeOnDrop for fields that implement it replacing the need to skip them instead.

This will also validate that these fields types implement ZeroizeOnDrop.

See #652 for further discussion.
Builds on top of #699.

Fixes #652.

@daxpedda daxpedda marked this pull request as draft January 4, 2022 19:09
fn zeroize_or_on_drop(&mut self);
}

#[doc(hidden)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d suggest making these proper sealed traits by placing them in a private module, rather than merely hiding them from the docs. Otherwise they’re still a part of the public API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They need to be part of the public API, otherwise the derive proc-macro can't access them. Some alternatives:

  • Rename them further to __AssertZeroizeOnDrop.
  • Remove them from zeroize and let the proc-macro insert them. This has the disadvantage of requiring every impl block to add them, which might increase compile time.
  • Mark traits as unsafe.

Or a combination of these.

Copy link
Member

@tarcieri tarcieri Jan 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise the derive proc-macro can't access them

Aah!

Rename them further to __AssertZeroizeOnDrop.

Another option would be to put them in a module like pub mod __internal, which would also let you do #[doc(hidden)] in a single place.

Mark traits as unsafe.

Since the trait doesn't deal strictly with memory safety I'd prefer not to do this.

Copy link
Contributor Author

@daxpedda daxpedda Jan 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done the __internal thing!

@daxpedda daxpedda marked this pull request as ready for review January 5, 2022 03:40
@daxpedda daxpedda requested a review from tarcieri January 6, 2022 19:25
@tarcieri tarcieri merged commit de5f676 into RustCrypto:master Jan 6, 2022
@tarcieri
Copy link
Member

tarcieri commented Jan 6, 2022

Thanks!

This was referenced Jan 14, 2022
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

Successfully merging this pull request may close these issues.

zeroize: add (back) ZeroizeOnDrop trait
2 participants