threads / patch / 64233

patchfiles-backend: check symref name before update

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

## tl;dr

11 messages between Oct 1, 2025 and Oct 5, 2025. Diffs are folded; open one to read it.

replies: 10people: 5as markdown or json

Han Young· Oct 1, 2025, 15:08 UTC · lore
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
Han Young· Oct 1, 2025, 15:08 UTC · re: Han Young · lore

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

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(+)
Show changes to refs/files-backend.c +10 −0
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
Junio C Hamano· Oct 1, 2025, 19:22 UTC · re: Han Young · lore

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

Han Young <hanyang.tony@bytedance.com> writes:
Show 25 quoted lines
> 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.

Show 9 quoted lines
> +		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.
Karthik Nayak· Oct 2, 2025, 09:54 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 37 quoted lines
> 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]
Patrick Steinhardt· Oct 2, 2025, 11:47 UTC · re: Karthik Nayak · lore

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

On Thu, Oct 02, 2025 at 02:54:54AM -0700, Karthik Nayak wrote:
Show 62 quoted lines
> 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
Junio C Hamano· Oct 2, 2025, 13:36 UTC · re: Patrick Steinhardt · lore

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

Patrick Steinhardt <ps@pks.im> writes:
Show 70 quoted lines
> 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.
Patrick Steinhardt· Oct 2, 2025, 15:30 UTC · re: Junio C Hamano · lore

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

On Thu, Oct 02, 2025 at 06:36:07AM -0700, Junio C Hamano wrote:
Show 12 quoted lines
> 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
Junio C Hamano· Oct 2, 2025, 17:34 UTC · re: Patrick Steinhardt · lore

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

Patrick Steinhardt <ps@pks.im> writes:
Show 12 quoted lines
>> ...  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.

shejialuo· Oct 5, 2025, 08:19 UTC · re: Patrick Steinhardt · lore

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

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

Karthik Nayak· Oct 2, 2025, 09:34 UTC · re: Han Young · lore

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

Han Young <hanyang.tony@bytedance.com> writes:
Show 24 quoted lines
> 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.

Show 8 quoted lines
> 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
Junio C Hamano· Oct 2, 2025, 14:45 UTC · re: Karthik Nayak · lore

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

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.

← back to recent threads