Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 63 additions & 1 deletion digest/src/buffer_macros/variable.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
/// Creates a buffered wrapper around block-level "core" type which implements variable output size traits
/// with output size selected at compile time.
#[doc(hidden)]
Copy link
Copy Markdown
Member

@newpavlov newpavlov Apr 3, 2026

Choose a reason for hiding this comment

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

This looks like an unnecessary complication. Why can't you simply copy what is done in buffer_fixed!?

Copy link
Copy Markdown
Author

@kayabaNerve kayabaNerve Apr 3, 2026

Choose a reason for hiding this comment

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

To keep the cfg(feature = "zeroize") local to digest and not in the generated code which assumes the caller will have a feature of the same name. In all my work, I don't have zeroize as optional and don't define such a feature, so I'd not have such codegen applied despite explicitly wanting it as shown by enabling digest/zeroize. The fact the other macro exhibited that pattern would be a hygiene issue and bug in it, AFAIK.

Copy link
Copy Markdown
Member

@newpavlov newpavlov Apr 3, 2026

Choose a reason for hiding this comment

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

This introduces inconsistency across macros and makes the macros API even more complicated and confusing than it's already is.

The fact the other macro exhibited that pattern would be a hygiene issue and bug in it, AFAIK.

No, it's intentional. The macro is a helper to define a buffered struct, so the generated code should work with features defined by the downstream crate. If it does not define zeroize crate feature for some reason, then it should not implement ZeroizeOnDrop using the macro and instead do it with an explicit impl.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I don't actually believe this makes the macro API more complicated, as the user-facing API is identical. The only change is when digest/zeroize, it'll also derive ZeroizeOnDrop.

No, it's intentional. The macro is a helper to define a buffered struct, so the generated code should work with features defined by the downstream crate.

If I have to update this macro to be unhygienic and impose undocumented requirements on the caller's declared features (assumed to have the exact same presence and name as seen within RustCrypto) to get it merged, I'm fine doing so because I honestly can't be arsed to care, but please confirm if that's a prerequisite for this PR getting merged. Right now, I'm unclear if this is still a discussion or if I am being told this is a hard blocker, sorry.

If it does not define zeroize crate feature for some reason, then it should not implement ZeroizeOnDrop using the macro and instead do it with an explicit impl

I'm unsure if this is anything more than a footgun. Specifically, the digest/zeroize feature means that zeroize is a dependency and already present in-tree. I have no idea why, when zeroize is present, digest wouldn't derive traits from zeroize. If the goal is a caller who's using these macros but can opt-in/out of these features, then this macro should have an explicit variant with ZeroizeOnDrop/a way to exclude traits/shouldn't generate ZeroizeOnDrop at all IMO. This would imply I should PR this trait implementation directly to blake2, though I assume such a PR will then be kicked back as it should be part of the macro.

Also, manually unconditionally implementing ZeroizeOnDrop somewhat conflicts with a macro which conditionally implements it. I don't believe it'd ever actually come up, as it'd only be realized if that feature was added and enabled, but it truly seems like a poor solution to me.

#[macro_export]
macro_rules! buffer_ct_variable {
macro_rules! buffer_ct_variable_internal {
(
$(#[$attr:meta])*
$vis:vis struct $name:ident<$out_size:ident>($core_ty:ty);
Expand Down Expand Up @@ -209,3 +210,64 @@ macro_rules! buffer_ct_variable {
}
};
}

#[doc(hidden)]
#[cfg(not(feature = "zeroize"))]
#[macro_export]
macro_rules! buffer_ct_variable_zeroize_on_drop {
($name:ident, $out_size:ident, $max_size:ty) => {};
}

#[doc(hidden)]
#[cfg(feature = "zeroize")]
#[macro_export]
macro_rules! buffer_ct_variable_zeroize_on_drop {
($name:ident, $out_size:ident, $max_size:ty) => {
// While `$name` will not implement `Drop`, it is still `ZeroizeOnDrop` as all its fields
// are `ZeroizeOnDrop`. The following ensures this.
impl<$out_size> $name<$out_size>
where
$out_size: $crate::array::ArraySize
+ $crate::typenum::IsLessOrEqual<$max_size, Output = $crate::typenum::True>,
{
// This is `pub` to ensure it's actually compiled and not eliminated as dead code
#[doc(hidden)]
pub fn __ensure_all_fields_impl_zeroize_on_drop(&mut self) {
let Self { core, buffer } = self;
fn implements_zeroize_on_drop(_value: &mut impl $crate::zeroize::ZeroizeOnDrop) {}
implements_zeroize_on_drop(core);
implements_zeroize_on_drop(buffer);
}
}

impl<$out_size> $crate::zeroize::ZeroizeOnDrop for $name<$out_size> where
$out_size: $crate::array::ArraySize
+ $crate::typenum::IsLessOrEqual<$max_size, Output = $crate::typenum::True>
{
}
};
}

/// Creates a buffered wrapper around block-level "core" type which implements variable output size traits
/// with output size selected at compile time.
#[macro_export]
macro_rules! buffer_ct_variable {
(
$(#[$attr:meta])*
$vis:vis struct $name:ident<$out_size:ident>($core_ty:ty);
exclude: SerializableState;
// Ideally, we would use `$core_ty::OutputSize`, but unfortunately the compiler
// does not accept such code. The likely reason is this issue:
// https://github.com/rust-lang/rust/issues/79629
max_size: $max_size:ty;
) => {
$crate::buffer_ct_variable_internal!(
$(#[$attr])*
$vis struct $name<$out_size>($core_ty);
exclude: SerializableState;
max_size: $max_size;
);

$crate::buffer_ct_variable_zeroize_on_drop!($name, $out_size, $max_size);
}
}