Re: [PATCH] refs: fix corruption by not correctly syncing packed-refs to disk

From: Jeff King <peff@peff.net>
Date: Dec 20, 2022, 15:44 UTC
Message-ID: <Y6HYTt58iecdBEdw@coredump.intra.peff.net>
In-Reply-To: <8c8ecf8e3718cbca049ee7a283edd7b7887e464e.1671547905.git.ps@pks.im>

On Tue, Dec 20, 2022 at 03:52:14PM +0100, Patrick Steinhardt wrote:

Show 5 quoted lines

> 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).

Show 12 quoted lines

> 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