| Message ID | cover.1727199118.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | hash.h: support choosing a separate SHA-1 for non-cryptographic uses | expand |
On Tue, Sep 24, 2024 at 01:32:01PM -0400, Taylor Blau wrote: > This version is another fairly substantial reroll, with the main > differences being renaming the "fast" SHA-1 options to "unsafe", as well > as not running the collision checks via finalize_object_file() when > handling loose objects (see the relevant patches for details on why). I left some rambling comments on the collision check changes. Nothing earth-shattering, but you may or may not want to tweak based on what I said. I'm happy with the s/fast/unsafe/ rename. The rest of it was more or less the same, and looks good to me. I do think with the extra collision-check it would be OK to teach index-pack to use the fast sha1, too. That would speed up receiving objects by the same absolute numbers (but less as a relative portion, since delta resolution is usually much more expensive). And it would also make verify-pack a bit faster, too (IIRC fsck for some reason does not use verify-pack, so its code is a potential candidate, too, if it's not already using csum-file). Those don't strictly need to come now, but it seems like they might be worthwhile while we're in the area (OTOH, using the fast hash only when sending is a belt-and-suspenders with the collision check). > Note also there is an important bug fix in finalize_object_file() to > unlink() the temporary file when we do run the collision check, but no > collisions were found. This bug was causing a pile-up of tmp_obj_XXXXXX > files in GitHub's infrastructure. Oops. :) I wondered if we could have a test here, but I don't think this can be easily triggered in the tests. The loose object collision happens only via TOCTOU race. I also tried instrumenting the code like below, and it triggers zero times in the test suite. diff --git a/object-file.c b/object-file.c index b9a3a1f62d..d9172df8d9 100644 --- a/object-file.c +++ b/object-file.c @@ -40,6 +40,7 @@ #include "fsck.h" #include "loose.h" #include "object-file-convert.h" +#include "trace.h" /* The maximum size for an object header. */ #define MAX_HEADER_LEN 32 @@ -1994,6 +1995,7 @@ int finalize_object_file(const char *tmpfile, const char *filename) int finalize_object_file_flags(const char *tmpfile, const char *filename, enum finalize_object_file_flags flags) { + static struct trace_key t = TRACE_KEY_INIT(COLLISION); struct stat st; int ret = 0; @@ -2031,6 +2033,8 @@ int finalize_object_file_flags(const char *tmpfile, const char *filename, errno = saved_errno; return error_errno(_("unable to write file %s"), filename); } + if (flags & FOF_SKIP_COLLISION_CHECK) + trace_printf_key(&t, "skipping check of %s and %s", tmpfile, filename); if (!(flags & FOF_SKIP_COLLISION_CHECK) && check_collision(tmpfile, filename)) return -1;
Here is a reroll of mine and Peff's series to add a build-time knob to allow selecting an alternative SHA-1 implementation for non-cryptographic hashing within Git, starting with the `hashwrite()` family of functions. This version is another fairly substantial reroll, with the main differences being renaming the "fast" SHA-1 options to "unsafe", as well as not running the collision checks via finalize_object_file() when handling loose objects (see the relevant patches for details on why). Note also there is an important bug fix in finalize_object_file() to unlink() the temporary file when we do run the collision check, but no collisions were found. This bug was causing a pile-up of tmp_obj_XXXXXX files in GitHub's infrastructure. Thanks in advance for your review! Taylor Blau (8): finalize_object_file(): check for name collision before renaming finalize_object_file(): refactor unlink_or_warn() placement finalize_object_file(): implement collision check pack-objects: use finalize_object_file() to rename pack/idx/etc sha1: do not redefine `platform_SHA_CTX` and friends hash.h: scaffolding for _unsafe hashing variants Makefile: allow specifying a SHA-1 for non-cryptographic uses csum-file.c: use unsafe SHA-1 implementation when available Makefile | 25 ++++++ block-sha1/sha1.h | 2 + csum-file.c | 18 ++-- hash.h | 72 +++++++++++++++ object-file.c | 124 ++++++++++++++++++++++++-- object-file.h | 6 ++ pack-write.c | 7 +- sha1/openssl.h | 2 + sha1dc_git.h | 3 + t/t5303-pack-corruption-resilience.sh | 7 +- tmp-objdir.c | 26 ++++-- 11 files changed, 266 insertions(+), 26 deletions(-) Range-diff against v3: 1: 738b1eb17b4 = 1: 6f1ee91fff3 finalize_object_file(): check for name collision before renaming 2: e1c2c39711f = 2: 133047ca8c9 finalize_object_file(): refactor unlink_or_warn() placement 3: 0feee5d1d4f ! 3: ed9eeef8513 finalize_object_file(): implement collision check @@ Commit message The new check will cause the write of new differing content to be a failure, rather than a silent noop, and we'll retain the temporary file - on disk. + on disk. If there's no collision present, we'll clean up the temporary + file as usual after either rename()-ing or link()-ing it into place. Note that this may cause some extra computation when the files are in - fact identical, but this should happen rarely. For example, when writing - a loose object, we compute the object id first, then check manually for - an existing object (so that we can freshen its timestamp) before - actually trying to write and link the new data. + fact identical, but this should happen rarely. + + Loose objects are exempt from this check, and the collision check may be + skipped by calling the _flags variant of this function with the + FOF_SKIP_COLLISION_CHECK bit set. This is done for a couple of reasons: + + - We don't treat the hash of the loose object file's contents as a + checksum, since the same loose object can be stored using different + bytes on disk (e.g., when adjusting core.compression, using a + different version of zlib, etc.). + + This is fundamentally different from cases where + finalize_object_file() is operating over a file which uses the hash + value as a checksum of the contents. In other words, a pair of + identical loose objects can be stored using different bytes on disk, + and that should not be treated as a collision. + + - We already use the path of the loose object as its hash value / + object name, so checking for collisions at the content level doesn't + add anything. + + This is why we do not bother to check the inflated object contents + for collisions either, since either (a) the object contents have the + fingerprint of a SHA-1 collision, in which case the collision + detecting SHA-1 implementation used to hash the contents to give us + a path would have already rejected it, or (b) the contents are part + of a colliding pair which does not bear the same fingerprints of + known collision attacks, in which case we would not have caught it + anyway. + + So skipping the collision check here does not change for better or + worse the hardness of loose object writes. + + As a small note related to the latter bullet point above, we must teach + the tmp-objdir routines to similarly skip the content-level collision + checks when calling migrate_one() on a loose object file, which we do by + setting the FOF_SKIP_COLLISION_CHECK bit when we are inside of a loose + object shard. Co-authored-by: Jeff King <peff@peff.net> Signed-off-by: Jeff King <peff@peff.net> @@ object-file.c: static void write_object_file_prepare_literally(const struct git_ /* * Move the just written object into its final resting place. */ + int finalize_object_file(const char *tmpfile, const char *filename) ++{ ++ return finalize_object_file_flags(tmpfile, filename, 0); ++} ++ ++int finalize_object_file_flags(const char *tmpfile, const char *filename, ++ enum finalize_object_file_flags flags) + { + struct stat st; + int ret = 0; @@ object-file.c: int finalize_object_file(const char *tmpfile, const char *filename) errno = saved_errno; return error_errno(_("unable to write file %s"), filename); } - /* FIXME!!! Collision check here ? */ -- unlink_or_warn(tmpfile); -+ if (check_collision(tmpfile, filename)) -+ return -1; ++ if (!(flags & FOF_SKIP_COLLISION_CHECK) && ++ check_collision(tmpfile, filename)) ++ return -1; + unlink_or_warn(tmpfile); } - out: +@@ object-file.c: static int write_loose_object(const struct object_id *oid, char *hdr, + warning_errno(_("failed utime() on %s"), tmp_file.buf); + } + +- return finalize_object_file(tmp_file.buf, filename.buf); ++ return finalize_object_file_flags(tmp_file.buf, filename.buf, ++ FOF_SKIP_COLLISION_CHECK); + } + + static int freshen_loose_object(const struct object_id *oid) +@@ object-file.c: int stream_loose_object(struct input_stream *in_stream, size_t len, + strbuf_release(&dir); + } + +- err = finalize_object_file(tmp_file.buf, filename.buf); ++ err = finalize_object_file_flags(tmp_file.buf, filename.buf, ++ FOF_SKIP_COLLISION_CHECK); + if (!err && compat) + err = repo_add_loose_object_map(the_repository, oid, &compat_oid); + cleanup: + + ## object-file.h ## +@@ object-file.h: int check_object_signature(struct repository *r, const struct object_id *oid, + */ + int stream_object_signature(struct repository *r, const struct object_id *oid); + ++enum finalize_object_file_flags { ++ FOF_SKIP_COLLISION_CHECK = 1, ++}; ++ + int finalize_object_file(const char *tmpfile, const char *filename); ++int finalize_object_file_flags(const char *tmpfile, const char *filename, ++ enum finalize_object_file_flags flags); + + /* Helper to check and "touch" a file */ + int check_and_freshen_file(const char *fn, int freshen); + + ## tmp-objdir.c ## +@@ tmp-objdir.c: static int read_dir_paths(struct string_list *out, const char *path) + return 0; + } + +-static int migrate_paths(struct strbuf *src, struct strbuf *dst); ++static int migrate_paths(struct strbuf *src, struct strbuf *dst, ++ enum finalize_object_file_flags flags); + +-static int migrate_one(struct strbuf *src, struct strbuf *dst) ++static int migrate_one(struct strbuf *src, struct strbuf *dst, ++ enum finalize_object_file_flags flags) + { + struct stat st; + +@@ tmp-objdir.c: static int migrate_one(struct strbuf *src, struct strbuf *dst) + return -1; + } else if (errno != EEXIST) + return -1; +- return migrate_paths(src, dst); ++ return migrate_paths(src, dst, flags); + } +- return finalize_object_file(src->buf, dst->buf); ++ return finalize_object_file_flags(src->buf, dst->buf, flags); + } + +-static int migrate_paths(struct strbuf *src, struct strbuf *dst) ++static int is_loose_object_shard(const char *name) ++{ ++ return strlen(name) == 2 && isxdigit(name[0]) && isxdigit(name[1]); ++} ++ ++static int migrate_paths(struct strbuf *src, struct strbuf *dst, ++ enum finalize_object_file_flags flags) + { + size_t src_len = src->len, dst_len = dst->len; + struct string_list paths = STRING_LIST_INIT_DUP; +@@ tmp-objdir.c: static int migrate_paths(struct strbuf *src, struct strbuf *dst) + + for (i = 0; i < paths.nr; i++) { + const char *name = paths.items[i].string; ++ enum finalize_object_file_flags flags_copy = flags; + + strbuf_addf(src, "/%s", name); + strbuf_addf(dst, "/%s", name); + +- ret |= migrate_one(src, dst); ++ if (is_loose_object_shard(name)) ++ flags_copy |= FOF_SKIP_COLLISION_CHECK; ++ ++ ret |= migrate_one(src, dst, flags_copy); + + strbuf_setlen(src, src_len); + strbuf_setlen(dst, dst_len); +@@ tmp-objdir.c: int tmp_objdir_migrate(struct tmp_objdir *t) + strbuf_addbuf(&src, &t->path); + strbuf_addstr(&dst, repo_get_object_directory(the_repository)); + +- ret = migrate_paths(&src, &dst); ++ ret = migrate_paths(&src, &dst, 0); + + strbuf_release(&src); + strbuf_release(&dst); 4: 620dde48a9d = 4: 3cc7f7b1f67 pack-objects: use finalize_object_file() to rename pack/idx/etc 5: bfe992765cd < -: ----------- i5500-git-daemon.sh: use compile-able version of Git without OpenSSL 6: 22863d9f6df = 5: 8f8ac0f5b0e sha1: do not redefine `platform_SHA_CTX` and friends 7: 119c318d812 ! 6: d300e9c6887 hash.h: scaffolding for _fast hashing variants @@ Metadata Author: Taylor Blau <me@ttaylorr.com> ## Commit message ## - hash.h: scaffolding for _fast hashing variants + hash.h: scaffolding for _unsafe hashing variants Git's default SHA-1 implementation is collision-detecting, which hardens us against known SHA-1 attacks against Git objects. This makes Git @@ Commit message the collision-detecting implementation, which is slower than non-collision detecting alternatives. - Prepare for loading a separate "fast" SHA-1 implementation that can be + Prepare for loading a separate "unsafe" 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 + control which implementation is used as the unsafe SHA-1 variant, but + does add scaffolding so that the "git_hash_algo" structure has five new + function pointers which are "unsafe" 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 + - git_hash_init_fn unsafe_init_fn + - git_hash_clone_fn unsafe_clone_fn + - git_hash_update_fn unsafe_update_fn + - git_hash_final_fn unsafe_final_fn + - git_hash_final_oid_fn unsafe_final_oid_fn The following commit will introduce compile-time knobs to specify which SHA-1 implementation is used for non-cryptographic uses. @@ hash.h #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 ++#ifndef platform_SHA_CTX_unsafe ++# define platform_SHA_CTX_unsafe platform_SHA_CTX ++# define platform_SHA1_Init_unsafe platform_SHA1_Init ++# define platform_SHA1_Update_unsafe platform_SHA1_Update ++# define platform_SHA1_Final_unsafe platform_SHA1_Final +# ifdef platform_SHA1_Clone -+# define platform_SHA1_Clone_fast platform_SHA1_Clone ++# define platform_SHA1_Clone_unsafe platform_SHA1_Clone +# endif +#endif + @@ hash.h #define git_SHA1_Update platform_SHA1_Update #define git_SHA1_Final platform_SHA1_Final -+#define git_SHA_CTX_fast platform_SHA_CTX_fast -+#define git_SHA1_Init_fast platform_SHA1_Init_fast -+#define git_SHA1_Update_fast platform_SHA1_Update_fast -+#define git_SHA1_Final_fast platform_SHA1_Final_fast ++#define git_SHA_CTX_unsafe platform_SHA_CTX_unsafe ++#define git_SHA1_Init_unsafe platform_SHA1_Init_unsafe ++#define git_SHA1_Update_unsafe platform_SHA1_Update_unsafe ++#define git_SHA1_Final_unsafe platform_SHA1_Final_unsafe + #ifdef platform_SHA1_Clone #define git_SHA1_Clone platform_SHA1_Clone #endif -+#ifdef platform_SHA1_Clone_fast -+# define git_SHA1_Clone_fast platform_SHA1_Clone_fast ++#ifdef platform_SHA1_Clone_unsafe ++# define git_SHA1_Clone_unsafe platform_SHA1_Clone_unsafe +#endif #ifndef platform_SHA256_CTX @@ hash.h: static inline void git_SHA1_Clone(git_SHA_CTX *dst, const git_SHA_CTX *s memcpy(dst, src, sizeof(*dst)); } #endif -+#ifndef SHA1_NEEDS_CLONE_HELPER_FAST -+static inline void git_SHA1_Clone_fast(git_SHA_CTX_fast *dst, -+ const git_SHA_CTX_fast *src) ++#ifndef SHA1_NEEDS_CLONE_HELPER_UNSAFE ++static inline void git_SHA1_Clone_unsafe(git_SHA_CTX_unsafe *dst, ++ const git_SHA_CTX_unsafe *src) +{ + memcpy(dst, src, sizeof(*dst)); +} @@ hash.h: enum get_oid_result { /* A suitably aligned type for stack allocations of hash contexts. */ union git_hash_ctx { git_SHA_CTX sha1; -+ git_SHA_CTX_fast sha1_fast; ++ git_SHA_CTX_unsafe sha1_unsafe; + git_SHA256_CTX sha256; }; @@ hash.h: struct git_hash_algo { /* The hash finalization function for object IDs. */ git_hash_final_oid_fn final_oid_fn; -+ /* The fast / non-cryptographic hash initialization function. */ -+ git_hash_init_fn fast_init_fn; ++ /* The non-cryptographic hash initialization function. */ ++ git_hash_init_fn unsafe_init_fn; + -+ /* The fast / non-cryptographic hash context cloning function. */ -+ git_hash_clone_fn fast_clone_fn; ++ /* The non-cryptographic hash context cloning function. */ ++ git_hash_clone_fn unsafe_clone_fn; + -+ /* The fast / non-cryptographic hash update function. */ -+ git_hash_update_fn fast_update_fn; ++ /* The non-cryptographic hash update function. */ ++ git_hash_update_fn unsafe_update_fn; + -+ /* The fast / non-cryptographic hash finalization function. */ -+ git_hash_final_fn fast_final_fn; ++ /* The non-cryptographic hash finalization function. */ ++ git_hash_final_fn unsafe_final_fn; + -+ /* The fast / non-cryptographic hash finalization function. */ -+ git_hash_final_oid_fn fast_final_oid_fn; ++ /* The non-cryptographic hash finalization function. */ ++ git_hash_final_oid_fn unsafe_final_oid_fn; + /* The OID of the empty tree. */ const struct object_id *empty_tree; @@ object-file.c: static void git_hash_sha1_final_oid(struct object_id *oid, git_ha oid->algo = GIT_HASH_SHA1; } -+static void git_hash_sha1_init_fast(git_hash_ctx *ctx) ++static void git_hash_sha1_init_unsafe(git_hash_ctx *ctx) +{ -+ git_SHA1_Init_fast(&ctx->sha1_fast); ++ git_SHA1_Init_unsafe(&ctx->sha1_unsafe); +} + -+static void git_hash_sha1_clone_fast(git_hash_ctx *dst, const git_hash_ctx *src) ++static void git_hash_sha1_clone_unsafe(git_hash_ctx *dst, const git_hash_ctx *src) +{ -+ git_SHA1_Clone_fast(&dst->sha1_fast, &src->sha1_fast); ++ git_SHA1_Clone_unsafe(&dst->sha1_unsafe, &src->sha1_unsafe); +} + -+static void git_hash_sha1_update_fast(git_hash_ctx *ctx, const void *data, ++static void git_hash_sha1_update_unsafe(git_hash_ctx *ctx, const void *data, + size_t len) +{ -+ git_SHA1_Update_fast(&ctx->sha1_fast, data, len); ++ git_SHA1_Update_unsafe(&ctx->sha1_unsafe, data, len); +} + -+static void git_hash_sha1_final_fast(unsigned char *hash, git_hash_ctx *ctx) ++static void git_hash_sha1_final_unsafe(unsigned char *hash, git_hash_ctx *ctx) +{ -+ git_SHA1_Final_fast(hash, &ctx->sha1_fast); ++ git_SHA1_Final_unsafe(hash, &ctx->sha1_unsafe); +} + -+static void git_hash_sha1_final_oid_fast(struct object_id *oid, git_hash_ctx *ctx) ++static void git_hash_sha1_final_oid_unsafe(struct object_id *oid, git_hash_ctx *ctx) +{ -+ git_SHA1_Final_fast(oid->hash, &ctx->sha1_fast); ++ git_SHA1_Final_unsafe(oid->hash, &ctx->sha1_unsafe); + memset(oid->hash + GIT_SHA1_RAWSZ, 0, GIT_MAX_RAWSZ - GIT_SHA1_RAWSZ); + oid->algo = GIT_HASH_SHA1; +} @@ object-file.c: const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = { .update_fn = git_hash_unknown_update, .final_fn = git_hash_unknown_final, .final_oid_fn = git_hash_unknown_final_oid, -+ .fast_init_fn = git_hash_unknown_init, -+ .fast_clone_fn = git_hash_unknown_clone, -+ .fast_update_fn = git_hash_unknown_update, -+ .fast_final_fn = git_hash_unknown_final, -+ .fast_final_oid_fn = git_hash_unknown_final_oid, ++ .unsafe_init_fn = git_hash_unknown_init, ++ .unsafe_clone_fn = git_hash_unknown_clone, ++ .unsafe_update_fn = git_hash_unknown_update, ++ .unsafe_final_fn = git_hash_unknown_final, ++ .unsafe_final_oid_fn = git_hash_unknown_final_oid, .empty_tree = NULL, .empty_blob = NULL, .null_oid = NULL, @@ object-file.c: const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = { .update_fn = git_hash_sha1_update, .final_fn = git_hash_sha1_final, .final_oid_fn = git_hash_sha1_final_oid, -+ .fast_init_fn = git_hash_sha1_init_fast, -+ .fast_clone_fn = git_hash_sha1_clone_fast, -+ .fast_update_fn = git_hash_sha1_update_fast, -+ .fast_final_fn = git_hash_sha1_final_fast, -+ .fast_final_oid_fn = git_hash_sha1_final_oid_fast, ++ .unsafe_init_fn = git_hash_sha1_init_unsafe, ++ .unsafe_clone_fn = git_hash_sha1_clone_unsafe, ++ .unsafe_update_fn = git_hash_sha1_update_unsafe, ++ .unsafe_final_fn = git_hash_sha1_final_unsafe, ++ .unsafe_final_oid_fn = git_hash_sha1_final_oid_unsafe, .empty_tree = &empty_tree_oid, .empty_blob = &empty_blob_oid, .null_oid = &null_oid_sha1, @@ object-file.c: 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, ++ .unsafe_init_fn = git_hash_sha256_init, ++ .unsafe_clone_fn = git_hash_sha256_clone, ++ .unsafe_update_fn = git_hash_sha256_update, ++ .unsafe_final_fn = git_hash_sha256_final, ++ .unsafe_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, 8: 137ec30d68a ! 7: af8fd9aa4ed Makefile: allow specifying a SHA-1 for non-cryptographic uses @@ Metadata ## Commit message ## Makefile: allow specifying a SHA-1 for non-cryptographic uses - Introduce _FAST variants of the OPENSSL_SHA1, BLK_SHA1, and + Introduce _UNSAFE variants of the OPENSSL_SHA1, BLK_SHA1, and APPLE_COMMON_CRYPTO_SHA1 compile-time knobs which indicate which SHA-1 implementation is to be used for non-cryptographic uses. @@ Makefile: include shared.mak # Define APPLE_COMMON_CRYPTO_SHA1 to use Apple's CommonCrypto for # SHA-1. # -+# Define the same Makefile knobs as above, but suffixed with _FAST to -+# use the corresponding implementations for "fast" SHA-1 hashing for ++# Define the same Makefile knobs as above, but suffixed with _UNSAFE to ++# use the corresponding implementations for unsafe SHA-1 hashing for +# non-cryptographic purposes. +# # If don't enable any of the *_SHA1 settings in this section, Git will @@ Makefile: endif endif endif -+ifdef OPENSSL_SHA1_FAST ++ifdef OPENSSL_SHA1_UNSAFE +ifndef OPENSSL_SHA1 + EXTLIBS += $(LIB_4_CRYPTO) -+ BASIC_CFLAGS += -DSHA1_OPENSSL_FAST ++ BASIC_CFLAGS += -DSHA1_OPENSSL_UNSAFE +endif +else -+ifdef BLK_SHA1_FAST ++ifdef BLK_SHA1_UNSAFE +ifndef BLK_SHA1 + LIB_OBJS += block-sha1/sha1.o -+ BASIC_CFLAGS += -DSHA1_BLK_FAST ++ BASIC_CFLAGS += -DSHA1_BLK_UNSAFE +endif +else -+ifdef APPLE_COMMON_CRYPTO_SHA1_FAST ++ifdef APPLE_COMMON_CRYPTO_SHA1_UNSAFE +ifndef APPLE_COMMON_CRYPTO_SHA1 + COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL -+ BASIC_CFLAGS += -DSHA1_APPLE_FAST ++ BASIC_CFLAGS += -DSHA1_APPLE_UNSAFE +endif +endif +endif @@ hash.h #include "block-sha1/sha1.h" #endif -+#if defined(SHA1_APPLE_FAST) ++#if defined(SHA1_APPLE_UNSAFE) +# include <CommonCrypto/CommonDigest.h> -+# define platform_SHA_CTX_fast CC_SHA1_CTX -+# define platform_SHA1_Init_fast CC_SHA1_Init -+# define platform_SHA1_Update_fast CC_SHA1_Update -+# define platform_SHA1_Final_fast CC_SHA1_Final -+#elif defined(SHA1_OPENSSL_FAST) ++# define platform_SHA_CTX_unsafe CC_SHA1_CTX ++# define platform_SHA1_Init_unsafe CC_SHA1_Init ++# define platform_SHA1_Update_unsafe CC_SHA1_Update ++# define platform_SHA1_Final_unsafe CC_SHA1_Final ++#elif defined(SHA1_OPENSSL_UNSAFE) +# include <openssl/sha.h> +# if defined(OPENSSL_API_LEVEL) && OPENSSL_API_LEVEL >= 3 -+# define SHA1_NEEDS_CLONE_HELPER_FAST ++# define SHA1_NEEDS_CLONE_HELPER_UNSAFE +# include "sha1/openssl.h" -+# define platform_SHA_CTX_fast openssl_SHA1_CTX -+# define platform_SHA1_Init_fast openssl_SHA1_Init -+# define platform_SHA1_Clone_fast openssl_SHA1_Clone -+# define platform_SHA1_Update_fast openssl_SHA1_Update -+# define platform_SHA1_Final_fast openssl_SHA1_Final ++# define platform_SHA_CTX_unsafe openssl_SHA1_CTX ++# define platform_SHA1_Init_unsafe openssl_SHA1_Init ++# define platform_SHA1_Clone_unsafe openssl_SHA1_Clone ++# define platform_SHA1_Update_unsafe openssl_SHA1_Update ++# define platform_SHA1_Final_unsafe openssl_SHA1_Final +# else -+# define platform_SHA_CTX_fast SHA_CTX -+# define platform_SHA1_Init_fast SHA1_Init -+# define platform_SHA1_Update_fast SHA1_Update -+# define platform_SHA1_Final_fast SHA1_Final ++# define platform_SHA_CTX_unsafe SHA_CTX ++# define platform_SHA1_Init_unsafe SHA1_Init ++# define platform_SHA1_Update_unsafe SHA1_Update ++# define platform_SHA1_Final_unsafe SHA1_Final +# endif -+#elif defined(SHA1_BLK_FAST) ++#elif defined(SHA1_BLK_UNSAFE) +# include "block-sha1/sha1.h" -+# define platform_SHA_CTX_fast blk_SHA_CTX -+# define platform_SHA1_Init_fast blk_SHA1_Init -+# define platform_SHA1_Update_fast blk_SHA1_Update -+# define platform_SHA1_Final_fast blk_SHA1_Final ++# define platform_SHA_CTX_unsafe blk_SHA_CTX ++# define platform_SHA1_Init_unsafe blk_SHA1_Init ++# define platform_SHA1_Update_unsafe blk_SHA1_Update ++# define platform_SHA1_Final_unsafe blk_SHA1_Final +#endif + #if defined(SHA256_NETTLE) 9: 4018261366f ! 8: 4b83dd05e9f csum-file.c: use fast SHA-1 implementation when available @@ Metadata Author: Taylor Blau <me@ttaylorr.com> ## Commit message ## - csum-file.c: use fast SHA-1 implementation when available + csum-file.c: use unsafe SHA-1 implementation when available - Update hashwrite() and friends to use the fast_-variants of hashing - functions, calling for e.g., "the_hash_algo->fast_update_fn()" instead + Update hashwrite() and friends to use the unsafe_-variants of hashing + functions, calling for e.g., "the_hash_algo->unsafe_update_fn()" instead of "the_hash_algo->update_fn()". These callers only use the_hash_algo to produce a checksum, which we depend on for data integrity, but not for cryptographic purposes, so - these callers are safe to use the fast (and potentially non-collision + these callers are safe to use the unsafe (and potentially non-collision detecting) SHA-1 implementation. To time this, I took a freshly packed copy of linux.git, and ran the - following with and without the OPENSSL_SHA1_FAST=1 build-knob. Both + following with and without the OPENSSL_SHA1_UNSAFE=1 build-knob. Both versions were compiled with -O3: $ git for-each-ref --format='%(objectname)' refs/heads refs/tags >in $ valgrind --tool=callgrind ~/src/git/git-pack-objects \ --revs --stdout --all-progress --use-bitmap-index <in >/dev/null - Without OPENSSL_SHA1_FAST=1 (that is, using the collision-detecting + Without OPENSSL_SHA1_UNSAFE=1 (that is, using the collision-detecting SHA-1 implementation for both cryptographic and non-cryptographic purposes), we spend a significant amount of our instruction count in hashwrite(): @@ Commit message , and the resulting "clone" takes 19.219 seconds of wall clock time, 18.94 seconds of user time and 0.28 seconds of system time. - Compiling with OPENSSL_SHA1_FAST=1, we spend ~60% fewer instructions in - hashwrite(): + Compiling with OPENSSL_SHA1_UNSAFE=1, we spend ~60% fewer instructions + in hashwrite(): $ callgrind_annotate --inclusive=yes | grep hashwrite | head -n1 59,164,001,176 (58.79%) /home/ttaylorr/src/git/csum-file.c:hashwrite [/home/ttaylorr/src/git/git-pack-objects] - , and generate the resulting "clone" much faster, in only 11.597 seconds + , and generate the resulting "clone" much unsafeer, in only 11.597 seconds of wall time, 11.37 seconds of user time, and 0.23 seconds of system time, for a ~40% speed-up. @@ csum-file.c: void hashflush(struct hashfile *f) if (offset) { if (!f->skip_hash) - the_hash_algo->update_fn(&f->ctx, f->buffer, offset); -+ the_hash_algo->fast_update_fn(&f->ctx, f->buffer, offset); ++ the_hash_algo->unsafe_update_fn(&f->ctx, f->buffer, offset); flush(f, f->buffer, offset); f->offset = 0; } @@ csum-file.c: int finalize_hashfile(struct hashfile *f, unsigned char *result, hashclr(f->buffer, the_repository->hash_algo); else - the_hash_algo->final_fn(f->buffer, &f->ctx); -+ the_hash_algo->fast_final_fn(f->buffer, &f->ctx); ++ the_hash_algo->unsafe_final_fn(f->buffer, &f->ctx); if (result) hashcpy(result, f->buffer, the_repository->hash_algo); @@ csum-file.c: void hashwrite(struct hashfile *f, const void *buf, unsigned int co */ if (!f->skip_hash) - the_hash_algo->update_fn(&f->ctx, buf, nr); -+ the_hash_algo->fast_update_fn(&f->ctx, buf, nr); ++ the_hash_algo->unsafe_update_fn(&f->ctx, buf, nr); flush(f, buf, nr); } else { /* @@ csum-file.c: static struct hashfile *hashfd_internal(int fd, const char *name, f->do_crc = 0; f->skip_hash = 0; - the_hash_algo->init_fn(&f->ctx); -+ the_hash_algo->fast_init_fn(&f->ctx); ++ the_hash_algo->unsafe_init_fn(&f->ctx); f->buffer_len = buffer_len; f->buffer = xmalloc(buffer_len); @@ csum-file.c: void hashfile_checkpoint(struct hashfile *f, struct hashfile_checkp hashflush(f); checkpoint->offset = f->total; - the_hash_algo->clone_fn(&checkpoint->ctx, &f->ctx); -+ the_hash_algo->fast_clone_fn(&checkpoint->ctx, &f->ctx); ++ the_hash_algo->unsafe_clone_fn(&checkpoint->ctx, &f->ctx); } int hashfile_truncate(struct hashfile *f, struct hashfile_checkpoint *checkpoint) @@ csum-file.c: int hashfile_truncate(struct hashfile *f, struct hashfile_checkpoin return -1; f->total = offset; - the_hash_algo->clone_fn(&f->ctx, &checkpoint->ctx); -+ the_hash_algo->fast_clone_fn(&f->ctx, &checkpoint->ctx); ++ the_hash_algo->unsafe_clone_fn(&f->ctx, &checkpoint->ctx); f->offset = 0; /* hashflush() was called in checkpoint */ return 0; } @@ csum-file.c: int hashfile_checksum_valid(const unsigned char *data, size_t total - the_hash_algo->init_fn(&ctx); - the_hash_algo->update_fn(&ctx, data, data_len); - the_hash_algo->final_fn(got, &ctx); -+ the_hash_algo->fast_init_fn(&ctx); -+ the_hash_algo->fast_update_fn(&ctx, data, data_len); -+ the_hash_algo->fast_final_fn(got, &ctx); ++ the_hash_algo->unsafe_init_fn(&ctx); ++ the_hash_algo->unsafe_update_fn(&ctx, data, data_len); ++ the_hash_algo->unsafe_final_fn(got, &ctx); return hasheq(got, data + data_len, the_repository->hash_algo); } base-commit: 6258f68c3c1092c901337895c864073dcdea9213