Re: [PATCH 7/8] core.fsync: new option to harden loose references

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2202 bytes --]

On Thu, Mar 10, 2022 at 10:40:07PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > @@ -1504,6 +1513,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
> >  	oidcpy(&lock->old_oid, &orig_oid);
> >  
> >  	if (write_ref_to_lockfile(lock, &orig_oid, 0, &err) ||
> > +	    files_sync_loose_ref(lock, &err) ||
> >  	    commit_ref_update(refs, lock, &orig_oid, logmsg, &err)) {
> >  		error("unable to write current sha1 into %s: %s", newrefname, err.buf);
> >  		strbuf_release(&err);
> 
> Given that write_ref_to_lockfile() on the success code path does this:
> 
> 	fd = get_lock_file_fd(&lock->lk);
> 	if (write_in_full(fd, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||
> 	    write_in_full(fd, &term, 1) < 0 ||
> 	    close_ref_gently(lock) < 0) {
> 		strbuf_addf(err,
> 			    "couldn't write '%s'", get_lock_file_path(&lock->lk));
> 		unlock_ref(lock);
> 		return -1;
> 	}
> 	return 0;
> 
> the above unfortunately does not work.  By the time the new call to
> files_sync_loose_ref() is made, lock->fd is closed by the call to
> close_lock_file_gently() made in close_ref_gently(), and because of
> that, you'll get an error like this:
> 
>     Writing objects: 100% (3/3), 279 bytes | 279.00 KiB/s, done.
>     Total 3 (delta 0), reused 0 (delta 0), pack-reused 0
>     remote: error: could not sync loose ref 'refs/heads/client_branch':
>     Bad file descriptor     
> 
> when running "make test" (the above is from t5702 but I wouldn't be
> surprised if this broke ALL ref updates).
> 
> Just before write_ref_to_lockfile() calls close_ref_gently() would
> be a good place to make the fsync_loose_ref() call, perhaps?
> 
> 
> Thanks.

Yeah, that thought indeed occurred to me this night, too. I was hoping
that I could fix this before anybody noticed ;)

It's a bit unfortunate that we can't just defer this to a later place to
hopefully implement this more efficiently, but so be it. The alternative
would be to re-open all locked loose refs and then sync them to disk,
but this would likely be a lot more painful than just syncing them to
disk before closing it.

Will fix, thanks.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]