Use SipHasher rather than SHA-512 for ISLE manifest.#3619
Merged
Conversation
bjorn3
approved these changes
Dec 17, 2021
alexcrichton
approved these changes
Dec 17, 2021
Fixes bytecodealliance#3609. It turns out that `sha2` is a nontrivial dependency for Cranelift in many contexts, partly because it pulls in a number of other crates as well. One option is to remove the hash check under certain circumstances, as implemented in bytecodealliance#3616. However, this is undesirable for other reasons: having different dependency options in Wasmtime in particular for crates.io vs. local builds is not really possible, and so either we still have the higher build cost in Wasmtime, or we turn off the checks by default, which goes against the original intent of ensuring developer safety (no mysterious stale-source bugs). This PR uses `SipHash` instead, which is built into the standard library. `SipHash` is deprecated, but it's fixed and deterministic (across runs and across Rust versions), which is what we need, unlike the suggested replacement `std::collections::hash_map::DefaultHasher`. The result is only 64 bits, and is not cryptographically secure, but we never needed that; we just need a simple check to indicate when we forget a `rebuild-isle`.
cfallin
force-pushed
the
isle-manifest-siphash
branch
from
3bb9763 to
5233175
Compare
github-actions
Bot
added
cranelift
Subscribe to Label ActionDetailsThis issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:x64", "isle"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3609. It turns out that
sha2is a nontrivial dependency forCranelift in many contexts, partly because it pulls in a number of other
crates as well.
One option is to remove the hash check under certain circumstances, as
implemented in #3616. However, this is undesirable for other reasons:
having different dependency options in Wasmtime in particular for
crates.io vs. local builds is not really possible, and so either we
still have the higher build cost in Wasmtime, or we turn off the checks
by default, which goes against the original intent of ensuring developer
safety (no mysterious stale-source bugs).
This PR uses
SipHashinstead, which is built into the standardlibrary.
SipHashis deprecated, but it's fixed and deterministic(across runs and across Rust versions), which is what we need, unlike
the suggested replacement
std::collections::hash_map::DefaultHasher.The result is only 64 bits, and is not cryptographically secure, but we
never needed that; we just need a simple check to indicate when we
forget a
rebuild-isle.