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

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