pack-objects: use finalize_object_file() to rename pack/idx/etc

In most places that write files to the object database (even packfiles via index-pack or fast-import), we use finalize_object_file(). This prefers link()/unlink() over rename(), because it means we will prefer data that is already in the repository to data that we are newly writing. We should do the same thing in pack-objects. Even though we don't think of it as accepting outside data (and thus not being susceptible to collision attacks), in theory a determined attacker could present just the right set of objects to cause an incremental repack to generate a pack with their desired hash. This has some test and real-world fallout, as seen in the adjustment to t5303 below. That test script assumes that we can "fix" corruption by repacking into a good state, including when the pack generated by that repack operation collides with a (corrupted) pack with the same hash. This violates our assumption from the previous adjustments to finalize_object_file() that if we're moving a new file over an existing one, that since their checksums match, so too must their contents. This makes "fixing" corruption like this a more explicit operation, since the test (and users, who may fix real-life corruption using a similar technique) must first move the broken contents out of the way. Note also that we now call adjust_shared_perm() twice. We already call adjust_shared_perm() in stage_tmp_packfiles(), and now call it again in finalize_object_file(). This is somewhat wasteful, but cleaning up the existing calls to adjust_shared_perm() is tricky (because sometimes we're writing to a tmpfile, and sometimes we're writing directly into the final destination), so let's tolerate some minor waste until we can more carefully clean up the now-redundant calls. Co-authored-by: Jeff King <peff@peff.net> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Taylor Blau committed Sep 26, 2024 at 11:22 UTC c177d3dc50d59042b1756e352e19f2dd8b01c25a
2 files changed +10 -4
pack-write.c
+4 -3
@@ -8,6 +8,7 @@
8 #include "csum-file.h"
9 #include "remote.h"
10 #include "chunk-format.h"
11 +#include "object-file.h"
12 #include "pack-mtimes.h"
13 #include "pack-objects.h"
14 #include "pack-revindex.h"
@@ -527,9 +528,9 @@ static void rename_tmp_packfile(struct strbuf *name_prefix, const char *source,
528 size_t name_prefix_len = name_prefix->len;
529
530 strbuf_addstr(name_prefix, ext);
530 - if (rename(source, name_prefix->buf))
531 - die_errno("unable to rename temporary file to '%s'",
532 - name_prefix->buf);
531 + if (finalize_object_file(source, name_prefix->buf))
532 + die("unable to rename temporary file to '%s'",
533 + name_prefix->buf);
534 strbuf_setlen(name_prefix, name_prefix_len);
535 }
536
t/t5303-pack-corruption-resilience.sh
+6 -1
@@ -44,9 +44,14 @@ create_new_pack() {
44 }
45
46 do_repack() {
47 + for f in $pack.*
48 + do
49 + mv $f "$(echo $f | sed -e 's/pack-/pack-corrupt-/')" || return 1
50 + done &&
51 pack=$(printf "$blob_1\n$blob_2\n$blob_3\n" |
52 git pack-objects $@ .git/objects/pack/pack) &&
49 - pack=".git/objects/pack/pack-${pack}"
53 + pack=".git/objects/pack/pack-${pack}" &&
54 + rm -f .git/objects/pack/pack-corrupt-*
55 }
56
57 do_corrupt_object() {