Re: [PATCH 2/4] hash.h: scaffolding for _fast hashing variants
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Sep 2, 2024, 13:41 UTC
- Message-ID
- <ZtXAhP69zu7cDnsA@tanuki>
- In-Reply-To
- <6ac6f934c32bdc600cdb8d2a08d4aa390c1f2994.1725206584.git.me@ttaylorr.com>
On Sun, Sep 01, 2024 at 12:03:24PM -0400, Taylor Blau wrote:
Show 24 quoted lines
> Git's default SHA-1 implementation is collision-detecting, which hardens > us against known SHA-1 attacks against Git objects. This makes Git > object writes safer at the expense of some speed when hashing through > the collision-detecting implementation, which is slower than > non-collision detecting alternatives. > > Prepare for loading a separate "fast" SHA-1 implementation that can be > used for non-cryptographic purposes, like computing the checksum of > files that use the hashwrite() API. > > This commit does not actually introduce any new compile-time knobs to > control which implementation is used as the fast SHA-1 variant, but does > add scaffolding so that the "git_hash_algo" structure has five new > function pointers which are "fast" variants of the five existing > hashing-related function pointers: > > - git_hash_init_fn fast_init_fn > - git_hash_clone_fn fast_clone_fn > - git_hash_update_fn fast_update_fn > - git_hash_final_fn fast_final_fn > - git_hash_final_oid_fn fast_final_oid_fn > > The following commit will introduce compile-time knobs to specify which > SHA-1 implementation is used for non-cryptographic uses.
While the property we care about in the context of this patch series indeed is that the second hash is faster, I think the more important property is that it's insecure. If I were seeing two APIs, one labelled fast and one labelled slow, I would of course pick the fast one. So I wonder whether we should rename things accordingly so that developers aren't intrigued to pick the fast one without thinking, and also to have a more useful signal that stands out to reviewers.
Show 23 quoted lines
> Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > hash.h | 42 ++++++++++++++++++++++++++++++++++++++++++ > object-file.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 84 insertions(+) > > diff --git a/hash.h b/hash.h > index 72ffbc862e5..f255e5c1e8a 100644 > --- a/hash.h > +++ b/hash.h > @@ -44,14 +44,32 @@ > #define platform_SHA1_Final SHA1_Final > #endif > > +#ifndef platform_SHA_CTX_fast > +#define platform_SHA_CTX_fast platform_SHA_CTX > +#define platform_SHA1_Init_fast platform_SHA1_Init > +#define platform_SHA1_Update_fast platform_SHA1_Update > +#define platform_SHA1_Final_fast platform_SHA1_Final > +#ifdef platform_SHA1_Clone > +#define platform_SHA1_Clone_fast platform_SHA1_Clone > +#endif > +#endif
We may want to apply our new coding guidelines around nested preprocessor directives, which should also use indenting.
Show 5 quoted lines
> @@ -222,6 +249,21 @@ struct git_hash_algo {
> /* The hash finalization function for object IDs. */
> git_hash_final_oid_fn final_oid_fn;
>
> + /* The fast hash initialization function. */Providing some context here why there are two sets of functions would help future readers.
Show 12 quoted lines
> @@ -219,6 +256,11 @@ const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {
> .update_fn = git_hash_sha256_update,
> .final_fn = git_hash_sha256_final,
> .final_oid_fn = git_hash_sha256_final_oid,
> + .fast_init_fn = git_hash_sha256_init,
> + .fast_clone_fn = git_hash_sha256_clone,
> + .fast_update_fn = git_hash_sha256_update,
> + .fast_final_fn = git_hash_sha256_final,
> + .fast_final_oid_fn = git_hash_sha256_final_oid,
> .empty_tree = &empty_tree_oid_sha256,
> .empty_blob = &empty_blob_oid_sha256,
> .null_oid = &null_oid_sha256,I was briefly wondering whether we'd rather want to have automatic fallbacks to the secure alternative when the fast one isn't set. I guess in the end that's not really worth it though, as it saves us one branch when not having such a fallback. And we only have so many hashes, so it's not like this would be a huge pain to maintain.
Patrick