From: shejialuo <shejialuo@gmail.com>
Date: Sun, 05 Oct 2025 08:19:21 GMT
Subject: Re: [PATCH 1/1] files-backend: check symref name before update
Message-ID: <aOIqCY4vC0Eqiz_M@ArchLinux>
In-Reply-To: <aN5mOTbGBcr355E6@pks.im>

On Thu, Oct 02, 2025 at 01:47:05PM +0200, Patrick Steinhardt wrote:
> 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.

IIRC, I intentionally didn't implement the code to check the HEAD. HEAD
is so special as its correctness also indicates whether the current
directory is a Git directory. We would check whether the content of HEAD
starts with "refs: ". If not, we would error the user "fatal: not a git
repository".

So, the only check we should do for HEAD is check whether the symref is
valid. When I implemented the code, I think I wrongly forgot to add such
check.

Thanks,
Jialuo