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

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

[Han Young]

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.

Below are the original bug report by Sigma:

  $ echo ref: refs/../HEAD > .git/HEAD
  $ git commit -m "test" --allow-empty
  fatal: cannot lock ref 'HEAD': Unable to create '/home/sigma/headtest/.git/refs/../HEAD.lock': File exists.

  Another git process seems to be running in this repository, e.g.
  an editor opened by 'git commit'. Please make sure all processes
  are terminated then try again. If it still fails, a git process
  may have crashed in this repository earlier:
  remove the file manually to continue.

In this case, while trying to update the symbolic reference refs/../HEAD,
the lock file conflicts with the ./git/HEAD.lock.

If the HEAD points to refs/../foo, a reference file named foo will be
created under ./git directory.

Han Young (1):
  files-backend: check symref name before update

 refs/files-backend.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

-- 
2.51.0.373.gaf4ee0e35.dirty

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

[Han Young]

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)) {
+		strbuf_addf(err,
+			    "reference '%s' appears to be broken",
+			    update->refname);
+		return -1;
+	}
+
 	/*
 	 * First make sure that referent is not already in the
 	 * transaction. This check is O(lg N) in the transaction
-- 
2.51.0.373.gaf4ee0e35.dirty

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

[Junio C Hamano]

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.

> +		strbuf_addf(err,
> +			    "reference '%s' appears to be broken",
> +			    update->refname);
> +		return -1;
> +	}
> +
>  	/*
>  	 * First make sure that referent is not already in the
>  	 * transaction. This check is O(lg N) in the transaction

Can we also have some tests?

Thanks.

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

[Karthik Nayak]

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

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

[snip]

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

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

[Patrick Steinhardt]

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.

Patrick

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

[Junio C Hamano]

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.

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

[Patrick Steinhardt]

On Thu, Oct 02, 2025 at 06:36:07AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > 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.

We have the same pattern in other command:

    - git commit-graph verify
    - git multi-pack-index verify
    - git bundle verify

So `git refs verify` is following the same direction.

I think it's a nice pattern to have this encapsulated functionality so
that it's easy to exercise certain subsystems in isolation. git-fsck(1)
then becomes a thin wrapper around these commands and is the one that
ties it all together, if desired.

> "git fsck" should keep complaining about the failure as before,
> whether it is done natively or by delegating to "git refs verify".

Yup.

Patrick

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> ...  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?), ...
>
> We have the same pattern in other command:
>
>     - git commit-graph verify
>     - git multi-pack-index verify
>     - git bundle verify
>
> So `git refs verify` is following the same direction.

Well, bundle falls into a searate category, though.

A bundle file is a thing on its own and wants to be independently
verifiable.  A packfile (.pack alone without .idx) is also a thing
that may want to be independently verifiable.  For that they need
to be accessible by end-users in a form of some command.

But everything else, ...

> I think it's a nice pattern to have this encapsulated functionality so
> that it's easy to exercise certain subsystems in isolation. git-fsck(1)
> then becomes a thin wrapper around these commands and is the one that
> ties it all together, if desired.

... including refs, commit-graphs, multi-pack-index do not have life
on their own outside the repository they originate in, so there is
no reason to expose them as separate commands to end-users.

I do agree that having a separate entry point for exercising them
and them alone would help debugging and development, but such an
entry point does not have to be a separate binary.  It could have
been "git fsck --refs-only" instead, for example.

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

[shejialuo]

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

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

[Karthik Nayak]

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

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.
>
> Below are the original bug report by Sigma:
>
>   $ echo ref: refs/../HEAD > .git/HEAD
>   $ git commit -m "test" --allow-empty
>   fatal: cannot lock ref 'HEAD': Unable to create '/home/sigma/headtest/.git/refs/../HEAD.lock': File exists.
>
>   Another git process seems to be running in this repository, e.g.
>   an editor opened by 'git commit'. Please make sure all processes
>   are terminated then try again. If it still fails, a git process
>   may have crashed in this repository earlier:
>   remove the file manually to continue.
>
> In this case, while trying to update the symbolic reference refs/../HEAD,
> the lock file conflicts with the ./git/HEAD.lock.
>
> If the HEAD points to refs/../foo, a reference file named foo will be
> created under ./git directory.
>

I quickly checked if this can also be done by using 'git-update-ref(1)'.
But the command calls on 'check_refname_format()' to check the new ref
for the symref update and fails:

  $ git update-ref --stdin
  symref-update HEAD refs/../HEAD
  fatal: invalid ref format: refs/../HEAD

So this is only possible by manually editing the .git/HEAD file, right?

In that case, isn't the repository already broken?

In other words, the fix seem to only stop us from creating files outside
the $GIT_DIR, but this seems like something that the user would have to
orchestrate intentionally.

The bigger question for me is if there is an instance that you'd want to
modify the HEAD file manually. Or is there a way this can be done via
any of the existing Git commands. Otherwise, I'm not sure I would call
this a bug.

> Han Young (1):
>   files-backend: check symref name before update
>
>  refs/files-backend.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> --
> 2.51.0.373.gaf4ee0e35.dirty

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

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

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> The bigger question for me is if there is an instance that you'd want to
> modify the HEAD file manually. Or is there a way this can be done via
> any of the existing Git commands. Otherwise, I'm not sure I would call
> this a bug.

As we discussed downthread, I tend to agree with you here that this
is a corrupt repository leading the tool to do a nonsensical thing,
and the true bug here is that "fsck" is not catching it.  But I do
not mind if we add a check at runtime, probably at the location the
patch under discussion identified, that makes sure a symbolic ref
points at a valid target the same way "git fsck" does.

Thanks.