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

DynDigest requires alloc when it does not have to #545

Closed
rvolgers opened this issue Feb 12, 2021 · 1 comment · Fixed by #573
Closed

DynDigest requires alloc when it does not have to #545

rvolgers opened this issue Feb 12, 2021 · 1 comment · Fixed by #573

Comments

@rvolgers
Copy link
Contributor

rvolgers commented Feb 12, 2021

It only provides functions which return the digest in a Box.

Perhaps this is under the assumption that DynDigest will always be boxed and so alloc is required anyway, but this does not have to be the case. You can have the digest living on the stack in a statically dispatched function, which then creates a &mut dyn DynDigest to pass it on to dynamically-dispatched functions. This might be done to keep code size down, because parameterizing a lot of functions by D: Digest will cause them all to be duplicated for every digest used in the program, which can be quite a lot if the digest to use can be specified in user input.

I propose adding finalize_slice and finalize_reset_slice to DynDigest and to Digest (TODO: ... and to one of the lower level traits, presumably?), which take a &mut [u8] and simply copy as many bytes of the digest into them as will fit. This makes these functions also useful for various use cases where hashes are used to expand key material into a variable sized stream of bytes.

The caller can simply use the existing output_size function to determine the size of the digest ahead of time, and use that information to pass an appropriate slice to finalize_slice.

Edit: originally I had the function names end in "_into_slice", but this makes it sound like it references the Into trait and related functions. Using just "_slice" sounds a bit more unique and may draw more attention to the unique part of its behavior, namely truncation if too short a slice is passed. Yak shaving welcome.

@rvolgers rvolgers mentioned this issue Feb 12, 2021
6 tasks
@newpavlov
Copy link
Member

newpavlov commented Feb 12, 2021

Yes, the trait was initially designed to be used together with boxed hash functions, but I think feature-gating only alloc dependent methods instead of the whole trait should be easy enough. Though I am not a fan of the proposed finalize_slice method, I think something like this could be a better option:

/// Finalize hasher and write result into provided buffer.
///
/// Length of a resulting slice will be equal to value returned by `output_size`.
/// `buf` length must be equal or bigger than it, otherwise method will return `Err(InvalidLength)`.
fn finalize_reset_into<'a>(&mut self, buf: &'a mut [u8]) -> Result<&'a [u8], InvalidLength>;

Also note that we can not add a consuming finalize_slice which would work with non-boxed hashers.

UPD: Hm, on a second thought, returning Result will almost always lead to uwnraps and because of the dynamic dispatch compiler will not be able to remove the panic branch, so the potentially truncating approach does have a certain merit.

tarcieri added a commit that referenced this issue Mar 4, 2021
Moves `alloc`-based gating for `DynDigest` to just the methods that use
`Box`, which enables usage of the trait in "heapless" environments.

Closes #545
tarcieri added a commit that referenced this issue Mar 4, 2021
Moves `alloc`-based gating for `DynDigest` to just the methods that use
`Box`, which enables usage of the trait in "heapless" environments.

Closes #545
tarcieri added a commit that referenced this issue Mar 5, 2021
Moves `alloc`-based gating for `DynDigest` to just the methods that use
`Box`, which enables usage of the trait in "heapless" environments.

Closes #545
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 a pull request may close this issue.

2 participants