{"thread":{"id":"58987","subject":"[PATCH] refs: fix corruption by not correctly syncing packed-refs to disk","startedAt":"2022-12-20T14:53:05Z","lastAt":"2022-12-25T11:26:09Z","messageCount":3,"participants":["Patrick Steinhardt","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"469386","messageId":"8c8ecf8e3718cbca049ee7a283edd7b7887e464e.1671547905.git.ps@pks.im","threadId":"58987","inReplyTo":null,"subject":"[PATCH] refs: fix corruption by not correctly syncing packed-refs to disk","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-12-20T14:52:14Z","receivedAt":"2022-12-20T14:53:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"At GitLab we have recently received a report where a repository was left\nwith a corrupted `packed-refs` file after the node hard-crashed even\nthough `core.fsync=reference` was set. This is something that in theory\nshould not happen if we correctly did the atomic-rename dance to:\n\n    1. Write the data into a temporary file.\n\n    2. Synchronize the temporary file to disk.\n\n    3. Rename the temporary file into place.\n\nSo if we crash in the middle of writing the `packed-refs` file we should\nonly ever see either the old or the new state of the file.\n\nAnd while we do the dance when writing the `packed-refs` file, there is\nindeed one gotcha: we use a `FILE *` stream to write the temporary file,\nbut don't flush it before synchronizing it to disk. As a consequence any\ndata that is still buffered will not get synchronized and a crash of the\nmachine may cause corruption.\n\nFix this bug by flushing the file stream before we fsync.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n refs/packed-backend.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex c1c71d183e..6f5a0709fb 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1263,7 +1263,8 @@ static int write_with_updates(struct packed_ref_store *refs,\n \t\tgoto error;\n \t}\n \n-\tif (fsync_component(FSYNC_COMPONENT_REFERENCE, get_tempfile_fd(refs->tempfile)) ||\n+\tif (fflush(out) ||\n+\t    fsync_component(FSYNC_COMPONENT_REFERENCE, get_tempfile_fd(refs->tempfile)) ||\n \t    close_tempfile_gently(refs->tempfile)) {\n \t\tstrbuf_addf(err, \"error closing file %s: %s\",\n \t\t\t    get_tempfile_path(refs->tempfile),\n-- \n2.39.0\n\n"},{"id":"469387","messageId":"Y6HYTt58iecdBEdw@coredump.intra.peff.net","threadId":"58987","inReplyTo":"8c8ecf8e3718cbca049ee7a283edd7b7887e464e.1671547905.git.ps@pks.im","subject":"Re: [PATCH] refs: fix corruption by not correctly syncing packed-refs to disk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-20T15:44:14Z","receivedAt":"2022-12-20T15:44:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 20, 2022 at 03:52:14PM +0100, Patrick Steinhardt wrote:\n\n> And while we do the dance when writing the `packed-refs` file, there is\n> indeed one gotcha: we use a `FILE *` stream to write the temporary file,\n> but don't flush it before synchronizing it to disk. As a consequence any\n> data that is still buffered will not get synchronized and a crash of the\n> machine may cause corruption.\n\nThe problem description makes sense, and so does your fix.\n\nGrepping for other uses of fsync_component(), this looks like the only\nbuggy case (loose refs use write() directly, and most other files go via\nfinalize_hashfile(), which does likewise).\n\n> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n> index c1c71d183e..6f5a0709fb 100644\n> --- a/refs/packed-backend.c\n> +++ b/refs/packed-backend.c\n> @@ -1263,7 +1263,8 @@ static int write_with_updates(struct packed_ref_store *refs,\n>  \t\tgoto error;\n>  \t}\n>  \n> -\tif (fsync_component(FSYNC_COMPONENT_REFERENCE, get_tempfile_fd(refs->tempfile)) ||\n> +\tif (fflush(out) ||\n> +\t    fsync_component(FSYNC_COMPONENT_REFERENCE, get_tempfile_fd(refs->tempfile)) ||\n>  \t    close_tempfile_gently(refs->tempfile)) {\n\nIt kind of feels like this ought to be part of fsync_component() or\nclose_tempfile_gently(), but it would pollute those interfaces:\n\n  - fsync_component() doesn't otherwise know about stdio\n\n  - close_tempfile_gently() doesn't otherwise know about syncing (and it\n    would have to learn about fsync_components to do it right).\n\nSo given that this is the only affected site, it makes sense to just fix\nit for now and worry about a more generalized solution if we run into it\nagain.\n\n-Peff\n"},{"id":"469521","messageId":"xmqqtu1johbs.fsf@gitster.g","threadId":"58987","inReplyTo":"Y6HYTt58iecdBEdw@coredump.intra.peff.net","subject":"Re: [PATCH] refs: fix corruption by not correctly syncing packed-refs to disk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-25T11:25:59Z","receivedAt":"2022-12-25T11:26:09Z","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> On Tue, Dec 20, 2022 at 03:52:14PM +0100, Patrick Steinhardt wrote:\n>\n>> And while we do the dance when writing the `packed-refs` file, there is\n>> indeed one gotcha: we use a `FILE *` stream to write the temporary file,\n>> but don't flush it before synchronizing it to disk. As a consequence any\n>> data that is still buffered will not get synchronized and a crash of the\n>> machine may cause corruption.\n>\n> The problem description makes sense, and so does your fix.\n>\n> Grepping for other uses of fsync_component(), this looks like the only\n> buggy case (loose refs use write() directly, and most other files go via\n> finalize_hashfile(), which does likewise).\n> ...\n> So given that this is the only affected site, it makes sense to just fix\n> it for now and worry about a more generalized solution if we run into it\n> again.\n\nSounds good.\n\nThis came from bc22d845 (core.fsync: new option to harden\nreferences, 2022-03-11), before which we did not even fsync() the\nfile, so let me apply directly on top of that commit.  Those who are\nstuck on older versions of Git can choose to merge the result, even\nthough I may probably not bother merging it down to anything older\nthan 2.39 maintenance track.\n\nThanks.\n\n\n"}]}