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

digest: add FixedOutputDirty trait + finalize_into* #180

Merged
merged 1 commit into from Jun 9, 2020

Conversation

tarcieri
Copy link
Member

@tarcieri tarcieri commented Jun 9, 2020

Closes RustCrypto/hashes#86

Replaces the Reset trait with a set of _reset methods on all of the various traits, and uses these as the default impl for the methods which consume hashers.

Also adds a set of methods to FixedOutput (finalize_fixed_into*) which eliminate copies by accepting an existing buffer as an argument.

Adds a FixedOutputDirty trait which writes the digest output to a provided byte array, but does not reset the internal state. This is intended for implementations to use in order to ensure that they are not reset in the event the instance is consumed.

Also adds a set of finalize_into and finalize_into_reset methods to FixedOutput whhich also write their input into a provided byte array, and changes the existing finalize_fixed (and newly added finalize_fixed_reset) methods to have a default implementation which returns a byte array allocated on the stack.

Finally, adds a blanket impl of FixedOutput for FixedOutputDirty + Reset types which handles safely invoking the underlying implementation by either consuming the instance (avoiding a reset) or borrowing the hasher, obtaining the output, and resetting.

@tarcieri tarcieri requested a review from newpavlov June 9, 2020 14:51
@tarcieri
Copy link
Member Author

tarcieri commented Jun 9, 2020

@newpavlov this is slightly different from what you originally proposed, but I think achieves the same effect. Namely if you wanted something exactly like what you had before, it could be achieved with:

impl MyDigest {
    /// Private method not intended to be used directly
    fn finalize_fixed_core(&mut self, out: &mut GenericArray<u8, Self::OutputSize>, reset: bool) {
        [...]
    }
}

impl FixedOutput for MyDigest{
    type OutputSize = [...];

    #[inline]
    fn finalize_fixed(self) -> GenericArray<u8, Self::OutputSize> {
        let mut buf = Default::default();
        self.finalize_fixed_core(&mut buf, false);
        buf
    }

    #[inline]
    fn finalize_fixed_reset(&mut self) -> GenericArray<u8, Self::OutputSize> { .. }
    fn finalize_fixed_into(self, out: &mut GenericArray<u8, Self::OutputSize>) { .. }
    fn finalize_fixed_into_reset(&mut self, out: &mut GenericArray<u8, Self::OutputSize>) { .. }
}

...it's just by not making it part of the trait, it could actually be a private method.

digest/src/dyn_digest.rs Outdated Show resolved Hide resolved
@tarcieri tarcieri force-pushed the digest/finalize-fixed-into branch from 47d5f10 to a2f119e Compare June 9, 2020 15:02
Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

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

One problem with this approach is that it makes harder for compiler to eliminate reset code, this is why I was thinking about using the private method, which would keep reset code behind a simple branch which can eliminated by trivial const propagation. Before going with this approach, it would be nice to see that compiler is indeed will be able to properly remove reset code for finalize_fixed.

@tarcieri
Copy link
Member Author

tarcieri commented Jun 9, 2020

That's only if you rely on the default implementation though. If you want to avoid the resets entirely, just define your own implementation (per above) which doesn't invoke the reset code at all.

@newpavlov
Copy link
Member

newpavlov commented Jun 9, 2020

This will lead to unnecessary code duplication, which I would like to avoid.

How about something like this?

pub trait FixedOutput {
    // ...

    /// Retrieve result into provided buffer and leave hasher in a dirty state.
    /// Usage of this method in user code is discouraged, prefer `finalize_fixed`
    /// or `finalize_fixed_reset`.
    fn finalize_into_dirty(&mut self, out: &mut GenericArray<u8, Self::OutputSize>);

    fn finalize_into_reset(&mut self, out: &mut GenericArray<u8, Self::OutputSize>)
    where
        Self: Reset,
    {
        self.finalize_into_dirty(out);
        self.reset();
    }
}

pub trait Reset {
    fn reset(&mut self);
}

Yes, users may misuse finalize_into_dirty (use a scarier name?), but I don't think it will be a significant issue in practice.

@tarcieri
Copy link
Member Author

tarcieri commented Jun 9, 2020

I think you could achieve something like that with a separate trait and a blanket impl:

pub trait FixedOutputDirty {
    /// Retrieve result into provided buffer and leave hasher in a dirty state.
    /// Usage of this method in user code is discouraged, prefer `finalize_fixed`
    /// or `finalize_fixed_reset`.
    fn finalize_into_dirty(&mut self, out: &mut GenericArray<u8, Self::OutputSize>);
}

impl<D: FixedOutputDirty + Reset> FixedOutput for D {
    #[inline]
    fn finalize_into(&mut self, out: &mut GenericArray<u8, Self::OutputSize>)
    where
        Self: Reset,
    {
        self.finalize_into_dirty(out);
    }

    #[inline]
    fn finalize_into_reset(&mut self, out: &mut GenericArray<u8, Self::OutputSize>)
    where
        Self: Reset,
    {
        self.finalize_into_dirty(out);
        self.reset();
    }
}

@tarcieri
Copy link
Member Author

tarcieri commented Jun 9, 2020

Also should I shorten finalize_fixed_into to finalize_into? Seems ok to me.

@newpavlov
Copy link
Member

The separate trait looks good, making room for FixedOutput to be used separately from the dirty trait (e.g. for HW accelerated impls) is certainly nice.

Also should I shorten finalize_fixed_into to finalize_into?

Yes, sounds good.

@tarcieri tarcieri force-pushed the digest/finalize-fixed-into branch from a2f119e to 7ef7bee Compare June 9, 2020 17:14
@tarcieri tarcieri changed the title digest: replace Reset trait with methods; add *into* methods digest: add FixedOutputDirty trait + finalize_into* Jun 9, 2020
@tarcieri tarcieri force-pushed the digest/finalize-fixed-into branch from 7ef7bee to daf6cfb Compare June 9, 2020 17:17
@tarcieri
Copy link
Member Author

tarcieri commented Jun 9, 2020

Updated to use a FixedOutputDirty trait + blanket impl. I like this approach especially because it's a much less invasive change.

@tarcieri tarcieri force-pushed the digest/finalize-fixed-into branch from daf6cfb to 4f53f96 Compare June 9, 2020 17:19
Adds a `FixedOutputDirty` trait which writes the digest output to a
provided byte array, but does not reset the internal state. This is
intended for implementations to use in order to ensure that they are
not reset in the event the instance is consumed.

Also adds a set of `finalize_into` and `finalize_into_reset` methods to
`FixedOutput` whhich also write their input into a provided byte array,
and changes the existing `finalize_fixed` (and newly added
`finalize_fixed_reset`) methods to have a default implementation which
returns a byte array allocated on the stack.

Finally, adds a blanket impl of `FixedOutput` for `FixedOutputDirty` +
`Reset` types which handles safely invoking the underlying
implementation by either consuming the instance (avoiding a reset)
or borrowing the hasher, obtaining the output, and resetting.
@tarcieri tarcieri force-pushed the digest/finalize-fixed-into branch from 4f53f96 to 2526a6a Compare June 9, 2020 17:24
@tarcieri tarcieri merged commit 496aa73 into master Jun 9, 2020
@tarcieri tarcieri deleted the digest/finalize-fixed-into branch June 9, 2020 17:30
@newpavlov
Copy link
Member

BTW should we do something similar for XOF and VariableOutput?

@tarcieri
Copy link
Member Author

tarcieri commented Jun 9, 2020

Sure

tarcieri added a commit that referenced this pull request Jun 9, 2020
Adds traits which provide an implementation-focused low-level API
similar to the `FixedOutputDirty` trait introduced in #180,
but for the `ExtendableOutput` and `VariableOutput` traits.

Like `FixedOutputDirty`, these traits have blanket impls when the same
type also impls the `Reset` trait.

Also adds corresponding `*_reset` methods to the respective `finalize*`
methods of these traits.
tarcieri added a commit that referenced this pull request Jun 9, 2020
…183)

Adds traits which provide an implementation-focused low-level API
similar to the `FixedOutputDirty` trait introduced in #180,
but for the `ExtendableOutput` and `VariableOutput` traits.

Like `FixedOutputDirty`, these traits have blanket impls when the same
type also impls the `Reset` trait.

Also adds corresponding `*_reset` methods to the respective `finalize*`
methods of these traits.
@tarcieri tarcieri mentioned this pull request Jun 10, 2020
dns2utf8 pushed a commit to dns2utf8/traits that referenced this pull request Jan 24, 2023
Adds a proper conversion between argon2::Error and password_hash::Error
and uses it when hashing the password.
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.

Add fixed_result_into to prevent copy
2 participants