On Tue, Dec 20, 2022 at 03:52:14PM +0100, Patrick Steinhardt wrote:
> And while we do the dance when writing the `packed-refs` file, there is
> indeed one gotcha: we use a `FILE *` stream to write the temporary file,
> but don't flush it before synchronizing it to disk. As a consequence any
> data that is still buffered will not get synchronized and a crash of the
> machine may cause corruption.
The problem description makes sense, and so does your fix.
Grepping for other uses of fsync_component(), this looks like the only
buggy case (loose refs use write() directly, and most other files go via
finalize_hashfile(), which does likewise).
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index c1c71d183e..6f5a0709fb 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -1263,7 +1263,8 @@ static int write_with_updates(struct packed_ref_store *refs,
> goto error;
> }
>
> - if (fsync_component(FSYNC_COMPONENT_REFERENCE, get_tempfile_fd(refs->tempfile)) ||
> + if (fflush(out) ||
> + fsync_component(FSYNC_COMPONENT_REFERENCE, get_tempfile_fd(refs->tempfile)) ||
> close_tempfile_gently(refs->tempfile)) {
It kind of feels like this ought to be part of fsync_component() or
close_tempfile_gently(), but it would pollute those interfaces:
- fsync_component() doesn't otherwise know about stdio
- close_tempfile_gently() doesn't otherwise know about syncing (and it
would have to learn about fsync_components to do it right).
So given that this is the only affected site, it makes sense to just fix
it for now and worry about a more generalized solution if we run into it
again.
-Peff
