Re: [PATCH 1/1] files-backend: check symref name before update

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> On Thu, Oct 02, 2025 at 02:54:54AM -0700, Karthik Nayak wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>> 
>> > Han Young <hanyang.tony@bytedance.com> writes:
>> >
>> >> From: Han Young <hanyoung@protonmail.com>
>> >>
>> >> In the ref files backend, the symbolic reference name is not checked
>> >> before an update. This could cause reference and lock files to be created
>> >> outside the refs/ directory. Validate the reference before adding it to
>> >> the ref update transaction.
>> >>
>> >> Reported-by: Sigma <git@sigma-star.io>
>> >> Signed-off-by: Han Young <hanyoung@protonmail.com>
>> >> ---
>> >>  refs/files-backend.c | 10 ++++++++++
>> >>  1 file changed, 10 insertions(+)
>> >>
>> >> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> >> index bc3347d18..d47a8c392 100644
>> >> --- a/refs/files-backend.c
>> >> +++ b/refs/files-backend.c
>> >> @@ -2516,6 +2516,16 @@ static enum ref_transaction_error split_symref_update(struct ref_update *update,
>> >>  	struct ref_update *new_update;
>> >>  	unsigned int new_flags;
>> >>
>> >> +	/*
>> >> +	 * Check the referent is valid before adding it to the transaction.
>> >> +	 */
>> >> +	if (!refname_is_safe(referent)) {
>> >
>> > Shouldn't this new condition share the logic with what is done by
>> > fsck?  IOW, after doing this
>> >
>> >   $ echo ref: refs/../HEAD > .git/HEAD
>> >
>> > "git fsck" or "git refs verify" should barf (if not, we should make
>> > them barf), and this code should use the same logic to notice that
>> > the target of the symbolic ref is bogus.
>> >
>> 
>> Good point. I see that 'git fsck' does complain about this:
>> 
>>   $ git fsck
>>   Checking ref database: 100% (1/1), done.
>>   Checking object directories: 100% (256/256), done.
>>   error: invalid HEAD
>>   dangling commit ccd1771e44a18887197d3ee26ca37c2e892b9fb6
>>   dangling commit f99d68ea2c378218e2360dee4e24115c404f6a66
>> 
>> However 'git refs verify' doesn't...
>> 
>>   $ git refs verify --verbose
>>   Checking references consistency
>>   Checking refs/heads/master
>>   Checking packed-refs file .git/packed-refs
>> 
>> Okay, so this seems like because fsck also parses all references to mark
>> reachability and also parses 'HEAD' via `refs_resolve_ref_unsafe()`
>> which fails.
>> 
>> This symref checks and checking root refs is definitely something we
>> should consider adding to 'git refs verify'.
>
> Agreed! Overall, the goal is that all logic to verify references should
> be contained in `git refs verify`, so that git-fsck(1) only needs to
> shell out to that command to perform the full check.
>
> So if this logic isn't yet part of `git refs verify`, we should migrate
> it over.

Absolutely.  As "git refs verify" is a way to do the sanity check of
the ref part (presumably without incurring cost to sanity check
other aspect, like fsck does?  why is it a separate command in the
first place?), it should learn how to do so.  "git fsck" should keep
complaining about the failure as before, whether it is done natively
or by delegating to "git refs verify".

Thanks.