csum-file: introduce hashfile_checkpoint_init()

In 106140a99f (builtin/fast-import: fix segfault with unsafe SHA1
backend, 2024-12-30) and 9218c0bfe1 (bulk-checkin: fix segfault with
unsafe SHA1 backend, 2024-12-30), we observed the effects of failing to
initialize a hashfile_checkpoint with the same hash function
implementation as is used by the hashfile it is used to checkpoint.

While both 106140a99f and 9218c0bfe1 work around the immediate crash,
changing the hash function implementation within the hashfile API to,
for example, the non-unsafe variant would re-introduce the crash. This
is a result of the tight coupling between initializing hashfiles and
hashfile_checkpoints.

Introduce and use a new function which ensures that both parts of a
hashfile and hashfile_checkpoint pair use the same hash function
implementation to avoid such crashes.

A few things worth noting:

  - In the change to builtin/fast-import.c::stream_blob(), we can see
    that by removing the explicit reference to
    'the_hash_algo->unsafe_init_fn()', we are hardened against the
    hashfile API changing away from the_hash_algo (or its unsafe
    variant) in the future.

  - The bulk-checkin code no longer needs to explicitly zero-initialize
    the hashfile_checkpoint, since it is now done as a result of calling
    'hashfile_checkpoint_init()'.

  - Also in the bulk-checkin code, we add an additional call to
    prepare_to_stream() outside of the main loop in order to initialize
    'state->f' so we know which hash function implementation to use when
    calling 'hashfile_checkpoint_init()'.

    This is OK, since subsequent 'prepare_to_stream()' calls are noops.
    However, we only need to call 'prepare_to_stream()' when we have the
    HASH_WRITE_OBJECT bit set in our flags. Without that bit, calling
    'prepare_to_stream()' does not assign 'state->f', so we have nothing
    to initialize.

  - Other uses of the 'checkpoint' in 'deflate_blob_to_pack()' are
    appropriately guarded.

Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 0f86392761abbe6acb217fef7f4fe7c3ff5ac1fa..4a6c7ab52accfe7d2a56b70a32f3e74307b4caf8 100644 (file)
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -1106,7 +1106,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)
                || (pack_size + PACK_SIZE_THRESHOLD + len) < pack_size)
                cycle_packfile();
 
-       the_hash_algo->unsafe_init_fn(&checkpoint.ctx);
+       hashfile_checkpoint_init(pack_file, &checkpoint);
        hashfile_checkpoint(pack_file, &checkpoint);
        offset = checkpoint.offset;
 

diff --git a/bulk-checkin.c b/bulk-checkin.c
index 433070a3bda461a2ad62da67cc5515f8822d2df3..892176d23d2e99e5121f8027c3263152ce8c800a 100644 (file)
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -261,7 +261,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
        git_hash_ctx ctx;
        unsigned char obuf[16384];
        unsigned header_len;
-       struct hashfile_checkpoint checkpoint = {0};
+       struct hashfile_checkpoint checkpoint;
        struct pack_idx_entry *idx = NULL;
 
        seekback = lseek(fd, 0, SEEK_CUR);
@@ -272,12 +272,15 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
                                          OBJ_BLOB, size);
        the_hash_algo->init_fn(&ctx);
        the_hash_algo->update_fn(&ctx, obuf, header_len);
-       the_hash_algo->unsafe_init_fn(&checkpoint.ctx);
 
        /* Note: idx is non-NULL when we are writing */
-       if ((flags & HASH_WRITE_OBJECT) != 0)
+       if ((flags & HASH_WRITE_OBJECT) != 0) {
                CALLOC_ARRAY(idx, 1);
 
+               prepare_to_stream(state, flags);
+               hashfile_checkpoint_init(state->f, &checkpoint);
+       }
+
        already_hashed_to = 0;
 
        while (1) {

diff --git a/csum-file.c b/csum-file.c
index ebffc80ef71db53f7fcced9b333fed3b241fbc21..232121f415fa50922da3f0cbe3ce4c51ce7f4976 100644 (file)
--- a/csum-file.c
+++ b/csum-file.c
@@ -206,6 +206,13 @@ struct hashfile *hashfd_throughput(int fd, const char *name, struct progress *tp
        return hashfd_internal(fd, name, tp, 8 * 1024);
 }
 
+void hashfile_checkpoint_init(struct hashfile *f,
+                             struct hashfile_checkpoint *checkpoint)
+{
+       memset(checkpoint, 0, sizeof(*checkpoint));
+       f->algop->init_fn(&checkpoint->ctx);
+}
+
 void hashfile_checkpoint(struct hashfile *f, struct hashfile_checkpoint *checkpoint)
 {
        hashflush(f);

diff --git a/csum-file.h b/csum-file.h
index 2b45f4673a21f07d8827675197b1c655e703f9c9..b7475f16c20e8a571f3b7e16831d1fdd4420d298 100644 (file)
--- a/csum-file.h
+++ b/csum-file.h
@@ -36,6 +36,7 @@ struct hashfile_checkpoint {
        git_hash_ctx ctx;
 };
 
+void hashfile_checkpoint_init(struct hashfile *, struct hashfile_checkpoint *);
 void hashfile_checkpoint(struct hashfile *, struct hashfile_checkpoint *);
 int hashfile_truncate(struct hashfile *, struct hashfile_checkpoint *);