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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 11, 2022, 06:40 UTC
Message-ID: <xmqqzglx9em0.fsf@gitster.g>
In-Reply-To: <f1e8a7bb3bf0f4c0414819cb1d5579dc08fd2a4f.1646905589.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 8 quoted lines

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