git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v4 4/8] pack-objects: use finalize_object_file() to rename pack/idx/etc

From
Taylor Blau <me@ttaylorr.com>
Date
Sep 24, 2024, 17:32 UTC
Message-ID
<3cc7f7b1f67fe823834c36f3be20be8ee56e16a4.1727199118.git.me@ttaylorr.com>
In-Reply-To
<cover.1727199118.git.me@ttaylorr.com>

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>
---
 pack-write.c                          | 7 ++++---
 t/t5303-pack-corruption-resilience.sh | 7 ++++++-
 2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/pack-write.c b/pack-write.c
index 27965672f17..f415604c159 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -8,6 +8,7 @@
 #include "csum-file.h"
 #include "remote.h"
 #include "chunk-format.h"
+#include "object-file.h"
 #include "pack-mtimes.h"
 #include "pack-objects.h"
 #include "pack-revindex.h"
@@ -528,9 +529,9 @@ static void rename_tmp_packfile(struct strbuf *name_prefix, const char *source,
 	size_t name_prefix_len = name_prefix->len;
 
 	strbuf_addstr(name_prefix, ext);
-	if (rename(source, name_prefix->buf))
-		die_errno("unable to rename temporary file to '%s'",
-			  name_prefix->buf);
+	if (finalize_object_file(source, name_prefix->buf))
+		die("unable to rename temporary file to '%s'",
+		    name_prefix->buf);
 	strbuf_setlen(name_prefix, name_prefix_len);
 }
 
diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh
index 61469ef4a68..e6a43ec9ae3 100755
--- a/t/t5303-pack-corruption-resilience.sh
+++ b/t/t5303-pack-corruption-resilience.sh
@@ -44,9 +44,14 @@ create_new_pack() {
 }
 
 do_repack() {
+    for f in $pack.*
+    do
+	    mv $f "$(echo $f | sed -e 's/pack-/pack-corrupt-/')" || return 1
+    done &&
     pack=$(printf "$blob_1\n$blob_2\n$blob_3\n" |
           git pack-objects $@ .git/objects/pack/pack) &&
-    pack=".git/objects/pack/pack-${pack}"
+    pack=".git/objects/pack/pack-${pack}" &&
+    rm -f .git/objects/pack/pack-corrupt-*
 }
 
 do_corrupt_object() {
-- 
2.46.2.636.g4b83dd05e9f
Previous: Taylor BlauNext: Junio C Hamano
Message 77 of 99 in “hash.h: support choosing a separate SHA-1 for non-cryptographic uses”
  1. 0/4 hash.h: support choosing a separate SHA-1 for non-cryptographic usesTaylor Blau, Sep 1, 2024
  2. 1/4 sha1: do not redefine `platform_SHA_CTX` and friendsTaylor Blau, Sep 1, 2024
  3. Patrick SteinhardtSep 2, 2024
  4. Taylor BlauSep 3, 2024
  5. 2/4 hash.h: scaffolding for _fast hashing variantsTaylor Blau, Sep 1, 2024
  6. Patrick SteinhardtSep 2, 2024
  7. Junio C HamanoSep 3, 2024
  8. Taylor BlauSep 3, 2024
  9. Junio C HamanoSep 3, 2024
  10. Taylor BlauSep 3, 2024
  11. Patrick SteinhardtSep 4, 2024
  12. Junio C HamanoSep 4, 2024
  13. Taylor BlauSep 3, 2024
  14. 3/4 Makefile: allow specifying a SHA-1 for non-cryptographic usesTaylor Blau, Sep 1, 2024
  15. Patrick SteinhardtSep 2, 2024
  16. Taylor BlauSep 3, 2024
  17. 4/4 csum-file.c: use fast SHA-1 implementation when availableTaylor Blau, Sep 1, 2024
  18. Patrick SteinhardtSep 2, 2024
  19. brian m. carlsonSep 3, 2024
  20. Taylor BlauSep 3, 2024
  21. Junio C HamanoSep 2, 2024
  22. Taylor BlauSep 3, 2024
  23. Junio C HamanoSep 3, 2024
  24. brian m. carlsonSep 2, 2024
  25. Taylor BlauSep 3, 2024
  26. Junio C HamanoSep 3, 2024
  27. brian m. carlsonSep 4, 2024
  28. Jeff KingSep 5, 2024
  29. Junio C HamanoSep 5, 2024
  30. Taylor BlauSep 5, 2024
  31. Junio C HamanoSep 5, 2024
  32. Taylor BlauSep 5, 2024
  33. Taylor BlauSep 5, 2024
  34. Taylor BlauSep 5, 2024
  35. Jeff KingSep 5, 2024
  36. Junio C HamanoSep 5, 2024
  37. 0/4 hash.h: support choosing a separate SHA-1 for non-cryptographic usesTaylor Blau, Sep 5, 2024
  38. 1/4 sha1: do not redefine `platform_SHA_CTX` and friendsTaylor Blau, Sep 5, 2024
  39. 2/4 hash.h: scaffolding for _fast hashing variantsTaylor Blau, Sep 5, 2024
  40. 3/4 Makefile: allow specifying a SHA-1 for non-cryptographic usesTaylor Blau, Sep 5, 2024
  41. 4/4 csum-file.c: use fast SHA-1 implementation when availableTaylor Blau, Sep 5, 2024
  42. 0/9 hash.h: support choosing a separate SHA-1 for non-cryptographic usesTaylor Blau, Sep 6, 2024
  43. 1/9 finalize_object_file(): check for name collision before renamingTaylor Blau, Sep 6, 2024
  44. 2/9 finalize_object_file(): refactor unlink_or_warn() placementTaylor Blau, Sep 6, 2024
  45. 3/9 finalize_object_file(): implement collision checkTaylor Blau, Sep 6, 2024
  46. Junio C HamanoSep 6, 2024
  47. Chris TorekSep 6, 2024
  48. Jeff KingSep 10, 2024
  49. Junio C HamanoSep 10, 2024
  50. Patrick SteinhardtSep 16, 2024
  51. Taylor BlauSep 16, 2024
  52. Taylor BlauSep 16, 2024
  53. Junio C HamanoSep 17, 2024
  54. 4/9 pack-objects: use finalize_object_file() to rename pack/idx/etcTaylor Blau, Sep 6, 2024
  55. 5/9 i5500-git-daemon.sh: use compile-able version of Git without OpenSSLTaylor Blau, Sep 6, 2024
  56. Jeff KingSep 11, 2024
  57. Jeff KingSep 11, 2024
  58. Junio C HamanoSep 12, 2024
  59. Junio C HamanoSep 11, 2024
  60. Jeff KingSep 11, 2024
  61. 6/9 sha1: do not redefine `platform_SHA_CTX` and friendsTaylor Blau, Sep 6, 2024
  62. 7/9 hash.h: scaffolding for _fast hashing variantsTaylor Blau, Sep 6, 2024
  63. 8/9 Makefile: allow specifying a SHA-1 for non-cryptographic usesTaylor Blau, Sep 6, 2024
  64. 9/9 csum-file.c: use fast SHA-1 implementation when availableTaylor Blau, Sep 6, 2024
  65. Junio C HamanoSep 6, 2024
  66. 0/8 hash.h: support choosing a separate SHA-1 for non-cryptographic usesTaylor Blau, Sep 24, 2024
  67. 1/8 finalize_object_file(): check for name collision before renamingTaylor Blau, Sep 24, 2024
  68. Junio C HamanoSep 25, 2024
  69. 2/8 finalize_object_file(): refactor unlink_or_warn() placementTaylor Blau, Sep 24, 2024
  70. 3/8 finalize_object_file(): implement collision checkTaylor Blau, Sep 24, 2024
  71. Jeff KingSep 24, 2024
  72. Taylor BlauSep 24, 2024
  73. Jeff KingSep 24, 2024
  74. Taylor BlauSep 25, 2024
  75. Junio C HamanoSep 24, 2024
  76. Taylor BlauSep 24, 2024
  77. 4/8 pack-objects: use finalize_object_file() to rename pack/idx/etcTaylor Blau, Sep 24, 2024
  78. Junio C HamanoSep 24, 2024
  79. 5/8 sha1: do not redefine `platform_SHA_CTX` and friendsTaylor Blau, Sep 24, 2024
  80. 6/8 hash.h: scaffolding for _unsafe hashing variantsTaylor Blau, Sep 24, 2024
  81. 7/8 Makefile: allow specifying a SHA-1 for non-cryptographic usesTaylor Blau, Sep 24, 2024
  82. 8/8 csum-file.c: use unsafe SHA-1 implementation when availableTaylor Blau, Sep 24, 2024
  83. Jeff KingSep 24, 2024
  84. Elijah NewrenSep 25, 2024
  85. Junio C HamanoSep 25, 2024
  86. Taylor BlauSep 25, 2024
  87. Taylor BlauSep 25, 2024
  88. 0/8 hash.h: support choosing a separate SHA-1 for non-cryptographic usesTaylor Blau, Sep 26, 2024
  89. 1/8 finalize_object_file(): check for name collision before renamingTaylor Blau, Sep 26, 2024
  90. 2/8 finalize_object_file(): refactor unlink_or_warn() placementTaylor Blau, Sep 26, 2024
  91. 3/8 finalize_object_file(): implement collision checkTaylor Blau, Sep 26, 2024
  92. 4/8 pack-objects: use finalize_object_file() to rename pack/idx/etcTaylor Blau, Sep 26, 2024
  93. 5/8 sha1: do not redefine `platform_SHA_CTX` and friendsTaylor Blau, Sep 26, 2024
  94. 6/8 hash.h: scaffolding for _unsafe hashing variantsTaylor Blau, Sep 26, 2024
  95. 7/8 Makefile: allow specifying a SHA-1 for non-cryptographic usesTaylor Blau, Sep 26, 2024
  96. 8/8 csum-file.c: use unsafe SHA-1 implementation when availableTaylor Blau, Sep 26, 2024
  97. Elijah NewrenSep 26, 2024
  98. Junio C HamanoSep 27, 2024
  99. Jeff KingSep 27, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.