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

Implement ZeroizeOnDrop #699

Merged
merged 1 commit into from Jan 5, 2022
Merged

Conversation

daxpedda
Copy link
Contributor

@daxpedda daxpedda commented Jan 4, 2022

As discussed in #652.

zeroize/derive/src/lib.rs Outdated Show resolved Hide resolved
@@ -22,7 +22,7 @@ decl_derive!(
/// Supports the following attributes:
///
/// On the item level:
/// - `#[zeroize(drop)]`: call `zeroize()` when this item is dropped
/// - `#[zeroize(drop)]`: *deprecated* use `ZeroizeOnDrop` instead
Copy link
Member

Choose a reason for hiding this comment

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

To properly deprecate this I think it'd be good to emit a #[deprecated(...)] attribute in the generated code, although I'm fine with addressing that in a follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am looking into it.

}
}

fn derive_zeroize_on_drop(s: &synstructure::Structure<'_>) -> TokenStream {
Copy link
Member

Choose a reason for hiding this comment

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

Aah okay, so derive_zeroize_on_drop does exist, but only adds the marker trait.

I think it would make more sense to also derive the Drop impl here?

Copy link
Contributor Author

@daxpedda daxpedda Jan 4, 2022

Choose a reason for hiding this comment

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

Can't be done that way because the method body is completely different. I could switch up the function names?

Copy link
Member

Choose a reason for hiding this comment

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

Switching the names sounds good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switching the names was quiet confusing, I changed it up a little, how does it look to you now?

@daxpedda daxpedda mentioned this pull request Jan 4, 2022
Copy link
Member

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

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

This looks fine to me now.

If you'd like to add the #[deprecated] attribute in this PR go ahead, or otherwise I can just merge.

@daxpedda
Copy link
Contributor Author

daxpedda commented Jan 4, 2022

Just checked it out, I can't get a warning to actually show up, feel free to merge, this can be done in a separate PR anyway.

Do you know how this is done by any chance? I just emitted #[deprecated = "..."] above the trait implementation, did nothing.

@daxpedda
Copy link
Contributor Author

daxpedda commented Jan 4, 2022

So https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-deprecated-attribute says:

It cannot be applied to trait implementation items.

This is the related issue: rust-lang/rust#39935.

@tarcieri
Copy link
Member

tarcieri commented Jan 4, 2022

Yeah, you'd need to make some sort of pseudo-function (possibly in the zeroize crate) and call it from the deprecated impls.

Probably best to save something that convoluted for a follow-up PR.

@tarcieri tarcieri merged commit 3f645e3 into RustCrypto:master Jan 5, 2022
@tarcieri
Copy link
Member

tarcieri commented Jan 5, 2022

Thank you!

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.

None yet

2 participants