{"thread":{"id":"62728","subject":"[PATCH 0/2] object-file: retry linking file into place when occluding file vanishes","startedAt":"2025-01-03T08:19:59Z","lastAt":"2025-01-07T01:25:55Z","messageCount":16,"participants":["Patrick Steinhardt","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"509830","messageId":"20250103-b4-pks-object-file-racy-collision-check-v1-0-6ef9e2da1f87@pks.im","threadId":"62728","inReplyTo":null,"subject":"[PATCH 0/2] object-file: retry linking file into place when occluding file vanishes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-03T08:19:53Z","receivedAt":"2025-01-03T08:19:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis small patch series adapts the race fix for collision checks when\nmoving object files into place [1] to retry linking the object into\nplace instead of silently ignoring the error, as suggested by Peff in\n[2].\n\nThe series at [1] has already been merged into 'next', so this is built\non top of 1b4e9a5f8b (Merge branch 'ps/build-meson-html', 2025-01-02)\nwith ps/object-collision-check at 0ad3d65652 (object-file: fix race in\nobject collision check, 2024-12-30) merged into it.\n\nThanks!\n\nPatrick\n\n[1]: <20241230-b4-pks-object-file-racy-collision-check-v1-1-11571294e60a@pks.im>\n[2]: <20241231014220.GA225521@coredump.intra.peff.net>\n\n---\nPatrick Steinhardt (2):\n      object-file: rename variables in `check_collision()`\n      object-file: retry linking file into place when occluding file vanishes\n\n object-file.c | 61 +++++++++++++++++++++++++++++++++++------------------------\n 1 file changed, 36 insertions(+), 25 deletions(-)\n\n\n---\nbase-commit: 2be278337fd02495a86577a89fbf9387b2df6523\nchange-id: 20250103-b4-pks-object-file-racy-collision-check-a649bbf96a92\n\n"},{"id":"509831","messageId":"20250103-b4-pks-object-file-racy-collision-check-v1-1-6ef9e2da1f87@pks.im","threadId":"62728","inReplyTo":"20250103-b4-pks-object-file-racy-collision-check-v1-0-6ef9e2da1f87@pks.im","subject":"[PATCH 1/2] object-file: rename variables in `check_collision()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-03T08:19:54Z","receivedAt":"2025-01-03T08:20:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Rename variables used in `check_collision()` to clearly identify which\nfile is the source and which is the destination. This will make the next\nstep easier to reason about when we start to treat those files different\nfrom one another.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n object-file.c | 40 ++++++++++++++++++++--------------------\n 1 file changed, 20 insertions(+), 20 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex f84dcd2f2a7b88716ab47bc00ee7a605a82e8d21..e1989236ca87e565dea4d003f57882f257889ecf 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1970,56 +1970,56 @@ static void write_object_file_prepare_literally(const struct git_hash_algo *algo\n \thash_object_body(algo, &c, buf, len, oid, hdr, hdrlen);\n }\n \n-static int check_collision(const char *filename_a, const char *filename_b)\n+static int check_collision(const char *source, const char *dest)\n {\n-\tchar buf_a[4096], buf_b[4096];\n-\tint fd_a = -1, fd_b = -1;\n+\tchar buf_source[4096], buf_dest[4096];\n+\tint fd_source = -1, fd_dest = -1;\n \tint ret = 0;\n \n-\tfd_a = open(filename_a, O_RDONLY);\n-\tif (fd_a < 0) {\n+\tfd_source = open(source, O_RDONLY);\n+\tif (fd_source < 0) {\n \t\tif (errno != ENOENT)\n-\t\t\tret = error_errno(_(\"unable to open %s\"), filename_a);\n+\t\t\tret = error_errno(_(\"unable to open %s\"), source);\n \t\tgoto out;\n \t}\n \n-\tfd_b = open(filename_b, O_RDONLY);\n-\tif (fd_b < 0) {\n+\tfd_dest = open(dest, O_RDONLY);\n+\tif (fd_dest < 0) {\n \t\tif (errno != ENOENT)\n-\t\t\tret = error_errno(_(\"unable to open %s\"), filename_b);\n+\t\t\tret = error_errno(_(\"unable to open %s\"), dest);\n \t\tgoto out;\n \t}\n \n \twhile (1) {\n \t\tssize_t sz_a, sz_b;\n \n-\t\tsz_a = read_in_full(fd_a, buf_a, sizeof(buf_a));\n+\t\tsz_a = read_in_full(fd_source, buf_source, sizeof(buf_source));\n \t\tif (sz_a < 0) {\n-\t\t\tret = error_errno(_(\"unable to read %s\"), filename_a);\n+\t\t\tret = error_errno(_(\"unable to read %s\"), source);\n \t\t\tgoto out;\n \t\t}\n \n-\t\tsz_b = read_in_full(fd_b, buf_b, sizeof(buf_b));\n+\t\tsz_b = read_in_full(fd_dest, buf_dest, sizeof(buf_dest));\n \t\tif (sz_b < 0) {\n-\t\t\tret = error_errno(_(\"unable to read %s\"), filename_b);\n+\t\t\tret = error_errno(_(\"unable to read %s\"), dest);\n \t\t\tgoto out;\n \t\t}\n \n-\t\tif (sz_a != sz_b || memcmp(buf_a, buf_b, sz_a)) {\n+\t\tif (sz_a != sz_b || memcmp(buf_source, buf_dest, sz_a)) {\n \t\t\tret = error(_(\"files '%s' and '%s' differ in contents\"),\n-\t\t\t\t    filename_a, filename_b);\n+\t\t\t\t    source, dest);\n \t\t\tgoto out;\n \t\t}\n \n-\t\tif (sz_a < sizeof(buf_a))\n+\t\tif (sz_a < sizeof(buf_source))\n \t\t\tbreak;\n \t}\n \n out:\n-\tif (fd_a > -1)\n-\t\tclose(fd_a);\n-\tif (fd_b > -1)\n-\t\tclose(fd_b);\n+\tif (fd_source > -1)\n+\t\tclose(fd_source);\n+\tif (fd_dest > -1)\n+\t\tclose(fd_dest);\n \treturn ret;\n }\n \n\n-- \n2.48.0.rc1.241.g6c04ab211c.dirty\n\n"},{"id":"509832","messageId":"20250103-b4-pks-object-file-racy-collision-check-v1-2-6ef9e2da1f87@pks.im","threadId":"62728","inReplyTo":"20250103-b4-pks-object-file-racy-collision-check-v1-0-6ef9e2da1f87@pks.im","subject":"[PATCH 2/2] object-file: retry linking file into place when occluding file vanishes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-03T08:19:55Z","receivedAt":"2025-01-03T08:20:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In a preceding commit, we have adapted `check_collision()` to ignore\nthe case where either of the colliding files vanishes. This should be\nsafe in general when we assume that the contents of these two files were\nthe same. But the check is all about detecting collisions, so that\nassumption may be too optimistic.\n\nAdapt the code to retry linking the object into place when we see that\nthe destination file has racily vanished. This should generally succeed\nas we have just observed that the destination file does not exist\nanymore, except in the very unlikely event that it gets recreated by\nanother concurrent process again.\n\nFurthermore, stop treating `ENOENT` specially for the source file. It\nshouldn't happen that the source vanishes as we're using a fresh\ntemporary file for it, so if it does vanish it indicates an actual\nerror.\n\nSuggested-by: Jeff King <peff@peff.net>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n object-file.c | 23 +++++++++++++++++------\n 1 file changed, 17 insertions(+), 6 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex e1989236ca87e565dea4d003f57882f257889ecf..88432cc9c07c3c56ce31a298a0ee90e5b5acbaff 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1970,6 +1970,8 @@ static void write_object_file_prepare_literally(const struct git_hash_algo *algo\n \thash_object_body(algo, &c, buf, len, oid, hdr, hdrlen);\n }\n \n+#define CHECK_COLLISION_DEST_VANISHED -2\n+\n static int check_collision(const char *source, const char *dest)\n {\n \tchar buf_source[4096], buf_dest[4096];\n@@ -1978,8 +1980,7 @@ static int check_collision(const char *source, const char *dest)\n \n \tfd_source = open(source, O_RDONLY);\n \tif (fd_source < 0) {\n-\t\tif (errno != ENOENT)\n-\t\t\tret = error_errno(_(\"unable to open %s\"), source);\n+\t\tret = error_errno(_(\"unable to open %s\"), source);\n \t\tgoto out;\n \t}\n \n@@ -1987,6 +1988,8 @@ static int check_collision(const char *source, const char *dest)\n \tif (fd_dest < 0) {\n \t\tif (errno != ENOENT)\n \t\t\tret = error_errno(_(\"unable to open %s\"), dest);\n+\t\telse\n+\t\t\tret = CHECK_COLLISION_DEST_VANISHED;\n \t\tgoto out;\n \t}\n \n@@ -2034,8 +2037,10 @@ int finalize_object_file(const char *tmpfile, const char *filename)\n int finalize_object_file_flags(const char *tmpfile, const char *filename,\n \t\t\t       enum finalize_object_file_flags flags)\n {\n-\tstruct stat st;\n-\tint ret = 0;\n+\tint ret;\n+\n+retry:\n+\tret = 0;\n \n \tif (object_creation_mode == OBJECT_CREATION_USES_RENAMES)\n \t\tgoto try_rename;\n@@ -2056,6 +2061,8 @@ int finalize_object_file_flags(const char *tmpfile, const char *filename,\n \t * left to unlink.\n \t */\n \tif (ret && ret != EEXIST) {\n+\t\tstruct stat st;\n+\n \ttry_rename:\n \t\tif (!stat(filename, &st))\n \t\t\tret = EEXIST;\n@@ -2071,9 +2078,13 @@ int finalize_object_file_flags(const char *tmpfile, const char *filename,\n \t\t\terrno = saved_errno;\n \t\t\treturn error_errno(_(\"unable to write file %s\"), filename);\n \t\t}\n-\t\tif (!(flags & FOF_SKIP_COLLISION_CHECK) &&\n-\t\t    check_collision(tmpfile, filename))\n+\t\tif (!(flags & FOF_SKIP_COLLISION_CHECK)) {\n+\t\t\tret = check_collision(tmpfile, filename);\n+\t\t\tif (ret == CHECK_COLLISION_DEST_VANISHED)\n+\t\t\t\tgoto retry;\n+\t\t\telse if (ret)\n \t\t\t\treturn -1;\n+\t\t}\n \t\tunlink_or_warn(tmpfile);\n \t}\n \n\n-- \n2.48.0.rc1.241.g6c04ab211c.dirty\n\n"},{"id":"509863","messageId":"xmqqo70nswfv.fsf@gitster.g","threadId":"62728","inReplyTo":"20250103-b4-pks-object-file-racy-collision-check-v1-2-6ef9e2da1f87@pks.im","subject":"Re: [PATCH 2/2] object-file: retry linking file into place when occluding file vanishes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-03T16:18:12Z","receivedAt":"2025-01-03T16:18:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> In a preceding commit, we have adapted `check_collision()` to ignore\n> the case where either of the colliding files vanishes. This should be\n> safe in general when we assume that the contents of these two files were\n> the same. But the check is all about detecting collisions, so that\n> assumption may be too optimistic.\n>\n> Adapt the code to retry linking the object into place when we see that\n> the destination file has racily vanished. This should generally succeed\n> as we have just observed that the destination file does not exist\n> anymore, except in the very unlikely event that it gets recreated by\n> another concurrent process again.\n\nOK.  \n\nThe way the new code is structured, we are set to infinitely retry\nas long as our link() fails with EEXIST yet check_collision() finds\nthat the destination is missing.  It makes me somewhat uneasy, but\nsuch a condition needs an actively racing attacker that can perfectly\ntime the creation and the removal of the destination, so it would\nprobably be OK if this code lacks \"we retry only once and fail\"\nsafety valve.\n\n> Furthermore, stop treating `ENOENT` specially for the source file. It\n> shouldn't happen that the source vanishes as we're using a fresh\n> temporary file for it, so if it does vanish it indicates an actual\n> error.\n\nMakes sense.\n\n> Suggested-by: Jeff King <peff@peff.net>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  object-file.c | 23 +++++++++++++++++------\n>  1 file changed, 17 insertions(+), 6 deletions(-)\n\nWill queue.\n"},{"id":"509882","messageId":"20250103191001.GD3208749@coredump.intra.peff.net","threadId":"62728","inReplyTo":"20250103-b4-pks-object-file-racy-collision-check-v1-1-6ef9e2da1f87@pks.im","subject":"Re: [PATCH 1/2] object-file: rename variables in `check_collision()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-03T19:10:01Z","receivedAt":"2025-01-03T19:10:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 03, 2025 at 09:19:54AM +0100, Patrick Steinhardt wrote:\n\n> Rename variables used in `check_collision()` to clearly identify which\n> file is the source and which is the destination. This will make the next\n> step easier to reason about when we start to treat those files different\n> from one another.\n\nSeems obviously good, and definitely worth doing as a separate step from\nthe actual code change.\n\n-Peff\n"},{"id":"509886","messageId":"20250103194058.GE3208749@coredump.intra.peff.net","threadId":"62728","inReplyTo":"20250103-b4-pks-object-file-racy-collision-check-v1-2-6ef9e2da1f87@pks.im","subject":"Re: [PATCH 2/2] object-file: retry linking file into place when occluding file vanishes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-03T19:40:58Z","receivedAt":"2025-01-03T19:40:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 03, 2025 at 09:19:55AM +0100, Patrick Steinhardt wrote:\n\n> In a preceding commit, we have adapted `check_collision()` to ignore\n\nIf it's in next and we are building on top, I think we can mention it by\nname: 0ad3d65652 (object-file: fix race in object collision check,\n2024-12-30).\n\n> the case where either of the colliding files vanishes. This should be\n> safe in general when we assume that the contents of these two files were\n> the same. But the check is all about detecting collisions, so that\n> assumption may be too optimistic.\n\nI found this a little vague about what \"too optimistic\" means. ;)\nMaybe something like:\n\n  Prior to 0ad3d65652, callers could expect that a successful return\n  from finalize_object_file() means that either the file was moved into\n  place, or the identical bytes were already present. If neither of\n  those happens, we'd return an error.\n\n  Since that commit, if the destination file disappears between our\n  link() call and the collision check, we'd return success without\n  actually checking the contents, and without retrying the link. This\n  solves the common case that the files were indeed the same, but it\n  means that we may corrupt the repository if they weren't (this implies\n  a hash collision, but the whole point of this function is protecting\n  against hash collisions).\n\n  We can't be pessimistic and assume they're different; that hurts the\n  common case that 0ad3d65652 was trying to fix. But after seeing that\n  the destination file went away, we can retry linking again...\n\n> Furthermore, stop treating `ENOENT` specially for the source file. It\n> shouldn't happen that the source vanishes as we're using a fresh\n> temporary file for it, so if it does vanish it indicates an actual\n> error.\n\nOK. I think this is worth doing, but I'd probably have put it into its\nown commit.\n\n> @@ -1987,6 +1988,8 @@ static int check_collision(const char *source, const char *dest)\n>  \tif (fd_dest < 0) {\n>  \t\tif (errno != ENOENT)\n>  \t\t\tret = error_errno(_(\"unable to open %s\"), dest);\n> +\t\telse\n> +\t\t\tret = CHECK_COLLISION_DEST_VANISHED;\n>  \t\tgoto out;\n>  \t}\n\nOK. We're depending on error() never using \"-2\" itself, but I think that\nis a reasonable thing.\n\n> @@ -2034,8 +2037,10 @@ int finalize_object_file(const char *tmpfile, const char *filename)\n>  int finalize_object_file_flags(const char *tmpfile, const char *filename,\n>  \t\t\t       enum finalize_object_file_flags flags)\n>  {\n> -\tstruct stat st;\n> -\tint ret = 0;\n> +\tint ret;\n> +\n> +retry:\n> +\tret = 0;\n>  \n>  \tif (object_creation_mode == OBJECT_CREATION_USES_RENAMES)\n>  \t\tgoto try_rename;\n> @@ -2056,6 +2061,8 @@ int finalize_object_file_flags(const char *tmpfile, const char *filename,\n>  \t * left to unlink.\n>  \t */\n>  \tif (ret && ret != EEXIST) {\n> +\t\tstruct stat st;\n> +\n\nOK, we move the stat struct here where it's needed. I think that's\nsomewhat orthogonal to your patch, but reduced scoping does help make\nthe goto's less confusing.\n\nI suspect there's a way to write this as a loop that would be more\nstructured, but it would be a bigger refactor. Bonus points if it also\nget rid of the try_rename goto, too. ;)\n\nI'm OK punting on that, though.\n\n> @@ -2071,9 +2078,13 @@ int finalize_object_file_flags(const char *tmpfile, const char *filename,\n>  \t\t\terrno = saved_errno;\n>  \t\t\treturn error_errno(_(\"unable to write file %s\"), filename);\n>  \t\t}\n> -\t\tif (!(flags & FOF_SKIP_COLLISION_CHECK) &&\n> -\t\t    check_collision(tmpfile, filename))\n> +\t\tif (!(flags & FOF_SKIP_COLLISION_CHECK)) {\n> +\t\t\tret = check_collision(tmpfile, filename);\n> +\t\t\tif (ret == CHECK_COLLISION_DEST_VANISHED)\n> +\t\t\t\tgoto retry;\n> +\t\t\telse if (ret)\n>  \t\t\t\treturn -1;\n> +\t\t}\n>  \t\tunlink_or_warn(tmpfile);\n>  \t}\n\nI share Junio's uneasiness with looping forever based on external input\nfrom the filesystem (even though you _should_ eventually win the race,\nthat's not guaranteed, and of course a weird filesystem might confuse\nus). Could we put a stop-gap in it like:\n\ndiff --git a/object-file.c b/object-file.c\nindex 88432cc9c0..262a2f3df2 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -2038,6 +2038,7 @@ int finalize_object_file_flags(const char *tmpfile, const char *filename,\n \t\t\t       enum finalize_object_file_flags flags)\n {\n \tint ret;\n+\tint retries = 0;\n \n retry:\n \tret = 0;\n@@ -2080,8 +2081,11 @@ int finalize_object_file_flags(const char *tmpfile, const char *filename,\n \t\t}\n \t\tif (!(flags & FOF_SKIP_COLLISION_CHECK)) {\n \t\t\tret = check_collision(tmpfile, filename);\n-\t\t\tif (ret == CHECK_COLLISION_DEST_VANISHED)\n+\t\t\tif (ret == CHECK_COLLISION_DEST_VANISHED) {\n+\t\t\t\tif (retries++ > 5)\n+\t\t\t\t\treturn error(_(\"unable to write repeatedly vanishing file %s\"), filename);\n \t\t\t\tgoto retry;\n+\t\t\t}\n \t\t\telse if (ret)\n \t\t\t\treturn -1;\n \t\t}\n\nOtherwise, I think the logic looks good.\n\n-Peff\n"},{"id":"509887","messageId":"20250103195942.GA3212696@coredump.intra.peff.net","threadId":"62728","inReplyTo":"20250103194058.GE3208749@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] object-file: retry linking file into place when occluding file vanishes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-03T19:59:42Z","receivedAt":"2025-01-03T19:59:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 03, 2025 at 02:40:58PM -0500, Jeff King wrote:\n\n> I suspect there's a way to write this as a loop that would be more\n> structured, but it would be a bigger refactor. Bonus points if it also\n> get rid of the try_rename goto, too. ;)\n> \n> I'm OK punting on that, though.\n\nFor fun, here's a version without any goto's in it, that should behave\nthe same. But it would be very easy to miss a case. So I don't know if\nit is worth the regression risk, and I don't blame you if you delete\nthis message without looking carefully. ;)\n\nDiff is kind of hard to read, so you may want to apply (on top of your\npatches) and just look at the post-image.\n\ndiff --git a/object-file.c b/object-file.c\nindex 88432cc9c0..923d75a889 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -2037,58 +2037,66 @@ int finalize_object_file(const char *tmpfile, const char *filename)\n int finalize_object_file_flags(const char *tmpfile, const char *filename,\n \t\t\t       enum finalize_object_file_flags flags)\n {\n-\tint ret;\n+\tint tries = 5;\n \n-retry:\n-\tret = 0;\n+\twhile (tries-- > 0) {\n+\t\tint ret = 0;\n+\t\tif (object_creation_mode != OBJECT_CREATION_USES_RENAMES) {\n+\t\t\tif (!link(tmpfile, filename)) {\n+\t\t\t\tunlink_or_warn(tmpfile);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tret = errno;\n+\t\t}\n \n-\tif (object_creation_mode == OBJECT_CREATION_USES_RENAMES)\n-\t\tgoto try_rename;\n-\telse if (link(tmpfile, filename))\n-\t\tret = errno;\n-\telse\n-\t\tunlink_or_warn(tmpfile);\n+\t\t/*\n+\t\t * Coda hack - coda doesn't like cross-directory links,\n+\t\t * so we fall back to a rename, which will mean that it\n+\t\t * won't be able to check collisions, but that's not a\n+\t\t * big deal.\n+\t\t *\n+\t\t * The same holds for FAT formatted media.\n+\t\t *\n+\t\t * When this succeeds, we just return.  We have nothing\n+\t\t * left to unlink.\n+\t\t */\n+\t\tif (!ret || ret == EEXIST) {\n+\t\t\tstruct stat st;\n \n-\t/*\n-\t * Coda hack - coda doesn't like cross-directory links,\n-\t * so we fall back to a rename, which will mean that it\n-\t * won't be able to check collisions, but that's not a\n-\t * big deal.\n-\t *\n-\t * The same holds for FAT formatted media.\n-\t *\n-\t * When this succeeds, we just return.  We have nothing\n-\t * left to unlink.\n-\t */\n-\tif (ret && ret != EEXIST) {\n-\t\tstruct stat st;\n+\t\t\tif (!stat(filename, &st))\n+\t\t\t\tret = EEXIST;\n+\t\t\telse if (!rename(tmpfile, filename))\n+\t\t\t\tbreak;\n+\t\t\telse\n+\t\t\t\tret = errno;\n+\t\t}\n \n-\ttry_rename:\n-\t\tif (!stat(filename, &st))\n-\t\t\tret = EEXIST;\n-\t\telse if (!rename(tmpfile, filename))\n-\t\t\tgoto out;\n-\t\telse\n-\t\t\tret = errno;\n-\t}\n-\tif (ret) {\n+\t\t/* Do not retry most filesystem errors */\n \t\tif (ret != EEXIST) {\n \t\t\tint saved_errno = errno;\n \t\t\tunlink_or_warn(tmpfile);\n \t\t\terrno = saved_errno;\n \t\t\treturn error_errno(_(\"unable to write file %s\"), filename);\n \t\t}\n-\t\tif (!(flags & FOF_SKIP_COLLISION_CHECK)) {\n-\t\t\tret = check_collision(tmpfile, filename);\n-\t\t\tif (ret == CHECK_COLLISION_DEST_VANISHED)\n-\t\t\t\tgoto retry;\n-\t\t\telse if (ret)\n-\t\t\t\treturn -1;\n+\n+\t\tret = (flags & FOF_SKIP_COLLISION_CHECK) ? 0 :\n+\t\t\tcheck_collision(tmpfile, filename);\n+\n+\t\tif (!ret) {\n+\t\t\t/* Same contents (or we are allowed to assume such). */\n+\t\t\tunlink_or_warn(tmpfile);\n+\t\t\tbreak;\n \t\t}\n-\t\tunlink_or_warn(tmpfile);\n+\n+\t\tif (ret != CHECK_COLLISION_DEST_VANISHED)\n+\t\t\treturn -1; /* check_collision() already complained */\n+\n+\t\t/* loop again to retry vanished destination */\n \t}\n \n-out:\n+\tif (tries < 0)\n+\t\treturn error(_(\"unable to write repeatedly vanishing file %s\"), filename);\n+\n \tif (adjust_shared_perm(filename))\n \t\treturn error(_(\"unable to set permission to '%s'\"), filename);\n \treturn 0;\n\n-Peff\n"},{"id":"509891","messageId":"xmqqsepzprul.fsf@gitster.g","threadId":"62728","inReplyTo":"20250103194058.GE3208749@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] object-file: retry linking file into place when occluding file vanishes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-03T20:25:38Z","receivedAt":"2025-01-03T20:25:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I share Junio's uneasiness with looping forever based on external input\n> from the filesystem (even though you _should_ eventually win the race,\n> that's not guaranteed, and of course a weird filesystem might confuse\n> us).\n\nYeah, \"a weird filesystem\" would be a lot more plausible than a\ndetermined and accurate attacker to break it.  The only thing they\nhave to do is to yield EEXIST when failing link() for some other\nreason.\n\n> Could we put a stop-gap in it like:\n>\n> diff --git a/object-file.c b/object-file.c\n> index 88432cc9c0..262a2f3df2 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -2038,6 +2038,7 @@ int finalize_object_file_flags(const char *tmpfile, const char *filename,\n>  \t\t\t       enum finalize_object_file_flags flags)\n>  {\n>  \tint ret;\n> +\tint retries = 0;\n>  \n>  retry:\n>  \tret = 0;\n> @@ -2080,8 +2081,11 @@ int finalize_object_file_flags(const char *tmpfile, const char *filename,\n>  \t\t}\n>  \t\tif (!(flags & FOF_SKIP_COLLISION_CHECK)) {\n>  \t\t\tret = check_collision(tmpfile, filename);\n> -\t\t\tif (ret == CHECK_COLLISION_DEST_VANISHED)\n> +\t\t\tif (ret == CHECK_COLLISION_DEST_VANISHED) {\n> +\t\t\t\tif (retries++ > 5)\n> +\t\t\t\t\treturn error(_(\"unable to write repeatedly vanishing file %s\"), filename);\n>  \t\t\t\tgoto retry;\n> +\t\t\t}\n>  \t\t\telse if (ret)\n>  \t\t\t\treturn -1;\n>  \t\t}\n\nSounds sensible.\n\n> Otherwise, I think the logic looks good.\n>\n> -Peff\n\nThanks.\n"},{"id":"509895","messageId":"xmqqed1jo7k6.fsf@gitster.g","threadId":"62728","inReplyTo":"20250103195942.GA3212696@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] object-file: retry linking file into place when occluding file vanishes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-03T22:29:13Z","receivedAt":"2025-01-03T22:29:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Diff is kind of hard to read, so you may want to apply (on top of your\n> patches) and just look at the post-image.\n>\n> diff --git a/object-file.c b/object-file.c\n> index 88432cc9c0..923d75a889 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -2037,58 +2037,66 @@ int finalize_object_file(const char *tmpfile, const char *filename)\n>  int finalize_object_file_flags(const char *tmpfile, const char *filename,\n>  \t\t\t       enum finalize_object_file_flags flags)\n>  {\n> +\tint tries = 5;\n>  \n> +\twhile (tries-- > 0) {\n> +\t\tint ret = 0;\n> +\t\tif (object_creation_mode != OBJECT_CREATION_USES_RENAMES) {\n\nPlatforms that do not want hardlinks set CREATION_USES_RENAMES flag.\nWe skip this block on them.\n\n> +\t\t\tif (!link(tmpfile, filename)) {\n> +\t\t\t\tunlink_or_warn(tmpfile);\n> +\t\t\t\tbreak;\n\nIf we successfully hardlink, we remove the temporary and happily\nleave the retry loop.\n\n> +\t\t\t}\n> +\t\t\tret = errno;\n> +\t\t}\n>  \n> +\t\t/*\n> +\t\t * Coda hack - coda doesn't like cross-directory links,\n> +\t\t * so we fall back to a rename, which will mean that it\n> +\t\t * won't be able to check collisions, but that's not a\n> +\t\t * big deal.\n> +\t\t *\n> +\t\t * The same holds for FAT formatted media.\n> +\t\t *\n> +\t\t * When this succeeds, we just return.  We have nothing\n> +\t\t * left to unlink.\n> +\t\t */\n> +\t\tif (!ret || ret == EEXIST) {\n> +\t\t\tstruct stat st;\n\nEither we skipped the hardlink step (then ret==0), or tried to\nhardlink and saw EEXIST, we come here and try renaming.\n\n> +\t\t\tif (!stat(filename, &st))\n> +\t\t\t\tret = EEXIST;\n\nWe check if the destination already exists, and do the same thing as\nthe case where hardlink failed due to EEXIST, if that is the case.\n\nOtherwise, any failure of stat() we assume we are free to rename the\nnew thing there.  It is a bit strange that we do not check ENOENT\nhere.\n\nThe reason why this stat() fails is not all that interesting,\nbecause it is subject to a TOCTOU race, and the case we are more\ninterested in is when this stat() succeeds, which positively tells\nus that there is something at that path (hence we do not have to\ntrigger a failure from rename() to notice a potential collision).\n\nWait, what if stat() succeeds and !S_ISREG(st.st_mode)?  But that's\nthe original code for \"Coda hack\", and that is not something we are\ntrying to \"fix\" at this point (yet).\n\n> +\t\t\telse if (!rename(tmpfile, filename))\n> +\t\t\t\tbreak;\n\nIf we manage to rename(), we happily leave the retry loop.  Unlike\nthe hardlink case, there is no tmpfile to unlink.  Good.\n\n> +\t\t\telse\n> +\t\t\t\tret = errno;\n\nHere errno is guaranteed from the failure of rename().  If the\ndestination was created immediately after we got ENOENT from stat(),\nit is likely rename() gave us EEXIST, which we would check for\ncollission and retry.\n\n> +\t\t}\n>  \n> +\t\t/* Do not retry most filesystem errors */\n>  \t\tif (ret != EEXIST) {\n>  \t\t\tint saved_errno = errno;\n>  \t\t\tunlink_or_warn(tmpfile);\n>  \t\t\terrno = saved_errno;\n>  \t\t\treturn error_errno(_(\"unable to write file %s\"), filename);\n\nSensible.\n\n>  \t\t}\n> +\n> +\t\tret = (flags & FOF_SKIP_COLLISION_CHECK) ? 0 :\n> +\t\t\tcheck_collision(tmpfile, filename);\n> +\n> +\t\tif (!ret) {\n> +\t\t\t/* Same contents (or we are allowed to assume such). */\n> +\t\t\tunlink_or_warn(tmpfile);\n> +\t\t\tbreak;\n>  \t\t}\n> +\n> +\t\tif (ret != CHECK_COLLISION_DEST_VANISHED)\n> +\t\t\treturn -1; /* check_collision() already complained */\n> +\n> +\t\t/* loop again to retry vanished destination */\n\nOK.\n\n>  \t}\n>  \n> +\tif (tries < 0)\n> +\t\treturn error(_(\"unable to write repeatedly vanishing file %s\"), filename);\n> +\n\nOK.\n\n>  \tif (adjust_shared_perm(filename))\n>  \t\treturn error(_(\"unable to set permission to '%s'\"), filename);\n>  \treturn 0;\n"},{"id":"509962","messageId":"20250106-b4-pks-object-file-racy-collision-check-v2-0-8b3984ecbb18@pks.im","threadId":"62728","inReplyTo":"20250103-b4-pks-object-file-racy-collision-check-v1-0-6ef9e2da1f87@pks.im","subject":"[PATCH v2 0/3] object-file: retry linking file into place when occluding file vanishes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-06T09:24:24Z","receivedAt":"2025-01-06T09:24:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis small patch series adapts the race fix for collision checks when\nmoving object files into place [1] to retry linking the object into\nplace instead of silently ignoring the error, as suggested by Peff in\n[2].\n\nThe series at [1] has already been merged into 'next', so this is built\non top of 1b4e9a5f8b (Merge branch 'ps/build-meson-html', 2025-01-02)\nwith ps/object-collision-check at 0ad3d65652 (object-file: fix race in\nobject collision check, 2024-12-30) merged into it.\n\nChanges in v2:\n\n  - Add retry counter so that we eventually abort in case the colliding\n    files repeatedly reappears and vanishes again.\n  - Evict the change to stop handling ENOENT for the source file specially\n    into a separate commit.\n  - Commit message improvements.\n  - Link to v1: https://lore.kernel.org/r/20250103-b4-pks-object-file-racy-collision-check-v1-0-6ef9e2da1f87@pks.im\n\nThanks!\n\nPatrick\n\n[1]: <20241230-b4-pks-object-file-racy-collision-check-v1-1-11571294e60a@pks.im>\n[2]: <20241231014220.GA225521@coredump.intra.peff.net>\n\n---\nPatrick Steinhardt (3):\n      object-file: rename variables in `check_collision()`\n      object-file: don't special-case missing source file in collision check\n      object-file: retry linking file into place when occluding file vanishes\n\n object-file.c | 66 +++++++++++++++++++++++++++++++++++++----------------------\n 1 file changed, 41 insertions(+), 25 deletions(-)\n\nRange-diff versus v1:\n\n1:  389b9e4180 = 1:  0c38089e9f object-file: rename variables in `check_collision()`\n-:  ---------- > 2:  d4a7ec11c8 object-file: don't special-case missing source file in collision check\n2:  91a8bdf37d ! 3:  587fd01fb6 object-file: retry linking file into place when occluding file vanishes\n    @@ Metadata\n      ## Commit message ##\n         object-file: retry linking file into place when occluding file vanishes\n     \n    -    In a preceding commit, we have adapted `check_collision()` to ignore\n    -    the case where either of the colliding files vanishes. This should be\n    -    safe in general when we assume that the contents of these two files were\n    -    the same. But the check is all about detecting collisions, so that\n    -    assumption may be too optimistic.\n    +    Prior to 0ad3d65652 (object-file: fix race in object collision check,\n    +    2024-12-30), callers could expect that a successful return from\n    +    `finalize_object_file()` means that either the file was moved into\n    +    place, or the identical bytes were already present. If neither of those\n    +    happens, we'd return an error.\n     \n    -    Adapt the code to retry linking the object into place when we see that\n    -    the destination file has racily vanished. This should generally succeed\n    -    as we have just observed that the destination file does not exist\n    -    anymore, except in the very unlikely event that it gets recreated by\n    -    another concurrent process again.\n    +    Since that commit, if the destination file disappears between our\n    +    link(3p) call and the collision check, we'd return success without\n    +    actually checking the contents, and without retrying the link. This\n    +    solves the common case that the files were indeed the same, but it means\n    +    that we may corrupt the repository if they weren't (this implies a hash\n    +    collision, but the whole point of this function is protecting against\n    +    hash collisions).\n     \n    -    Furthermore, stop treating `ENOENT` specially for the source file. It\n    -    shouldn't happen that the source vanishes as we're using a fresh\n    -    temporary file for it, so if it does vanish it indicates an actual\n    -    error.\n    +    We can't be pessimistic and assume they're different; that hurts the\n    +    common case that the mentioned commit was trying to fix. But after\n    +    seeing that the destination file went away, we can retry linking again.\n    +    Adapt the code to do so when we see that the destination file has racily\n    +    vanished. This should generally succeed as we have just observed that\n    +    the destination file does not exist anymore, except in the very unlikely\n    +    event that it gets recreated by another concurrent process again.\n     \n    -    Suggested-by: Jeff King <peff@peff.net>\n    +    Helped-by: Jeff King <peff@peff.net>\n         Signed-off-by: Patrick Steinhardt <ps@pks.im>\n     \n      ## object-file.c ##\n    @@ object-file.c: static void write_object_file_prepare_literally(const struct git_\n      static int check_collision(const char *source, const char *dest)\n      {\n      \tchar buf_source[4096], buf_dest[4096];\n    -@@ object-file.c: static int check_collision(const char *source, const char *dest)\n    - \n    - \tfd_source = open(source, O_RDONLY);\n    - \tif (fd_source < 0) {\n    --\t\tif (errno != ENOENT)\n    --\t\t\tret = error_errno(_(\"unable to open %s\"), source);\n    -+\t\tret = error_errno(_(\"unable to open %s\"), source);\n    - \t\tgoto out;\n    - \t}\n    - \n     @@ object-file.c: static int check_collision(const char *source, const char *dest)\n      \tif (fd_dest < 0) {\n      \t\tif (errno != ENOENT)\n    @@ object-file.c: int finalize_object_file(const char *tmpfile, const char *filenam\n      {\n     -\tstruct stat st;\n     -\tint ret = 0;\n    ++\tunsigned retries = 0;\n     +\tint ret;\n     +\n     +retry:\n    @@ object-file.c: int finalize_object_file_flags(const char *tmpfile, const char *f\n     -\t\t    check_collision(tmpfile, filename))\n     +\t\tif (!(flags & FOF_SKIP_COLLISION_CHECK)) {\n     +\t\t\tret = check_collision(tmpfile, filename);\n    -+\t\t\tif (ret == CHECK_COLLISION_DEST_VANISHED)\n    ++\t\t\tif (ret == CHECK_COLLISION_DEST_VANISHED) {\n    ++\t\t\t\tif (retries++ > 5)\n    ++\t\t\t\t\treturn error(_(\"unable to write repeatedly vanishing file %s\"),\n    ++\t\t\t\t\t\t     filename);\n     +\t\t\t\tgoto retry;\n    ++\t\t\t}\n     +\t\t\telse if (ret)\n      \t\t\t\treturn -1;\n     +\t\t}\n\n---\nbase-commit: 2be278337fd02495a86577a89fbf9387b2df6523\nchange-id: 20250103-b4-pks-object-file-racy-collision-check-a649bbf96a92\n\n"},{"id":"509963","messageId":"20250106-b4-pks-object-file-racy-collision-check-v2-1-8b3984ecbb18@pks.im","threadId":"62728","inReplyTo":"20250106-b4-pks-object-file-racy-collision-check-v2-0-8b3984ecbb18@pks.im","subject":"[PATCH v2 1/3] object-file: rename variables in `check_collision()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-06T09:24:25Z","receivedAt":"2025-01-06T09:24:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Rename variables used in `check_collision()` to clearly identify which\nfile is the source and which is the destination. This will make the next\nstep easier to reason about when we start to treat those files different\nfrom one another.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n object-file.c | 40 ++++++++++++++++++++--------------------\n 1 file changed, 20 insertions(+), 20 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex f84dcd2f2a7b88716ab47bc00ee7a605a82e8d21..e1989236ca87e565dea4d003f57882f257889ecf 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1970,56 +1970,56 @@ static void write_object_file_prepare_literally(const struct git_hash_algo *algo\n \thash_object_body(algo, &c, buf, len, oid, hdr, hdrlen);\n }\n \n-static int check_collision(const char *filename_a, const char *filename_b)\n+static int check_collision(const char *source, const char *dest)\n {\n-\tchar buf_a[4096], buf_b[4096];\n-\tint fd_a = -1, fd_b = -1;\n+\tchar buf_source[4096], buf_dest[4096];\n+\tint fd_source = -1, fd_dest = -1;\n \tint ret = 0;\n \n-\tfd_a = open(filename_a, O_RDONLY);\n-\tif (fd_a < 0) {\n+\tfd_source = open(source, O_RDONLY);\n+\tif (fd_source < 0) {\n \t\tif (errno != ENOENT)\n-\t\t\tret = error_errno(_(\"unable to open %s\"), filename_a);\n+\t\t\tret = error_errno(_(\"unable to open %s\"), source);\n \t\tgoto out;\n \t}\n \n-\tfd_b = open(filename_b, O_RDONLY);\n-\tif (fd_b < 0) {\n+\tfd_dest = open(dest, O_RDONLY);\n+\tif (fd_dest < 0) {\n \t\tif (errno != ENOENT)\n-\t\t\tret = error_errno(_(\"unable to open %s\"), filename_b);\n+\t\t\tret = error_errno(_(\"unable to open %s\"), dest);\n \t\tgoto out;\n \t}\n \n \twhile (1) {\n \t\tssize_t sz_a, sz_b;\n \n-\t\tsz_a = read_in_full(fd_a, buf_a, sizeof(buf_a));\n+\t\tsz_a = read_in_full(fd_source, buf_source, sizeof(buf_source));\n \t\tif (sz_a < 0) {\n-\t\t\tret = error_errno(_(\"unable to read %s\"), filename_a);\n+\t\t\tret = error_errno(_(\"unable to read %s\"), source);\n \t\t\tgoto out;\n \t\t}\n \n-\t\tsz_b = read_in_full(fd_b, buf_b, sizeof(buf_b));\n+\t\tsz_b = read_in_full(fd_dest, buf_dest, sizeof(buf_dest));\n \t\tif (sz_b < 0) {\n-\t\t\tret = error_errno(_(\"unable to read %s\"), filename_b);\n+\t\t\tret = error_errno(_(\"unable to read %s\"), dest);\n \t\t\tgoto out;\n \t\t}\n \n-\t\tif (sz_a != sz_b || memcmp(buf_a, buf_b, sz_a)) {\n+\t\tif (sz_a != sz_b || memcmp(buf_source, buf_dest, sz_a)) {\n \t\t\tret = error(_(\"files '%s' and '%s' differ in contents\"),\n-\t\t\t\t    filename_a, filename_b);\n+\t\t\t\t    source, dest);\n \t\t\tgoto out;\n \t\t}\n \n-\t\tif (sz_a < sizeof(buf_a))\n+\t\tif (sz_a < sizeof(buf_source))\n \t\t\tbreak;\n \t}\n \n out:\n-\tif (fd_a > -1)\n-\t\tclose(fd_a);\n-\tif (fd_b > -1)\n-\t\tclose(fd_b);\n+\tif (fd_source > -1)\n+\t\tclose(fd_source);\n+\tif (fd_dest > -1)\n+\t\tclose(fd_dest);\n \treturn ret;\n }\n \n\n-- \n2.48.0.rc1.245.gb3e6e7acbc.dirty\n\n"},{"id":"509964","messageId":"20250106-b4-pks-object-file-racy-collision-check-v2-2-8b3984ecbb18@pks.im","threadId":"62728","inReplyTo":"20250106-b4-pks-object-file-racy-collision-check-v2-0-8b3984ecbb18@pks.im","subject":"[PATCH v2 2/3] object-file: don't special-case missing source file in collision check","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-06T09:24:26Z","receivedAt":"2025-01-06T09:24:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 0ad3d65652 (object-file: fix race in object collision check,\n2024-12-30) we have started to ignore ENOENT when opening either the\nsource or destination file of the collision check. This was done to\nhandle races more gracefully in case either of the potentially-colliding\ndisappears.\n\nThe fix is overly broad though: while the destination file may indeed\nvanish racily, this shouldn't ever happen for the source file, which is\na temporary object file (either loose or in packfile format) that we\nhave just created. So if any concurrent process would have removed that\ntemporary file it would indicate an actual issue.\n\nStop treating ENOENT specially for the source file so that we always\nbubble up this error.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n object-file.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex e1989236ca87e565dea4d003f57882f257889ecf..acfda5e303659195109c94f5b54b8a081fe92c59 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1978,8 +1978,7 @@ static int check_collision(const char *source, const char *dest)\n \n \tfd_source = open(source, O_RDONLY);\n \tif (fd_source < 0) {\n-\t\tif (errno != ENOENT)\n-\t\t\tret = error_errno(_(\"unable to open %s\"), source);\n+\t\tret = error_errno(_(\"unable to open %s\"), source);\n \t\tgoto out;\n \t}\n \n\n-- \n2.48.0.rc1.245.gb3e6e7acbc.dirty\n\n"},{"id":"509965","messageId":"20250106-b4-pks-object-file-racy-collision-check-v2-3-8b3984ecbb18@pks.im","threadId":"62728","inReplyTo":"20250106-b4-pks-object-file-racy-collision-check-v2-0-8b3984ecbb18@pks.im","subject":"[PATCH v2 3/3] object-file: retry linking file into place when occluding file vanishes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-06T09:24:27Z","receivedAt":"2025-01-06T09:24:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Prior to 0ad3d65652 (object-file: fix race in object collision check,\n2024-12-30), callers could expect that a successful return from\n`finalize_object_file()` means that either the file was moved into\nplace, or the identical bytes were already present. If neither of those\nhappens, we'd return an error.\n\nSince that commit, if the destination file disappears between our\nlink(3p) call and the collision check, we'd return success without\nactually checking the contents, and without retrying the link. This\nsolves the common case that the files were indeed the same, but it means\nthat we may corrupt the repository if they weren't (this implies a hash\ncollision, but the whole point of this function is protecting against\nhash collisions).\n\nWe can't be pessimistic and assume they're different; that hurts the\ncommon case that the mentioned commit was trying to fix. But after\nseeing that the destination file went away, we can retry linking again.\nAdapt the code to do so when we see that the destination file has racily\nvanished. This should generally succeed as we have just observed that\nthe destination file does not exist anymore, except in the very unlikely\nevent that it gets recreated by another concurrent process again.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n object-file.c | 25 +++++++++++++++++++++----\n 1 file changed, 21 insertions(+), 4 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex acfda5e303659195109c94f5b54b8a081fe92c59..aeca61b8ae3aadecef1ce11306b38e7368af829f 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1970,6 +1970,8 @@ static void write_object_file_prepare_literally(const struct git_hash_algo *algo\n \thash_object_body(algo, &c, buf, len, oid, hdr, hdrlen);\n }\n \n+#define CHECK_COLLISION_DEST_VANISHED -2\n+\n static int check_collision(const char *source, const char *dest)\n {\n \tchar buf_source[4096], buf_dest[4096];\n@@ -1986,6 +1988,8 @@ static int check_collision(const char *source, const char *dest)\n \tif (fd_dest < 0) {\n \t\tif (errno != ENOENT)\n \t\t\tret = error_errno(_(\"unable to open %s\"), dest);\n+\t\telse\n+\t\t\tret = CHECK_COLLISION_DEST_VANISHED;\n \t\tgoto out;\n \t}\n \n@@ -2033,8 +2037,11 @@ int finalize_object_file(const char *tmpfile, const char *filename)\n int finalize_object_file_flags(const char *tmpfile, const char *filename,\n \t\t\t       enum finalize_object_file_flags flags)\n {\n-\tstruct stat st;\n-\tint ret = 0;\n+\tunsigned retries = 0;\n+\tint ret;\n+\n+retry:\n+\tret = 0;\n \n \tif (object_creation_mode == OBJECT_CREATION_USES_RENAMES)\n \t\tgoto try_rename;\n@@ -2055,6 +2062,8 @@ int finalize_object_file_flags(const char *tmpfile, const char *filename,\n \t * left to unlink.\n \t */\n \tif (ret && ret != EEXIST) {\n+\t\tstruct stat st;\n+\n \ttry_rename:\n \t\tif (!stat(filename, &st))\n \t\t\tret = EEXIST;\n@@ -2070,9 +2079,17 @@ int finalize_object_file_flags(const char *tmpfile, const char *filename,\n \t\t\terrno = saved_errno;\n \t\t\treturn error_errno(_(\"unable to write file %s\"), filename);\n \t\t}\n-\t\tif (!(flags & FOF_SKIP_COLLISION_CHECK) &&\n-\t\t    check_collision(tmpfile, filename))\n+\t\tif (!(flags & FOF_SKIP_COLLISION_CHECK)) {\n+\t\t\tret = check_collision(tmpfile, filename);\n+\t\t\tif (ret == CHECK_COLLISION_DEST_VANISHED) {\n+\t\t\t\tif (retries++ > 5)\n+\t\t\t\t\treturn error(_(\"unable to write repeatedly vanishing file %s\"),\n+\t\t\t\t\t\t     filename);\n+\t\t\t\tgoto retry;\n+\t\t\t}\n+\t\t\telse if (ret)\n \t\t\t\treturn -1;\n+\t\t}\n \t\tunlink_or_warn(tmpfile);\n \t}\n \n\n-- \n2.48.0.rc1.245.gb3e6e7acbc.dirty\n\n"},{"id":"509971","messageId":"Z3u6bzyaDyzjV65X@pks.im","threadId":"62728","inReplyTo":"20250103194058.GE3208749@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] object-file: retry linking file into place when occluding file vanishes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-06T11:11:43Z","receivedAt":"2025-01-06T11:11:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 03, 2025 at 02:40:58PM -0500, Jeff King wrote:\n> On Fri, Jan 03, 2025 at 09:19:55AM +0100, Patrick Steinhardt wrote:\n> \n> > In a preceding commit, we have adapted `check_collision()` to ignore\n> \n> If it's in next and we are building on top, I think we can mention it by\n> name: 0ad3d65652 (object-file: fix race in object collision check,\n> 2024-12-30).\n\nOkay, good to know. I wasn't sure whether the patches might get rewound\nwhen `next` gets rewound.\n\n> > the case where either of the colliding files vanishes. This should be\n> > safe in general when we assume that the contents of these two files were\n> > the same. But the check is all about detecting collisions, so that\n> > assumption may be too optimistic.\n> \n> I found this a little vague about what \"too optimistic\" means. ;)\n> Maybe something like:\n> \n>   Prior to 0ad3d65652, callers could expect that a successful return\n>   from finalize_object_file() means that either the file was moved into\n>   place, or the identical bytes were already present. If neither of\n>   those happens, we'd return an error.\n> \n>   Since that commit, if the destination file disappears between our\n>   link() call and the collision check, we'd return success without\n>   actually checking the contents, and without retrying the link. This\n>   solves the common case that the files were indeed the same, but it\n>   means that we may corrupt the repository if they weren't (this implies\n>   a hash collision, but the whole point of this function is protecting\n>   against hash collisions).\n> \n>   We can't be pessimistic and assume they're different; that hurts the\n>   common case that 0ad3d65652 was trying to fix. But after seeing that\n>   the destination file went away, we can retry linking again...\n\nWell, as usual your commit messages are something to aspire to :)\nThanks.\n\n> > Furthermore, stop treating `ENOENT` specially for the source file. It\n> > shouldn't happen that the source vanishes as we're using a fresh\n> > temporary file for it, so if it does vanish it indicates an actual\n> > error.\n> \n> OK. I think this is worth doing, but I'd probably have put it into its\n> own commit.\n\nFair enough, can do.\n\n> > @@ -2034,8 +2037,10 @@ int finalize_object_file(const char *tmpfile, const char *filename)\n> >  int finalize_object_file_flags(const char *tmpfile, const char *filename,\n> >  \t\t\t       enum finalize_object_file_flags flags)\n> >  {\n> > -\tstruct stat st;\n> > -\tint ret = 0;\n> > +\tint ret;\n> > +\n> > +retry:\n> > +\tret = 0;\n> >  \n> >  \tif (object_creation_mode == OBJECT_CREATION_USES_RENAMES)\n> >  \t\tgoto try_rename;\n> > @@ -2056,6 +2061,8 @@ int finalize_object_file_flags(const char *tmpfile, const char *filename,\n> >  \t * left to unlink.\n> >  \t */\n> >  \tif (ret && ret != EEXIST) {\n> > +\t\tstruct stat st;\n> > +\n> \n> OK, we move the stat struct here where it's needed. I think that's\n> somewhat orthogonal to your patch, but reduced scoping does help make\n> the goto's less confusing.\n> \n> I suspect there's a way to write this as a loop that would be more\n> structured, but it would be a bigger refactor. Bonus points if it also\n> get rid of the try_rename goto, too. ;)\n> \n> I'm OK punting on that, though.\n\nYeah, I'll punt on it for now. I don't love the resulting structure, but\nit's also not that uncommon in our codebase.\n\n> > @@ -2071,9 +2078,13 @@ int finalize_object_file_flags(const char *tmpfile, const char *filename,\n> >  \t\t\terrno = saved_errno;\n> >  \t\t\treturn error_errno(_(\"unable to write file %s\"), filename);\n> >  \t\t}\n> > -\t\tif (!(flags & FOF_SKIP_COLLISION_CHECK) &&\n> > -\t\t    check_collision(tmpfile, filename))\n> > +\t\tif (!(flags & FOF_SKIP_COLLISION_CHECK)) {\n> > +\t\t\tret = check_collision(tmpfile, filename);\n> > +\t\t\tif (ret == CHECK_COLLISION_DEST_VANISHED)\n> > +\t\t\t\tgoto retry;\n> > +\t\t\telse if (ret)\n> >  \t\t\t\treturn -1;\n> > +\t\t}\n> >  \t\tunlink_or_warn(tmpfile);\n> >  \t}\n> \n> I share Junio's uneasiness with looping forever based on external input\n> from the filesystem (even though you _should_ eventually win the race,\n> that's not guaranteed, and of course a weird filesystem might confuse\n> us). Could we put a stop-gap in it like:\n\nFair enough. I was also wondering about whether or not I should have a\nretry counter when writing it but couldn't think about a (sane) scenario\nwhere it would be needed. But yeah, filesystems can be weird, and it's\nnot a lot of code either, so I'll add it.\n\nPatrick\n"},{"id":"509972","messageId":"Z3u6c1UVQyZuHLdk@pks.im","threadId":"62728","inReplyTo":"20250103195942.GA3212696@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] object-file: retry linking file into place when occluding file vanishes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-06T11:11:47Z","receivedAt":"2025-01-06T11:11:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 03, 2025 at 02:59:42PM -0500, Jeff King wrote:\n> On Fri, Jan 03, 2025 at 02:40:58PM -0500, Jeff King wrote:\n> \n> > I suspect there's a way to write this as a loop that would be more\n> > structured, but it would be a bigger refactor. Bonus points if it also\n> > get rid of the try_rename goto, too. ;)\n> > \n> > I'm OK punting on that, though.\n> \n> For fun, here's a version without any goto's in it, that should behave\n> the same. But it would be very easy to miss a case. So I don't know if\n> it is worth the regression risk, and I don't blame you if you delete\n> this message without looking carefully. ;)\n> \n> Diff is kind of hard to read, so you may want to apply (on top of your\n> patches) and just look at the post-image.\n\nThanks. For now though I think I prefer to go with the simpler diff that\nuses goto, as it feels less risky close to v2.48. We can still refactor\nthis in the next release cycle.\n\nPatrick\n"},{"id":"510038","messageId":"20250107012547.GA2363@coredump.intra.peff.net","threadId":"62728","inReplyTo":"Z3u6c1UVQyZuHLdk@pks.im","subject":"Re: [PATCH 2/2] object-file: retry linking file into place when occluding file vanishes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-07T01:25:47Z","receivedAt":"2025-01-07T01:25:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 06, 2025 at 12:11:47PM +0100, Patrick Steinhardt wrote:\n\n> On Fri, Jan 03, 2025 at 02:59:42PM -0500, Jeff King wrote:\n> > On Fri, Jan 03, 2025 at 02:40:58PM -0500, Jeff King wrote:\n> > \n> > > I suspect there's a way to write this as a loop that would be more\n> > > structured, but it would be a bigger refactor. Bonus points if it also\n> > > get rid of the try_rename goto, too. ;)\n> > > \n> > > I'm OK punting on that, though.\n> > \n> > For fun, here's a version without any goto's in it, that should behave\n> > the same. But it would be very easy to miss a case. So I don't know if\n> > it is worth the regression risk, and I don't blame you if you delete\n> > this message without looking carefully. ;)\n> > \n> > Diff is kind of hard to read, so you may want to apply (on top of your\n> > patches) and just look at the post-image.\n> \n> Thanks. For now though I think I prefer to go with the simpler diff that\n> uses goto, as it feels less risky close to v2.48. We can still refactor\n> this in the next release cycle.\n\nSounds good. I looked over your v2 and it seems fine to me.\n\n-Peff\n"}]}